ip6 dscp fails map lookup

ip6 dscp fails map lookup

[Brian Davidson]

Using 'ip6 dscp' in a map lookup does not give expected results. It
seems to always match the zero value (cs0). It appears in the first
rule that the byteorder is not being converted between the two bitwise
operations, which is what happens when using ip6 dscp directly in the
second rule.

# nft -f - <<EOF
add table ip6 t
add chain ip6 t c
add map ip6 t mapv6 { typeof ip6 dscp : meta mark; }
EOF

# nft -d netlink add rule ip6 t c meta mark set ip6 dscp map @mapv6
ip6 t c
  [ payload load 2b @ network header + 0 => reg 1 ]
  [ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ]
  [ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ]
  [ lookup reg 1 set mapv6 dreg 1 ]
  [ meta set mark with reg 1 ]

# nft -d netlink add rule ip6 t c meta mark set ip6 dscp
ip6 t c
  [ payload load 2b @ network header + 0 => reg 1 ]
  [ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ]
  [ byteorder reg 1 = ntoh(reg 1, 2, 2) ]
  [ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ]
  [ meta set mark with reg 1 ]

Re: ip6 dscp fails map lookup

[Florian Westphal]

Brian Davidson <davidson.brian@gmail.com> wrote:
> Using 'ip6 dscp' in a map lookup does not give expected results. It
> seems to always match the zero value (cs0). It appears in the first
> rule that the byteorder is not being converted between the two bitwise
> operations, which is what happens when using ip6 dscp directly in the
> second rule.
> 
> # nft -f - <<EOF
> add table ip6 t
> add chain ip6 t c
> add map ip6 t mapv6 { typeof ip6 dscp : meta mark; }
> EOF
> 
> # nft -d netlink add rule ip6 t c meta mark set ip6 dscp map @mapv6
> ip6 t c
>   [ payload load 2b @ network header + 0 => reg 1 ]
>   [ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ]
>   [ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ]
>   [ lookup reg 1 set mapv6 dreg 1 ]
>   [ meta set mark with reg 1 ]
> 
> # nft -d netlink add rule ip6 t c meta mark set ip6 dscp
> ip6 t c
>   [ payload load 2b @ network header + 0 => reg 1 ]
>   [ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ]
>   [ byteorder reg 1 = ntoh(reg 1, 2, 2) ]
>   [ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ]
>   [ meta set mark with reg 1 ]

Uhm.  Pablo, any idea why the byte-swap-or-not logic depends
on something *other* than if the mask length is > 8 bit or not?

