diff mbox

[nft] proto: fix VLAN header definition

Message ID 20151130123143.GA4233@salvia
State RFC
Delegated to: Pablo Neira
Headers show

Commit Message

Pablo Neira Ayuso Nov. 30, 2015, 12:31 p.m. UTC 
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?

Comments

Florian Westphal Nov. 30, 2015, 1:53 p.m. UTC | #1 
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Nov. 30, 2015, 1:57 p.m. UTC | #2 
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
 	}
 }