[PATCH nft] proto: fix VLAN header definition

[PATCH nft] proto: fix VLAN header definition

[Patrick McHardy]

The VID is located after Priority and CFI.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/proto.h | 4 ++--
 src/proto.c     | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/proto.h b/include/proto.h
index 974116f..d90bccd 100644
--- a/include/proto.h
+++ b/include/proto.h
@@ -159,9 +159,9 @@ enum eth_hdr_fields {
 
 enum vlan_hdr_fields {
 	VLANHDR_INVALID,
-	VLANHDR_VID,
-	VLANHDR_CFI,
 	VLANHDR_PCP,
+	VLANHDR_CFI,
+	VLANHDR_VID,
 	VLANHDR_TYPE,
 };
 
diff --git a/src/proto.c b/src/proto.c
index 0fe0b88..f2cf297 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -730,9 +730,9 @@ const struct proto_desc proto_vlan = {
 
 	},
 	.templates	= {
-		[VLANHDR_VID]		= VLANHDR_BITFIELD("id", 0, 12),
-		[VLANHDR_CFI]		= VLANHDR_BITFIELD("cfi", 12, 1),
-		[VLANHDR_PCP]		= VLANHDR_BITFIELD("pcp", 13, 3),
+		[VLANHDR_PCP]		= VLANHDR_BITFIELD("pcp", 0, 3),
+		[VLANHDR_CFI]		= VLANHDR_BITFIELD("cfi", 3, 1),
+		[VLANHDR_VID]		= VLANHDR_BITFIELD("id", 4, 12),
 		[VLANHDR_TYPE]		= VLANHDR_TYPE("type", &ethertype_type, vlan_type),
 	},
 };
-- 
2.5.0

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

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> The VID is located after Priority and CFI.

With this patch matching on vlan id does not work for me anymore
on x86-64.

With trace-patch nft but without this patch:

