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.
> >
> > Look: tcp dport set tcp sport
> >
> > works because it follows the 'easy path', ie. no adjustment to make
> > the checksum calculation happy (see payload_needs_adjustment() in
> > stmt_evaluate_payload().
> >
> > However:
> >
> > ip dscp set ip dscp
> >
> > bails out with:
> >
> > nft add rule ip t c ip dscp set ip dscp
> > Error: Value 252 exceeds valid range 0-63
> > add rule ip t c ip dscp set ip dscp
> > ^^^^^^^
> >
> > because this follows the more complicated path. Looking at this code,
> > this path assumes a constant value, ie. ip dscp set 10. A more complex
> > thing such a non-constant expression (as in the example above) will
> > need a bit of work.
> >
> > 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)
> > 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?
Thanks.