diff --git a/src/evaluate.c b/src/evaluate.c
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -545,7 +545,7 @@ static void expr_evaluate_bits(struct eval_ctx *ctx, struct expr **exprp)
 	and->len	= masklen;
 
 	if (shift) {
-		if (ctx->stmt_len > 0 && div_round_up(masklen, BITS_PER_BYTE) > 1) {
+		if (masklen > BITS_PER_BYTE) {
 			int op = byteorder_conversion_op(expr, BYTEORDER_HOST_ENDIAN);
 			and = unary_expr_alloc(&expr->location, op, and);
 			and->len = masklen;

Re: ip6 dscp fails map lookup

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Brian Davidson <davidson.brian@gmail.com> wrote:
> > Using 'ip6 dscp' in a map lookup does not give expected results. It
> > seems to always match the zero value (cs0). It appears in the first
> > rule that the byteorder is not being converted between the two bitwise
> > operations, which is what happens when using ip6 dscp directly in the
> > second rule.
> > 
> > # nft -f - <<EOF
> > add table ip6 t
> > add chain ip6 t c
> > add map ip6 t mapv6 { typeof ip6 dscp : meta mark; }
> > EOF
> > 
> > # nft -d netlink add rule ip6 t c meta mark set ip6 dscp map @mapv6
> > ip6 t c
> >   [ payload load 2b @ network header + 0 => reg 1 ]
> >   [ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ]
> >   [ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ]
> >   [ lookup reg 1 set mapv6 dreg 1 ]
> >   [ meta set mark with reg 1 ]
> > 
> > # nft -d netlink add rule ip6 t c meta mark set ip6 dscp
> > ip6 t c
> >   [ payload load 2b @ network header + 0 => reg 1 ]
> >   [ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ]
> >   [ byteorder reg 1 = ntoh(reg 1, 2, 2) ]
> >   [ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ]
> >   [ meta set mark with reg 1 ]
> 
> Uhm.  Pablo, any idea why the byte-swap-or-not logic depends
> on something *other* than if the mask length is > 8 bit or not?
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -545,7 +545,7 @@ static void expr_evaluate_bits(struct eval_ctx *ctx, struct expr **exprp)
>  	and->len	= masklen;
>  
>  	if (shift) {
> -		if (ctx->stmt_len > 0 && div_round_up(masklen, BITS_PER_BYTE) > 1) {
> +		if (masklen > BITS_PER_BYTE) {

I think this is right but binop xfer
won't remove the inserted byteorder conversion
in case the shift is to be removed by adjusting
a constant right hand side value.

I will have a look.

Re: ip6 dscp fails map lookup

[Pablo Neira Ayuso]

On Fri, Nov 03, 2023 at 10:59:04PM +0100, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Brian Davidson <davidson.brian@gmail.com> wrote:
> > > Using 'ip6 dscp' in a map lookup does not give expected results. It
> > > seems to always match the zero value (cs0). It appears in the first
> > > rule that the byteorder is not being converted between the two bitwise
> > > operations, which is what happens when using ip6 dscp directly in the
> > > second rule.
> > > 
> > > # nft -f - <<EOF
> > > add table ip6 t
> > > add chain ip6 t c
> > > add map ip6 t mapv6 { typeof ip6 dscp : meta mark; }
> > > EOF
> > > 
> > > # nft -d netlink add rule ip6 t c meta mark set ip6 dscp map @mapv6
> > > ip6 t c
> > >   [ payload load 2b @ network header + 0 => reg 1 ]
> > >   [ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ]
> > >   [ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ]
> > >   [ lookup reg 1 set mapv6 dreg 1 ]
> > >   [ meta set mark with reg 1 ]
> > > 
> > > # nft -d netlink add rule ip6 t c meta mark set ip6 dscp
> > > ip6 t c
> > >   [ payload load 2b @ network header + 0 => reg 1 ]
> > >   [ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ]
> > >   [ byteorder reg 1 = ntoh(reg 1, 2, 2) ]
> > >   [ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ]
> > >   [ meta set mark with reg 1 ]
> > 
> > Uhm.  Pablo, any idea why the byte-swap-or-not logic depends
> > on something *other* than if the mask length is > 8 bit or not?
> > 
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -545,7 +545,7 @@ static void expr_evaluate_bits(struct eval_ctx *ctx, struct expr **exprp)
> >  	and->len	= masklen;
> >  
> >  	if (shift) {
> > -		if (ctx->stmt_len > 0 && div_round_up(masklen, BITS_PER_BYTE) > 1) {
> > +		if (masklen > BITS_PER_BYTE) {
> 
> I think this is right but binop xfer
> won't remove the inserted byteorder conversion
> in case the shift is to be removed by adjusting
> a constant right hand side value.

ctx->stmt_len > 0 is also set from stmt_evaluate_{ct,meta}() so this
transformation is restricted to statements.

I was conservative edecd58755a8 with ("evaluate: support shifts larger than
the width of the left operand") to restrict this transformation to
statements only.

Do you think think this applies to other cases too?

BTW, whatever the final fix is, Fixes: tag for this should be:

Fixes: 668c18f67203 ("evaluate: place byteorder conversion before rshift in payload statement")

Re: ip6 dscp fails map lookup

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >  	if (shift) {
> > > -		if (ctx->stmt_len > 0 && div_round_up(masklen, BITS_PER_BYTE) > 1) {
> > > +		if (masklen > BITS_PER_BYTE) {
> > 
> > I think this is right but binop xfer
> > won't remove the inserted byteorder conversion
> > in case the shift is to be removed by adjusting
> > a constant right hand side value.
> 
> ctx->stmt_len > 0 is also set from stmt_evaluate_{ct,meta}() so this
> transformation is restricted to statements.
> 
> I was conservative edecd58755a8 with ("evaluate: support shifts larger than
> the width of the left operand") to restrict this transformation to
> statements only.
> 
> Do you think think this applies to other cases too?
> 
> BTW, whatever the final fix is, Fixes: tag for this should be:
> 
> Fixes: 668c18f67203 ("evaluate: place byteorder conversion before rshift in payload statement")

I don't think this is a regression, I do not think it has worked before.

table ip6 t {
	map mapv6 {
		typeof ip6 dscp : meta mark
		elements = { cs1 : 0x0000002a }
	}

	chain output {
		type filter hook output priority filter; policy accept;
		meta mark set ip6 dscp map @mapv6
		meta mark 0x0000002a counter
		ip6 dscp @mapv6 counter
		ip6 dscp cs1 counter
	}
}

A "ping6 -q -Q 0x20 ::1 -c1" should cause all counters
in above ruleset to increment, but that is not the case.

Even before above commit.

Re: ip6 dscp fails map lookup

[Pablo Neira Ayuso]

On Sun, Nov 05, 2023 at 06:41:39PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >  	if (shift) {
> > > > -		if (ctx->stmt_len > 0 && div_round_up(masklen, BITS_PER_BYTE) > 1) {
> > > > +		if (masklen > BITS_PER_BYTE) {
> > > 
> > > I think this is right but binop xfer
> > > won't remove the inserted byteorder conversion
> > > in case the shift is to be removed by adjusting
> > > a constant right hand side value.
> > 
> > ctx->stmt_len > 0 is also set from stmt_evaluate_{ct,meta}() so this
> > transformation is restricted to statements.
> > 
> > I was conservative edecd58755a8 with ("evaluate: support shifts larger than
> > the width of the left operand") to restrict this transformation to
> > statements only.
> > 
> > Do you think think this applies to other cases too?
> > 
> > BTW, whatever the final fix is, Fixes: tag for this should be:
> > 
> > Fixes: 668c18f67203 ("evaluate: place byteorder conversion before rshift in payload statement")
> 
> I don't think this is a regression, I do not think it has worked before.
>
> table ip6 t {
> 	map mapv6 {
> 		typeof ip6 dscp : meta mark
> 		elements = { cs1 : 0x0000002a }
> 	}
> 
> 	chain output {
> 		type filter hook output priority filter; policy accept;
> 		meta mark set ip6 dscp map @mapv6
> 		meta mark 0x0000002a counter
> 		ip6 dscp @mapv6 counter
> 		ip6 dscp cs1 counter
> 	}
> }
> 
> A "ping6 -q -Q 0x20 ::1 -c1" should cause all counters
> in above ruleset to increment, but that is not the case.
> 
> Even before above commit.

After my patch:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231105174148.160390-1-pablo@netfilter.org/

        chain output {
                type filter hook output priority filter; policy accept;
                meta mark set ip6 dscp map @mapv6
                meta mark 0x0000002a counter packets 2 bytes 208
                ip6 dscp @mapv6 counter packets 0 bytes 0
                ip6 dscp cs1 counter packets 2 bytes 208
        }

So 'ip6 dscp @mapv6' is still broken.

With your patch, simple 'ip6 dscp cs1' breaks:

        @nh,0,16 & 0xfc0 & 0xfc0 == 0x2 counter packets 0 bytes 0

Re: ip6 dscp fails map lookup

[Pablo Neira Ayuso]

On Sun, Nov 05, 2023 at 06:41:39PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >  	if (shift) {
> > > > -		if (ctx->stmt_len > 0 && div_round_up(masklen, BITS_PER_BYTE) > 1) {
> > > > +		if (masklen > BITS_PER_BYTE) {
> > > 
> > > I think this is right but binop xfer
> > > won't remove the inserted byteorder conversion
> > > in case the shift is to be removed by adjusting
> > > a constant right hand side value.
> > 
> > ctx->stmt_len > 0 is also set from stmt_evaluate_{ct,meta}() so this
> > transformation is restricted to statements.
> > 
> > I was conservative edecd58755a8 with ("evaluate: support shifts larger than
> > the width of the left operand") to restrict this transformation to
> > statements only.
> > 
> > Do you think think this applies to other cases too?
> > 
> > BTW, whatever the final fix is, Fixes: tag for this should be:
> > 
> > Fixes: 668c18f67203 ("evaluate: place byteorder conversion before rshift in payload statement")
> 
> I don't think this is a regression, I do not think it has worked before.
> 
> table ip6 t {
> 	map mapv6 {
> 		typeof ip6 dscp : meta mark
> 		elements = { cs1 : 0x0000002a }
> 	}
> 
> 	chain output {
> 		type filter hook output priority filter; policy accept;
> 		meta mark set ip6 dscp map @mapv6
> 		meta mark 0x0000002a counter
> 		ip6 dscp @mapv6 counter
> 		ip6 dscp cs1 counter
> 	}
> }
> 
> A "ping6 -q -Q 0x20 ::1 -c1" should cause all counters
> in above ruleset to increment, but that is not the case.

Thanks for your simple test, it is passing with these two patches:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231105174148.160390-1-pablo@netfilter.org/
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231105212846.311755-1-pablo@netfilter.org/

I have extended it to also cover for dynamic sets:

table ip6 t {
        map mapv6 {
                typeof ip6 dscp : meta mark
                elements = { cs1 : 0x0000002a }
        }

        map mapv62 {
                typeof ip6 dscp : meta mark
                size 65535
                flags dynamic
        }

        chain output {
                type filter hook output priority filter; policy accept;
                meta mark set ip6 dscp map @mapv6
                meta mark 0x0000002a counter
                ip6 dscp @mapv6 counter
                ip6 dscp cs1 counter
                update @mapv62 { ip6 dscp : meta mark }
        }
}

I just quickly checked for concatenations with ip6 dscp, it seems the
byteorder conversion is also missing there, so v2 will follow up.