[PATCH nft 0/3] netlink_delinearize cleanups

[PATCH nft 0/3] netlink_delinearize cleanups

[Florian Westphal]

No indended behavioural changes here.
binop postprocessing has been extended a lot and in a few places
things only work because expr->map and expr->left alias to the same
location.

 src/netlink_delinearize.c |    2 -
 src/netlink_delinearize.c |   74 +++++++++++++++++++++++++-------------------
 2 files changed, 44 insertions(+), 32 deletions(-)

[PATCH nft 1/3] netlink_delinearize: use correct member type

[Florian Westphal]

expr is a map, so this should use expr->map, not expr->left.
These fields are aliased, so this would break if that is ever changed.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/netlink_delinearize.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 0c2b439eac6f..66120d659dc3 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2300,7 +2300,7 @@ static void binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr)
 
 static void map_binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr)
 {
-	struct expr *binop = expr->left;
+	struct expr *binop = expr->map;
 
 	if (binop->op != OP_AND)
 		return;
-- 
2.32.0

[PATCH nft 2/3] netlink_delinearize: rename misleading variable

[Florian Westphal]

relational_binop_postprocess() is called for EXPR_RELATIONAL,
so "expr->right" is safe to use.

But the RHS can be something other than a value.
This has been extended to handle other types, so rename to 'right'.

No code changes intended.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/netlink_delinearize.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 66120d659dc3..1446cfdad2e1 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2313,20 +2313,20 @@ static void map_binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr)
 static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
 					 struct expr **exprp)
 {
-	struct expr *expr = *exprp, *binop = expr->left, *value = expr->right;
+	struct expr *expr = *exprp, *binop = expr->left, *right = expr->right;
 
 	if (binop->op == OP_AND && (expr->op == OP_NEQ || expr->op == OP_EQ) &&
-	    value->dtype->basetype &&
-	    value->dtype->basetype->type == TYPE_BITMASK) {
-		switch (value->etype) {
+	    right->dtype->basetype &&
+	    right->dtype->basetype->type == TYPE_BITMASK) {
+		switch (right->etype) {
 		case EXPR_VALUE:
-			if (!mpz_cmp_ui(value->value, 0)) {
+			if (!mpz_cmp_ui(right->value, 0)) {
 				/* Flag comparison: data & flags != 0
 				 *
 				 * Split the flags into a list of flag values and convert the
 				 * op to OP_EQ.
 				 */
-				expr_free(value);
+				expr_free(right);
 
 				expr->left  = expr_get(binop->left);
 				expr->right = binop_tree_to_list(NULL, binop->right);
@@ -2342,8 +2342,8 @@ static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
 				}
 				expr_free(binop);
 			} else if (binop->right->etype == EXPR_VALUE &&
-				   value->etype == EXPR_VALUE &&
-				   !mpz_cmp(value->value, binop->right->value)) {
+				   right->etype == EXPR_VALUE &&
+				   !mpz_cmp(right->value, binop->right->value)) {
 				/* Skip flag / flag representation for:
 				 * data & flag == flag
 				 * data & flag != flag
@@ -2353,7 +2353,7 @@ static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
 				*exprp = flagcmp_expr_alloc(&expr->location, expr->op,
 							    expr_get(binop->left),
 							    binop_tree_to_list(NULL, binop->right),
-							    expr_get(value));
+							    expr_get(right));
 				expr_free(expr);
 			}
 			break;
@@ -2361,7 +2361,7 @@ static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
 			*exprp = flagcmp_expr_alloc(&expr->location, expr->op,
 						    expr_get(binop->left),
 						    binop_tree_to_list(NULL, binop->right),
-						    binop_tree_to_list(NULL, value));
+						    binop_tree_to_list(NULL, right));
 			expr_free(expr);
 			break;
 		default:
@@ -2372,9 +2372,9 @@ static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
 		   expr_mask_is_prefix(binop->right)) {
 		expr->left = expr_get(binop->left);
 		expr->right = prefix_expr_alloc(&expr->location,
-						expr_get(value),
+						expr_get(right),
 						expr_mask_to_prefix(binop->right));
-		expr_free(value);
+		expr_free(right);
 		expr_free(binop);
 	} else if (binop->op == OP_AND &&
 		   binop->right->etype == EXPR_VALUE) {
-- 
2.32.0

[PATCH nft 3/3] netlink_delinearize: binop: make accesses to expr->left/right conditional

[Florian Westphal]

This function can be called for different expression types, including
some (EXPR_MAP) where expr->left/right alias to different member
variables.

This makes accesses to those members conditional by checking the
expression type ahead of the access.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/netlink_delinearize.c | 50 ++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 1446cfdad2e1..87db2e510593 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2223,8 +2223,8 @@ static void binop_adjust_one(const struct expr *binop, struct expr *value,
 	}
 }
 
-static void __binop_adjust(const struct expr *binop, struct expr *right,
-			   unsigned int shift)
+static void binop_adjust(const struct expr *binop, struct expr *right,
+			 unsigned int shift)
 {
 	struct expr *i;
 
@@ -2243,7 +2243,7 @@ static void __binop_adjust(const struct expr *binop, struct expr *right,
 				binop_adjust_one(binop, i->key->right, shift);
 				break;
 			case EXPR_SET_ELEM:
-				__binop_adjust(binop, i->key->key, shift);
+				binop_adjust(binop, i->key->key, shift);
 				break;
 			default:
 				BUG("unknown expression type %s\n", expr_name(i->key));
@@ -2260,22 +2260,22 @@ static void __binop_adjust(const struct expr *binop, struct expr *right,
 	}
 }
 
-static void binop_adjust(struct expr *expr, unsigned int shift)
+static void binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr,
+			      struct expr **expr_binop)
 {
-	__binop_adjust(expr->left, expr->right, shift);
-}
-
-static void binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr)
-{
-	struct expr *binop = expr->left;
+	struct expr *binop = *expr_binop;
 	struct expr *left = binop->left;
 	struct expr *mask = binop->right;
 	unsigned int shift;
 
+	assert(binop->etype == EXPR_BINOP);
+
 	if ((left->etype == EXPR_PAYLOAD &&
 	    payload_expr_trim(left, mask, &ctx->pctx, &shift)) ||
 	    (left->etype == EXPR_EXTHDR &&
 	     exthdr_find_template(left, mask, &shift))) {
+		struct expr *right = NULL;
+
 		/* mask is implicit, binop needs to be removed.
 		 *
 		 * Fix all values of the expression according to the mask
@@ -2285,16 +2285,28 @@ static void binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr)
 		 * Finally, convert the expression to 1) by replacing
 		 * the binop with the binop payload/exthdr expression.
 		 */
-		binop_adjust(expr, shift);
+		switch (expr->etype) {
+		case EXPR_BINOP:
+		case EXPR_RELATIONAL:
+			right = expr->right;
+			binop_adjust(binop, right, shift);
+			break;
+		case EXPR_MAP:
+			right = expr->mappings;
+			binop_adjust(binop, right, shift);
+			break;
+		default:
+			break;
+		}
 
-		assert(expr->left->etype == EXPR_BINOP);
 		assert(binop->left == left);
-		expr->left = expr_get(left);
+		*expr_binop = expr_get(left);
 		expr_free(binop);
+
 		if (left->etype == EXPR_PAYLOAD)
 			payload_match_postprocess(ctx, expr, left);
-		else if (left->etype == EXPR_EXTHDR)
-			expr_set_type(expr->right, left->dtype, left->byteorder);
+		else if (left->etype == EXPR_EXTHDR && right)
+			expr_set_type(right, left->dtype, left->byteorder);
 	}
 }
 
@@ -2307,7 +2319,7 @@ static void map_binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr)
 
 	if (binop->left->etype == EXPR_PAYLOAD &&
 	    binop->right->etype == EXPR_VALUE)
-		binop_postprocess(ctx, expr);
+		binop_postprocess(ctx, expr, &expr->map);
 }
 
 static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
@@ -2401,7 +2413,7 @@ static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
 		 * payload_expr_trim will figure out if the mask is needed to match
 		 * templates.
 		 */
-		binop_postprocess(ctx, expr);
+		binop_postprocess(ctx, expr, &expr->left);
 	}
 }
 
