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