table bridge filter {
chain input {
   type filter hook input priority -200; policy accept;
   vlan id 4094 counter packets 827 bytes 63839

With this patch, the counters remain at zero:

unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375
vlan id 4094 counter packets 0 bytes 0

(The 'unknown unknown' line is the 'old' vlan rule added by unpatched
 nft binary, the 'vlan id' is the one added with the patched one).

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

[Patrick McHardy]

On 27.11, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > The VID is located after Priority and CFI.
> 
> With this patch matching on vlan id does not work for me anymore
> on x86-64.
> 
> With trace-patch nft but without this patch:
> 
> table bridge filter {
> chain input {
>    type filter hook input priority -200; policy accept;
>    vlan id 4094 counter packets 827 bytes 63839
> 
> With this patch, the counters remain at zero:
> 
> unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375
> vlan id 4094 counter packets 0 bytes 0
> 
> (The 'unknown unknown' line is the 'old' vlan rule added by unpatched
>  nft binary, the 'vlan id' is the one added with the patched one).

Odd, since it decodes correctly. Could you send the output of
nft --debug=netlink with and without the patch?

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

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> On 27.11, Florian Westphal wrote:
> > Patrick McHardy <kaber@trash.net> wrote:
> > > The VID is located after Priority and CFI.
> > 
> > With this patch matching on vlan id does not work for me anymore
> > on x86-64.
> > 
> > With trace-patch nft but without this patch:
> > 
> > table bridge filter {
> > chain input {
> >    type filter hook input priority -200; policy accept;
> >    vlan id 4094 counter packets 827 bytes 63839
> > 
> > With this patch, the counters remain at zero:
> > 
> > unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375
> > vlan id 4094 counter packets 0 bytes 0
> > 
> > (The 'unknown unknown' line is the 'old' vlan rule added by unpatched
> >  nft binary, the 'vlan id' is the one added with the patched one).
> 
> Odd, since it decodes correctly. Could you send the output of
> nft --debug=netlink with and without the patch?

master:
nft --debug=netlink  add rule  bridge filter input vlan id 4094 counter
bridge filter input
 [ payload load 2b @ link header + 12 => reg 1 ]
 [ cmp eq reg 1 0x00000081 ]
 [ payload load 2b @ link header + 14 => reg 1 ]
 [ bitwise reg 1 = (reg=1 & 0x0000ff0f ) ^ 0x00000000 ]
 [ cmp eq reg 1 0x0000fe0f ]
 [ counter pkts 0 bytes 0 ]

patched:
# src/nft --debug=netlink  add rule  bridge filter input vlan id 4094 counter
bridge filter input
 [ payload load 2b @ link header + 12 => reg 1 ]
 [ cmp eq reg 1 0x00000081 ]
 [ payload load 2b @ link header + 14 => reg 1 ]
 [ bitwise reg 1 = (reg=1 & 0x0000f0ff ) ^ 0x00000000 ]
 [ cmp eq reg 1 0x0000e0ff ]
 [ counter pkts 0 bytes 0 ]

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

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > On 27.11, Florian Westphal wrote:
> > > Patrick McHardy <kaber@trash.net> wrote:
> > > > The VID is located after Priority and CFI.
> > > 
> > > With this patch matching on vlan id does not work for me anymore
> > > on x86-64.
> > > 
> > > With trace-patch nft but without this patch:
> > > 
> > > table bridge filter {
> > > chain input {
> > >    type filter hook input priority -200; policy accept;
> > >    vlan id 4094 counter packets 827 bytes 63839
> > > 
> > > With this patch, the counters remain at zero:
> > > 
> > > unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375
> > > vlan id 4094 counter packets 0 bytes 0
> > > 
> > > (The 'unknown unknown' line is the 'old' vlan rule added by unpatched
> > >  nft binary, the 'vlan id' is the one added with the patched one).
> > 
> > Odd, since it decodes correctly. Could you send the output of
> > nft --debug=netlink with and without the patch?
> 
> master:
> nft --debug=netlink  add rule  bridge filter input vlan id 4094 counter
> bridge filter input
>  [ payload load 2b @ link header + 12 => reg 1 ]
>  [ cmp eq reg 1 0x00000081 ]
>  [ payload load 2b @ link header + 14 => reg 1 ]
>  [ bitwise reg 1 = (reg=1 & 0x0000ff0f ) ^ 0x00000000 ]
>  [ cmp eq reg 1 0x0000fe0f ]
>  [ counter pkts 0 bytes 0 ]

I checked nft_payload again and I believe rebuild of the vlan header is
correct (a bug there would also explain this problem).

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

[Patrick McHardy]

On 27.11, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Patrick McHardy <kaber@trash.net> wrote:
> > > On 27.11, Florian Westphal wrote:
> > > > Patrick McHardy <kaber@trash.net> wrote:
> > > > > The VID is located after Priority and CFI.
> > > > 
> > > > With this patch matching on vlan id does not work for me anymore
> > > > on x86-64.
> > > > 
> > > > With trace-patch nft but without this patch:
> > > > 
> > > > table bridge filter {
> > > > chain input {
> > > >    type filter hook input priority -200; policy accept;
> > > >    vlan id 4094 counter packets 827 bytes 63839
> > > > 
> > > > With this patch, the counters remain at zero:
> > > > 
> > > > unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375
> > > > vlan id 4094 counter packets 0 bytes 0
> > > > 
> > > > (The 'unknown unknown' line is the 'old' vlan rule added by unpatched
> > > >  nft binary, the 'vlan id' is the one added with the patched one).
> > > 
> > > Odd, since it decodes correctly. Could you send the output of
> > > nft --debug=netlink with and without the patch?
> > 
> > master:
> > nft --debug=netlink  add rule  bridge filter input vlan id 4094 counter
> > bridge filter input
> >  [ payload load 2b @ link header + 12 => reg 1 ]
> >  [ cmp eq reg 1 0x00000081 ]
> >  [ payload load 2b @ link header + 14 => reg 1 ]
> >  [ bitwise reg 1 = (reg=1 & 0x0000ff0f ) ^ 0x00000000 ]
> >  [ cmp eq reg 1 0x0000fe0f ]
> >  [ counter pkts 0 bytes 0 ]
> 
> I checked nft_payload again and I believe rebuild of the vlan header is
> correct (a bug there would also explain this problem).

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.

I have to check that code in more detail.

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

[Florian Westphal]

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).

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

[Patrick McHardy]

On 27.11, 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).

Yeah, the actual variant chosen doesn't matter, I guess we just need to make
sure it is handled consistently. I chose the other way because you can simply
use the offsets defined in the standards. That's how I (think I) expressed
all the protocol definitions, so if we're using the other way around, we'll
possibly have some more bitfields which are not correct with the current code.

I'll do a audit of the code and the other definitions and see which way would
be easier to fix.

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

[Pablo Neira Ayuso]

[-- 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),

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 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.

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.

With Patricks fix to make VLAN header in IEEE notation:

src/nft --debug=netlink add rule bridge raw prerouting ether type vlan vlan type ip vlan id 4094 ip version 4 counter
bridge raw prerouting
  [ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00008100 ]
  [ payload load 2b @ link header + 16 => reg 1 ]
  [ cmp eq reg 1 0x00000800 ]
  [ payload load 2b @ link header + 14 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x00000f00 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000f00 ]
  [ payload load 1b @ network header + 0 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x000000f0 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000040 ]
  [ counter pkts 0 bytes 0 ]

master (counter increments):
# nft --debug=netlink add rule bridge raw prerouting ether type vlan vlan type ip vlan id 4094 ip version 4 counter
bridge raw prerouting
  [ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00000081 ]
  [ payload load 2b @ link header + 16 => reg 1 ]
  [ cmp eq reg 1 0x00000008 ]
  [ payload load 2b @ link header + 14 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x0000ff0f ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x0000fe0f ]
  [ payload load 1b @ network header + 0 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x000000f0 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000040 ]
  [ counter pkts 0 bytes 0 ]

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

[Pablo Neira Ayuso]

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

On Sun, Nov 29, 2015 at 01:09:29AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 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.
> 
> 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.

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

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),

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Nov 29, 2015 at 01:09:29AM +0100, Florian Westphal wrote:
> > 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.

Almost there.  Again, with Patricks patch to fix VLAN header:

# src/nft --debug=netlink add rule bridge raw prerouting ether type vlan vlan type ip vlan id 4094 ip version 4 counter 
bridge raw prerouting
  [ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00000081 ]
  [ payload load 2b @ link header + 16 => reg 1 ]
  [ cmp eq reg 1 0x00000008 ]
  [ payload load 2b @ link header + 14 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x00000f00 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000f00 ]
  [ payload load 1b @ network header + 0 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x000000f0 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000040 ]
  [ counter pkts 0 bytes 0 ]

for comparision, master.  All looks ok except the bitwise and cmp of vlan id.
# nft --debug=netlink add rule bridge raw prerouting ether type vlan vlan type ip vlan id 4094 ip version 4 counter
bridge raw prerouting 
  [ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00000081 ]
  [ payload load 2b @ link header + 16 => reg 1 ]
  [ cmp eq reg 1 0x00000008 ]
  [ payload load 2b @ link header + 14 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x0000ff0f ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x0000fe0f ]
  [ payload load 1b @ network header + 0 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x000000f0 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000040 ]
  [ counter pkts 0 bytes 0 ]

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

[Pablo Neira Ayuso]

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

On Sun, Nov 29, 2015 at 11:37:43PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, Nov 29, 2015 at 01:09:29AM +0100, Florian Westphal wrote:
> > > 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.
> 
> Almost there. Again, with Patricks patch to fix VLAN header:
> 
> # src/nft --debug=netlink add rule bridge raw prerouting ether type vlan vlan type ip vlan id 4094 ip version 4 counter 
> bridge raw prerouting

OK, new try, the idea behind is to calculate this shift through:

        x = offset % BITS_PER_BYTE
        y = len % BITS_PER_BYTE

to get both offset and length at byte level.

Then calculate the shift based on this:

        shift = BITS_PER_BYTE - (x + y)

Does this look good to you?

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

diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 9840cd4..96f4dda 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -113,7 +113,8 @@ static void netlink_gen_payload_mask(struct netlink_linearize_ctx *ctx,
 	mpz_t mask;
 
 	offset = expr->payload.offset % BITS_PER_BYTE;
-	shift = (expr->len % BITS_PER_BYTE) - offset;
+	len = expr->len % BITS_PER_BYTE;
+	shift = BITS_PER_BYTE - (offset + len);
 	masklen = expr->len + shift;
 
 	if (masklen > 128)
@@ -291,7 +292,7 @@ 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, offset;
+	unsigned int shift, offset, len;
 
 	if (right->ops->type != EXPR_VALUE ||
 	    left->ops->type != EXPR_PAYLOAD)
@@ -300,7 +301,8 @@ static void payload_shift_value(const struct expr *left, struct expr *right)
 	if (left->payload.offset % BITS_PER_BYTE ||
 	    (left->payload.offset + left->len) % BITS_PER_BYTE) {
 		offset = left->payload.offset % BITS_PER_BYTE;
-		shift = (left->len % BITS_PER_BYTE) - offset;
+		len = left->len % BITS_PER_BYTE;
+		shift = BITS_PER_BYTE - (offset + len);
 		mpz_lshift_ui(right->value, shift);
 	}
 }

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Nov 29, 2015 at 11:37:43PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Sun, Nov 29, 2015 at 01:09:29AM +0100, Florian Westphal wrote:
> > > > 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.
> > 
> > Almost there. Again, with Patricks patch to fix VLAN header:
> > 
> > # src/nft --debug=netlink add rule bridge raw prerouting ether type vlan vlan type ip vlan id 4094 ip version 4 counter 
> > bridge raw prerouting
> 
> OK, new try, the idea behind is to calculate this shift through:
> 
>         x = offset % BITS_PER_BYTE
>         y = len % BITS_PER_BYTE
> 
> to get both offset and length at byte level.
> 
> Then calculate the shift based on this:
> 
>         shift = BITS_PER_BYTE - (x + y)
> 
> Does this look good to you?

Sorry, that patch doesn't apply for me to master branch,
neither standalone nor on top of your previous patch.

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

[Pablo Neira Ayuso]

On Mon, Nov 30, 2015 at 02:53:22PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, Nov 29, 2015 at 11:37:43PM +0100, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Sun, Nov 29, 2015 at 01:09:29AM +0100, Florian Westphal wrote:
> > > > > 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.
> > > 
> > > Almost there. Again, with Patricks patch to fix VLAN header:
> > > 
> > > # src/nft --debug=netlink add rule bridge raw prerouting ether type vlan vlan type ip vlan id 4094 ip version 4 counter 
> > > bridge raw prerouting
> > 
> > OK, new try, the idea behind is to calculate this shift through:
> > 
> >         x = offset % BITS_PER_BYTE
> >         y = len % BITS_PER_BYTE
> > 
> > to get both offset and length at byte level.
> > 
> > Then calculate the shift based on this:
> > 
> >         shift = BITS_PER_BYTE - (x + y)
> > 
> > Does this look good to you?
> 
> Sorry, that patch doesn't apply for me to master branch,
> neither standalone nor on top of your previous patch.

I see, sorry. Anyway, let me revisit these arithmetics again, they
still don't correctly cover all cases as we need.