@@ -2777,7 +2789,7 @@ static void stmt_payload_binop_pp(struct rule_pp_ctx *ctx, struct expr *binop)
 
 	assert(payload->etype == EXPR_PAYLOAD);
 	if (payload_expr_trim(payload, mask, &ctx->pctx, &shift)) {
-		__binop_adjust(binop, mask, shift);
+		binop_adjust(binop, mask, shift);
 		payload_expr_complete(payload, &ctx->pctx);
 		expr_set_type(mask, payload->dtype,
 			      payload->byteorder);
@@ -2872,7 +2884,7 @@ static void stmt_payload_binop_postprocess(struct rule_pp_ctx *ctx)
 		mpz_set(mask->value, bitmask);
 		mpz_clear(bitmask);
 
-		binop_postprocess(ctx, expr);
+		binop_postprocess(ctx, expr, &expr->left);
 		if (!payload_is_known(payload)) {
 			mpz_set(mask->value, tmp);
 			mpz_clear(tmp);
-- 
2.32.0

Re: [PATCH nft 0/3] netlink_delinearize cleanups

[Pablo Neira Ayuso]

On Wed, Dec 01, 2021 at 12:59:53PM +0100, Florian Westphal wrote:
> No indended behavioural changes here.
> binop postprocessing has been extended a lot and in a few places
> things only work because expr->map and expr->left alias to the same
> location.

LGTM.

We should add more type sanitization to expressions at some point, the
risk is that this might break a few things not covered by tests though.