Re: [PATCH nft] evaluate: don't eval unary arguments.

[Jeremy Sowden <jeremy@azazel.net>]

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

On 2020-02-23, at 23:23:21 +0100, Pablo Neira Ayuso wrote:
> On Sun, Feb 23, 2020 at 10:14:11PM +0000, Jeremy Sowden wrote:
> > On 2020-01-28, at 19:49:18 +0100, Pablo Neira Ayuso wrote:
> > > On Mon, Jan 27, 2020 at 11:13:43AM +0000, Jeremy Sowden wrote:
> > > > On 2020-01-27, at 10:33:04 +0100, Pablo Neira Ayuso wrote:
> > > > > On Sun, Jan 19, 2020 at 06:12:03PM +0000, Jeremy Sowden wrote:
> > > > > > When a unary expression is inserted to implement a
> > > > > > byte-order conversion, the expression being converted has
> > > > > > already been evaluated and so expr_evaluate_unary doesn't
> > > > > > need to do so.  For most types of expression, the double
> > > > > > evaluation doesn't matter since evaluation is idempotent.
> > > > > > However, in the case of payload expressions which are munged
> > > > > > during evaluation, it can cause unexpected errors:
> > > > > >
> > > > > >   # nft add table ip t
> > > > > >   # nft add chain ip t c '{ type filter hook input priority filter; }'
> > > > > >   # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
> > > > > >   Error: Value 252 exceeds valid range 0-63
> > > > > >   add rule ip t c ip dscp set ip dscp | 0x10
> > > > > >                               ^^^^^^^
> > > > >
> > > > > I'm still hitting this after applying this patch.
> > > > >
> > > > > nft add rule ip t c ip dscp set ip dscp or 0x10
> > > > > Error: Value 252 exceeds valid range 0-63
> > > > > add rule ip t c ip dscp set ip dscp or 0x10
> > >
> > > [...]
> > >
> > > I think stmt_evaluate_payload() is incomplete, this function was
> > > not made to deal with non-constant expression as values.
> > >
> > > [...]
> > >
> > > Probably you can start making a patchset make this work:
> > >
> > >         add rule ip t c tcp dport set tcp dport lshift 1
> > >
> > > which triggers:
> > >
> > > BUG: invalid binary operation 4
> > > nft: netlink_linearize.c:592: netlink_gen_binop: Assertion `0'
> > > failed.
> > >
> > > since it's missing the bytecode to generate the left-shift. Not
> > > very useful for users, but we can get something already merged
> > > upstream and you'll be half-way done. Merge also a few tests.
> >
> > This assertion failure had already been fixed by the bitwise shift
> > patches you had recently applied.  However, the rule itself doesn't
> > yet quite work because `tcp dport lshift 1` has the wrong
> > endianness.  Thus given an original `tcp dport` of 40, we end up
> > with 20480, instead of 80.
>
> I think the generated bytecode should be like this:
>
>         r1 <- payload to fetch value
>         swap byteorder in r1
>         shift value in r1
>         cmp r1 and immediate value (in host byteorder)

Currently, nft generates this:

  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 2b @ transport header + 0 => reg 1 ]
  [ byteorder reg 1 = ntoh(reg 1, 2, 2) ]
  [ bitwise reg 1 = ( reg 1 << 0x00000001 ) ]
  [ payload write reg 1 => 2b @ transport header + 2 csum_type 1 csum_off 16 csum_flags 0x0 ]

I have a patch to insert the missing hton:

  --- a/src/evaluate.c
  +++ b/src/evaluate.c
  @@ -2218,6 +2218,11 @@ static int stmt_evaluate_payload(struct eval_ctx *ctx, struct stmt *stmt)
                                payload->byteorder, &stmt->payload.val) < 0)
                  return -1;

  +       if (!expr_is_constant(stmt->payload.val) &&
  +           byteorder_conversion(ctx, &stmt->payload.val,
  +                                payload->byteorder) < 0)
  +               return -1;
  +
          need_csum = stmt_evaluate_payload_need_csum(payload);

          if (!payload_needs_adjustment(payload)) {

giving:

  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 2b @ transport header + 0 => reg 1 ]
  [ byteorder reg 1 = ntoh(reg 1, 2, 2) ]
  [ bitwise reg 1 = ( reg 1 << 0x00000001 ) ]
  [ byteorder reg 1 = hton(reg 1, 2, 2) ]
  [ payload write reg 1 => 2b @ transport header + 2 csum_type 1 csum_off 16 csum_flags 0x0 ]

> > > Then, once the more fundamental rshift/lshift bits are merged,
> > > look at this 'harder' path. Just a proposal.
> > >
> > > For reference, the expression tree that stmt_evaluate_payload() to
> > > make the checksum adjustment looks like this:
> > >
> > >            xor
> > >           /   \
> > >         and   value
> > >         / \
> > > payload_   mask
> > >  bytes
> > >
> > > payload_bytes extends the payload expression to get up to 16-bits.
> > > The left hand side is there to fetch bits that need to be left
> > > untouched. The right hand side represent the bits that need to be
> > > set.
> > >
> > > In the new non-constant scenario, the 'value' tree is actually a
> > > binary operation:
> > >
> > >          shift
> > >         /    \
> > >    payload   imm
> > >
> > > The unary should not really be there, it's likely related to some
> > > incorrect byteorder issue that kicks in with non-constant
> > > expression.
> > >
> > > So more work on stmt_evaluate_payload() is required.
> >
> > After giving this some thought, it occurred to me that this could be
> > fixed by extending bitwise boolean operations to support a variable
> > righthand operand (IIRC, before Christmas Florian suggested
> > something along these lines to me in another, related context), so
> > I've gone down that route.  Patches to follow shortly.
>
> Would this require a new kernel extensions? What's the idea behind
> this?

In addition to what Florian has mentioned elsewhere (and the original
reason I started looking at this), is Kevin Darbyshire-Bryant's desire
to be able to do something like:

  nft add rule t c ct mark set ip dscp lshift 16 or 0x10

That specific example wouldn't require a variable RHS (but would require
other changes), but Florian suggested generalizing the solution, and
setting payload fields using non-constant expressions would.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]