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:
signature.asc
Description: PGP signature
