On Sun, Nov 29, 2015 at 01:09:29AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > On Fri, Nov 27, 2015 at 11:54:17AM +0100, Florian Westphal wrote:
> > > Patrick McHardy <kaber@xxxxxxxxx> wrote:
> > > > Yes, I also did that and it looks correct. I think we probably have a
> > > > discrepancy with bit numbering:
> > > >
> > > > Looking at an older patch of you:
> > > >
> > > > - [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 0, 4),
> > > > - [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 4, 4),
> > > > + [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 4, 4),
> > > > + [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 0, 4),
> > > >
> > > > So you seem to assume a numbering which corresponds to how you would express
> > > > it in C. My patch assumes numbering as used in the RFCs/IEEE standards, which
> > > > is basically the opposite direction.
> > >
> > > Right, there is a general problem with all sub-byte fields.
> > >
> > > I just noticed that decoding of ip version/hdrlen doesn't work either.
> > > (ip hdrlength 4 ip version 5).
> > >
> > > I am sure that I tested matching on ip version/hdrlen on both
> > > x86-64 and a MSB machine (don't recall architecture, ppc i think).
> >
> > The existing approach works fine in x86-64 and ppc here.
> >
> > pahole also reports that version bitfield offset starts at 0, then
> > hdrlength starts at 4, both in x86-64 and ppc.
> >
> > Probably the problem is the way we calculate the shifts. I managed to
> > set the offset according to RFCs/IEEE by adjusting the existing
> > arithmetics, I think the offset semantics was accidentally changes
> > with this first approach to address sub-byte matching.
>
> Thanks for looking at this. I'll take a closer look tomorrow,
> your patch works fine for ip version/hdrlength but seems it messes
> with endianess somewhere.
I forgot to update payload_shift_value() too, to skip the shift when
not needed, sorry, new patch attached.
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 0790dce..579f038 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -109,21 +109,21 @@ static void netlink_gen_payload_mask(struct netlink_linearize_ctx *ctx,
{
struct nft_data_linearize nld, zero = {};
struct nftnl_expr *nle;
- unsigned int offset, len, masklen;
+ unsigned int shift, len, masklen;
mpz_t mask;
- offset = expr->payload.offset % BITS_PER_BYTE;
- masklen = expr->len + offset;
+ shift = BITS_PER_BYTE - ((expr->payload.offset + expr->len) % BITS_PER_BYTE);
+ masklen = expr->len + shift;
if (masklen > 128)
- BUG("expr mask length is %u (len %u, offset %u)\n",
- masklen, expr->len, offset);
+ BUG("expr mask length is %u (len %u, shift %u)\n",
+ masklen, expr->len, shift);
mpz_init2(mask, masklen);
mpz_bitmask(mask, expr->len);
- if (offset)
- mpz_lshift_ui(mask, offset);
+ if (shift)
+ mpz_lshift_ui(mask, shift);
nle = alloc_nft_expr("bitwise");
@@ -158,7 +158,8 @@ static void netlink_gen_payload(struct netlink_linearize_ctx *ctx,
nftnl_rule_add_expr(ctx->nlr, nle);
- if (expr->len % BITS_PER_BYTE)
+ if (expr->payload.offset % BITS_PER_BYTE ||
+ (expr->payload.offset + expr->len) % BITS_PER_BYTE)
netlink_gen_payload_mask(ctx, expr, dreg);
}
@@ -283,11 +284,17 @@ static void netlink_gen_range(struct netlink_linearize_ctx *ctx,
static void payload_shift_value(const struct expr *left, struct expr *right)
{
+ unsigned int shift;
+
if (right->ops->type != EXPR_VALUE ||
left->ops->type != EXPR_PAYLOAD)
return;
- mpz_lshift_ui(right->value, left->payload.offset % BITS_PER_BYTE);
+ if (left->payload.offset % BITS_PER_BYTE ||
+ (left->payload.offset + left->len) % BITS_PER_BYTE) {
+ shift = BITS_PER_BYTE - ((left->payload.offset + left->len) % BITS_PER_BYTE);
+ mpz_lshift_ui(right->value, shift);
+ }
}
static struct expr *netlink_gen_prefix(struct netlink_linearize_ctx *ctx,
diff --git a/src/proto.c b/src/proto.c
index 0fe0b88..9839124 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -508,8 +508,8 @@ const struct proto_desc proto_ip = {
PROTO_LINK(IPPROTO_SCTP, &proto_sctp),
},
.templates = {
- [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 4, 4),
- [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 0, 4),
+ [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 0, 4),
+ [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 4, 4),
[IPHDR_TOS] = IPHDR_FIELD("tos", tos),
[IPHDR_LENGTH] = IPHDR_FIELD("length", tot_len),
[IPHDR_ID] = IPHDR_FIELD("id", id),
