Re: [PATCH nft] proto: fix VLAN header definition

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

On Fri, Nov 27, 2015 at 11:54:17AM +0100, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> 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.

See attached proof-of-concept patch.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 2506 bytes --]

diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 0790dce..751c0a3 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,14 @@ 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);
+	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),