Re: ip6 dscp fails map lookup

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

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