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 > > > ^^^^^^ > > > Probably problem is somewhere else? I'm not sure why we can assume > > > here that the argument of the unary expression should not be > > > evaluated. > > > > I'll take another look. I think what happened here is that I came across this problem while working on using payload expressions to set CT and packet marks, and in that specific case stopping the double-evaluation in expr_evaluate_unary fixed it. However, when I looked for another example that was allowed by the current grammar to put into the commit message, the one I found was caused by stmt_evaluate_payload instead, but on the assumption that the cause was the same, I didn't actually verify it. Whoops. > 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. > > 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. Thanks for the analysis, Pablo. I'll get started. J.
Attachment:
signature.asc
Description: PGP signature