On 2022-05-23, at 19:42:40 +0200, Pablo Neira Ayuso wrote: > I just tested patches 9 and 14 alone, and meta mark set ip dscp ... > now works fine. > > So most of the work from 7 to 14 is to allow to use shifts. > > So 8, 10, 11, 12 and 13 enable the use of shifts is meta and ct > statements? Yes. > The new _NBIT field is there to store the original length for the > payload field (6 bits, for the ip dscp case)? It's for this ip6 dscp case: ct mark set ip6 dscp << 2 | 16 This is linearized as: [ payload load 2b @ network header + 0 => reg 1 ] [ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ] [ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ] [ byteorder reg 1 = ntoh(reg 1, 2, 1) ] [ bitwise reg 1 = ( reg 1 << 0x00000002 ) ] [ bitwise reg 1 = ( reg 1 & 0x00000fef ) ^ 0x00000010 ] [ ct set mark with reg 1 ] The problem is the last bitwise expression: [ bitwise reg 1 = ( reg 1 & 0x00000fef ) ^ 0x00000010 ] `ip6 dscp` spans two octets: @nh(0,16) & 0xfc0 >> 6 The original length is 12 bits. The LSHIFT expression changes the byte-order to host-endian. When the OR expression is evaluated, it is converted from: ${lhs} | 16 to: ${lhs} & 0xfef ^ 0x10 and when it is linearized, the bit-length is rounded up to 16 bits and the byte-order is converted to big-endian. During delinearization we try to turn the mask-and-xor back into its original form before the original endianness and length are known, so starting from: ${lhs} & 0xef0f ^ 0x1000 we fail to strip the mask and end up with: ct mark set ip6 dscp << 2 & 4095 | 16 Having gone back and reviewed this bug in the writing of this e-mail I've realized that the introduction of native kernel bitwise op's in patches 27-28 could obviate it. In the current iteration of the patch-set, the new bitwise op's are only used to support previously unsupported bitwise expressions with variable right hand operands; currently supported operations with constant right hand operands are still converted to mask-and-xor operations. If the linearization code were changed to use the native op's for expressions with constant RHS's too, the problematic conversion from mask-and-xor would go away. When I tried this out, I had to make a couple of other changes to get it working. The big one was to register allocation. Although netfilter registers are 32-bits wide these days, they are currently allocated in blocks of four for backwards-compatibility with older kernels. Some of the new test-cases added in this series failed because of a lack of available registers, so I changed the allocation as follows: --- a/src/netlink_linearize.c +++ b/src/netlink_linearize.c @@ -97,7 +97,7 @@ static void __release_register(struct netlink_linearize_ctx *ctx, static enum nft_registers get_register(struct netlink_linearize_ctx *ctx, const struct expr *expr) { - if (expr && expr->etype == EXPR_CONCAT) + if (expr && expr->len) return __get_register(ctx, expr->len); else return __get_register(ctx, NFT_REG_SIZE * BITS_PER_BYTE); @@ -106,7 +106,7 @@ static enum nft_registers get_register(struct netlink_linearize_ctx *ctx, static void release_register(struct netlink_linearize_ctx *ctx, const struct expr *expr) { - if (expr && expr->etype == EXPR_CONCAT) + if (expr && expr->len) __release_register(ctx, expr->len); else __release_register(ctx, NFT_REG_SIZE * BITS_PER_BYTE); It's been seven years since the switch from 128- to 32-bit registers. Would something like this change be acceptable? J. > On Mon, Apr 04, 2022 at 01:13:51PM +0100, Jeremy Sowden wrote: > > If we want to left-shift a value of narrower type and assign the result > > to a variable of a wider type, we are constrained to only shifting up to > > the width of the narrower type. Thus: > > > > add rule t c meta mark set ip dscp << 2 > > > > works, but: > > > > add rule t c meta mark set ip dscp << 8 > > > > does not, even though the lvalue is large enough to accommodate the > > result. > > > > Evaluation of the left-hand operand of a shift overwrites the `len` > > field of the evaluation context when `expr_evaluate_primary` is called. > > Instead, preserve the `len` value of the evaluation context for shifts, > > and support shifts up to that size, even if they are larger than the > > length of the left operand. > > > > Update netlink_delinearize.c to handle the case where the length of a > > shift expression does not match that of its left-hand operand. > > > > Signed-off-by: Jeremy Sowden <jeremy@xxxxxxxxxx> > > --- > > src/evaluate.c | 23 ++++++++++++++--------- > > src/netlink_delinearize.c | 4 ++-- > > 2 files changed, 16 insertions(+), 11 deletions(-) > > > > diff --git a/src/evaluate.c b/src/evaluate.c > > index be493f85010c..ee4da5a2b889 100644 > > --- a/src/evaluate.c > > +++ b/src/evaluate.c > > @@ -1116,14 +1116,18 @@ static int constant_binop_simplify(struct eval_ctx *ctx, struct expr **expr) > > static int expr_evaluate_shift(struct eval_ctx *ctx, struct expr **expr) > > { > > struct expr *op = *expr, *left = op->left, *right = op->right; > > + unsigned int shift = mpz_get_uint32(right->value); > > + unsigned int op_len = left->len; > > > > - if (mpz_get_uint32(right->value) >= left->len) > > - return expr_binary_error(ctx->msgs, right, left, > > - "%s shift of %u bits is undefined " > > - "for type of %u bits width", > > - op->op == OP_LSHIFT ? "Left" : "Right", > > - mpz_get_uint32(right->value), > > - left->len); > > + if (shift >= op_len) { > > + if (shift >= ctx->ectx.len) > > + return expr_binary_error(ctx->msgs, right, left, > > + "%s shift of %u bits is undefined for type of %u bits width", > > + op->op == OP_LSHIFT ? "Left" : "Right", > > + shift, > > + op_len); > > + op_len = ctx->ectx.len; > > + } > > > > /* Both sides need to be in host byte order */ > > if (byteorder_conversion(ctx, &op->left, BYTEORDER_HOST_ENDIAN) < 0) > > @@ -1134,7 +1138,7 @@ static int expr_evaluate_shift(struct eval_ctx *ctx, struct expr **expr) > > > > op->dtype = &integer_type; > > op->byteorder = BYTEORDER_HOST_ENDIAN; > > - op->len = left->len; > > + op->len = op_len; > > > > if (expr_is_constant(left)) > > return constant_binop_simplify(ctx, expr); > > @@ -1167,6 +1171,7 @@ static int expr_evaluate_binop(struct eval_ctx *ctx, struct expr **expr) > > { > > struct expr *op = *expr, *left, *right; > > const char *sym = expr_op_symbols[op->op]; > > + unsigned int ectx_len = ctx->ectx.len; > > > > if (expr_evaluate(ctx, &op->left) < 0) > > return -1; > > @@ -1174,7 +1179,7 @@ static int expr_evaluate_binop(struct eval_ctx *ctx, struct expr **expr) > > > > if (op->op == OP_LSHIFT || op->op == OP_RSHIFT) > > __expr_set_context(&ctx->ectx, &integer_type, > > - left->byteorder, ctx->ectx.len, 0); > > + left->byteorder, ectx_len, 0); > > if (expr_evaluate(ctx, &op->right) < 0) > > return -1; > > right = op->right; > > diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c > > index cf5359bf269e..9f6fdee3e92d 100644 > > --- a/src/netlink_delinearize.c > > +++ b/src/netlink_delinearize.c > > @@ -486,7 +486,7 @@ static struct expr *netlink_parse_bitwise_bool(struct netlink_parse_ctx *ctx, > > mpz_ior(m, m, o); > > } > > > > - if (left->len > 0 && mpz_scan0(m, 0) == left->len) { > > + if (left->len > 0 && mpz_scan0(m, 0) >= left->len) { > > /* mask encompasses the entire value */ > > expr_free(mask); > > } else { > > @@ -536,7 +536,7 @@ static struct expr *netlink_parse_bitwise_shift(struct netlink_parse_ctx *ctx, > > right->byteorder = BYTEORDER_HOST_ENDIAN; > > > > expr = binop_expr_alloc(loc, op, left, right); > > - expr->len = left->len; > > + expr->len = nftnl_expr_get_u32(nle, NFTNL_EXPR_BITWISE_LEN) * BITS_PER_BYTE; > > > > return expr; > > } > > -- > > 2.35.1 > > >
Attachment:
signature.asc
Description: PGP signature