Re: [nft PATCH v4 09/32] netlink_delinearize: add postprocessing for payload binops

[Jeremy Sowden <jeremy@azazel.net>]

[-- Attachment #1: Type: text/plain, Size: 3456 bytes --]

On 2022-05-23, at 19:19:08 +0200, Pablo Neira Ayuso wrote:
> On Mon, Apr 04, 2022 at 01:13:47PM +0100, Jeremy Sowden wrote:
> > If a user uses a payload expression as a statement argument:
> >
> >   nft add rule t c meta mark set ip dscp lshift 2 or 0x10
> >
> > we may need to undo munging during delinearization.
> >
> > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > ---
> >  src/netlink_delinearize.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> > index 733977bc526d..12624db4c3a5 100644
> > --- a/src/netlink_delinearize.c
> > +++ b/src/netlink_delinearize.c
> > @@ -2454,6 +2454,42 @@ static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
> >  	}
> >  }
> >
> > +static bool payload_binop_postprocess(struct rule_pp_ctx *ctx,
> > +				      struct expr **exprp)
> > +{
> > +	struct expr *expr = *exprp;
> > +
> > +	if (expr->op != OP_RSHIFT)
> > +		return false;
> > +
> > +	if (expr->left->etype == EXPR_UNARY) {
> > +		/*
> > +		 * If the payload value was originally in a different byte-order
> > +		 * from the payload expression, there will be a byte-order
> > +		 * conversion to remove.
> > +		 */
>
> The comment assumes this is a payload expression, the unary is
> stripped off here...
>
> > +		struct expr *left = expr_get(expr->left->arg);
> > +		expr_free(expr->left);
> > +		expr->left = left;
> > +	}
> > +
> > +	if (expr->left->etype != EXPR_BINOP || expr->left->op != OP_AND)
> > +		return false;
> > +
> > +	if (expr->left->left->etype != EXPR_PAYLOAD)
>
> ... but the check for payload is coming here.

Will fix.

> I assume this postprocessing is to undo the switch from network
> byteorder to host byteorder for the ip dscp of the example above?
>
> Could you describe an example expression tree to depict this
> delinearize scenario?

Currently, demunging is only done for payload statement expressions, in
`stmt_payload_postprocess`.  However, this patch-set will lead to the
appearance of munged payload expressions in other contexts, such as the
example given above in the commit message:

  nft add rule t c meta mark set ip dscp lshift 2 or 0x10

The expression tree for the value assigned to `meta mark` is:

                              OR
		             /  \
                       LSHIFT    0x10
                      /      \
                RSHIFT        2
               /      \
            AND        2
           /   \
   @nh(8,8)     0xfc

and the `@nh(8,8) & 0xfc >> 2` expression needs to be demunged to `ip dscp`.

> > +		return false;
> > +
> > +	expr_set_type(expr->right, &integer_type,
> > +		      BYTEORDER_HOST_ENDIAN);
> > +	expr_postprocess(ctx, &expr->right);
> > +
> > +	binop_postprocess(ctx, expr, &expr->left);
> > +	*exprp = expr_get(expr->left);
> > +	expr_free(expr);
> > +
> > +	return true;
> > +}
> > +
> >  static struct expr *string_wildcard_expr_alloc(struct location *loc,
> >  					       const struct expr *mask,
> >  					       const struct expr *expr)
> > @@ -2566,6 +2602,9 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
> >  		expr_set_type(expr, expr->arg->dtype, !expr->arg->byteorder);
> >  		break;
> >  	case EXPR_BINOP:
> > +		if (payload_binop_postprocess(ctx, exprp))
> > +			break;
> > +
> >  		expr_postprocess(ctx, &expr->left);
> >  		switch (expr->op) {
> >  		case OP_LSHIFT:
> > --
> > 2.35.1
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]