From: Jeremy Sowden <jeremy@azazel.net>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Netfilter Devel <netfilter-devel@vger.kernel.org>
Subject: Re: [PATCH nft] evaluate: don't eval unary arguments.
Date: Mon, 24 Feb 2020 12:36:46 +0000 [thread overview]
Message-ID: <20200224123646.GA505545@azazel.net> (raw)
In-Reply-To: <20200223222321.kjfsxjl6ftbcrink@salvia>
[-- 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 --]
prev parent reply other threads:[~2020-02-24 12:36 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-19 18:12 [PATCH nft] evaluate: don't eval unary arguments Jeremy Sowden
2020-01-27 9:33 ` Pablo Neira Ayuso
2020-01-27 11:13 ` Jeremy Sowden
2020-01-28 18:49 ` Pablo Neira Ayuso
2020-02-04 11:02 ` Jeremy Sowden
2020-02-23 22:14 ` Jeremy Sowden
2020-02-23 22:23 ` Pablo Neira Ayuso
2020-02-23 22:34 ` Florian Westphal
2020-02-23 22:38 ` Pablo Neira Ayuso
2020-02-23 23:12 ` Florian Westphal
2020-02-24 12:36 ` Jeremy Sowden [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200224123646.GA505545@azazel.net \
--to=jeremy@azazel.net \
--cc=netfilter-devel@vger.kernel.org \
--cc=pablo@netfilter.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).