[PATCH nft 0/7] really handle stacked l2 headers

[PATCH nft 0/7] really handle stacked l2 headers

[Florian Westphal]

Eric Garver reported a number of issues when matching vlan headers:

In:  update @macset { ether saddr . vlan id timeout 5s }
Out: update @macset { @ll,48,48 . @ll,112,16 & 0xfff timeout 5s }

This is because of amnesia in nft during expression decoding:
When we encounter 'vlan id', the L2 protocl (ethernet) is replaced by
vlan, so we attempt to match @ll,48,48 vs. the vlan header and come up
empty.

The vlan decode fails because we can't handle '& 0xfff' in this
instance, so we can locate the right offset but the payload expression
length doesn't match the template length (16 vs 12 bits).


The main patch is patch 3, which adds a stack of l2 protocols to track
instead of only keeping the cumulative size.

The latter is ok for serialization (we have the expression tree, so its
enough to add the size of the 'previous' l2 headers to payload
expressions that match the new 'top' l2 header.

But for deserialization, we need to be able to search all protocols base
headers seen.

The remaining patches improve handling of 'integer base type'
expressions and add test cases.

[PATCH nft 1/7] netlink_delinearize: allow postprocessing on concatenated elements

[Florian Westphal]

Currently there is no case where the individual expressions inside a
mapped concatenation need to be munged.

However, to support proper delinearization for an input like
'rule netdev nt nc set update ether saddr . vlan id timeout 5s @macset'

we need to allow this.

Right now, this gets listed as:

update @macset { @ll,48,48 . @ll,112,16 & 0xfff timeout 5s }

because the ethernet protocol is replaced by vlan beforehand,
so we fail to map @ll,48,48 to a vlan protocol.

Likewise, we can't map the vlan info either because we cannot
cope with the 'and' operation properly, nor is it removed.

Prepare for this by deleting and re-adding so that we do not
corrupt the linked list.

After this, the list can be safely changed and a followup patch
can start to delete/reallocate expressions.

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

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 3bdd98d47eb0..3835b3e522b9 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2539,16 +2539,21 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 		unsigned int type = expr->dtype->type, ntype = 0;
 		int off = expr->dtype->subtypes;
 		const struct datatype *dtype;
+		LIST_HEAD(tmp);
+		struct expr *n;
 
-		list_for_each_entry(i, &expr->expressions, list) {
+		list_for_each_entry_safe(i, n, &expr->expressions, list) {
 			if (type) {
 				dtype = concat_subtype_lookup(type, --off);
 				expr_set_type(i, dtype, dtype->byteorder);
 			}
+			list_del(&i->list);
 			expr_postprocess(ctx, &i);
+			list_add_tail(&i->list, &tmp);
 
 			ntype = concat_subtype_add(ntype, i->dtype->type);
 		}
+		list_splice(&tmp, &expr->expressions);
 		datatype_set(expr, concat_type_alloc(ntype));
 		break;
 	}
-- 
2.35.1

[PATCH nft 2/7] netlink_delinearize: postprocess binary ands in set expressions

[Florian Westphal]

Input:
update ether saddr . vlan id timeout 5s @macset

is now rendered as:
update @macset { @ll,48,48 . vlan id timeout 5s }

The @ll, ... is due to vlan description replacing the ethernet one,
so payload decode fails to take the concatenation apart (the ethernet
header payload info is matched vs. vlan template).

This will be adjusted by a followup patch.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/netlink.h         |  4 ++++
 src/netlink_delinearize.c | 39 +++++++++++++++++++++++++++++++++++----
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/include/netlink.h b/include/netlink.h
index e8e0f68ae1a4..2d5532387c0c 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -42,10 +42,14 @@ struct netlink_parse_ctx {
 	struct netlink_ctx	*nlctx;
 };
 
+
+#define RULE_PP_IN_CONCATENATION	(1 << 0)
+
 struct rule_pp_ctx {
 	struct proto_ctx	pctx;
 	struct payload_dep_ctx	pdctx;
 	struct stmt		*stmt;
+	unsigned int		flags;
 };
 
 extern const struct input_descriptor indesc_netlink;
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 3835b3e522b9..652c4975f8a5 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2260,12 +2260,13 @@ static void binop_adjust(const struct expr *binop, struct expr *right,
 	}
 }
 
-static void binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr,
-			      struct expr **expr_binop)
+static void __binop_postprocess(struct rule_pp_ctx *ctx,
+				struct expr *expr,
+				struct expr *left,
+				struct expr *mask,
+				struct expr **expr_binop)
 {
 	struct expr *binop = *expr_binop;
-	struct expr *left = binop->left;
-	struct expr *mask = binop->right;
 	unsigned int shift;
 
 	assert(binop->etype == EXPR_BINOP);
@@ -2310,6 +2311,16 @@ static void binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr,
 	}
 }
 
+static void binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr,
+			      struct expr **expr_binop)
+{
+	struct expr *binop = *expr_binop;
+	struct expr *left = binop->left;
+	struct expr *mask = binop->right;
+
+	__binop_postprocess(ctx, expr, left, mask, expr_binop);
+}
+
 static void map_binop_postprocess(struct rule_pp_ctx *ctx, struct expr *expr)
 {
 	struct expr *binop = expr->map;
@@ -2542,6 +2553,7 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 		LIST_HEAD(tmp);
 		struct expr *n;
 
+		ctx->flags |= RULE_PP_IN_CONCATENATION;
 		list_for_each_entry_safe(i, n, &expr->expressions, list) {
 			if (type) {
 				dtype = concat_subtype_lookup(type, --off);
@@ -2553,6 +2565,7 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 
 			ntype = concat_subtype_add(ntype, i->dtype->type);
 		}
+		ctx->flags &= ~RULE_PP_IN_CONCATENATION;
 		list_splice(&tmp, &expr->expressions);
 		datatype_set(expr, concat_type_alloc(ntype));
 		break;
@@ -2569,6 +2582,24 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 			expr_set_type(expr->right, &integer_type,
 				      BYTEORDER_HOST_ENDIAN);
 			break;
+		case OP_AND:
+			expr_set_type(expr->right, expr->left->dtype,
+				      expr->left->byteorder);
+
+			/* Only process OP_AND if we are inside a concatenation.
+			 *
+			 * Else, we remove it too early, for normal contect OP_AND
+			 * removal needs to be performed as part of the relational
+			 * operation because the RHS constant might need to be adjusted
+			 * (shifted).
+			 */
+			if ((ctx->flags & RULE_PP_IN_CONCATENATION) &&
+			    expr->left->etype == EXPR_PAYLOAD &&
+			    expr->right->etype == EXPR_VALUE) {
+				__binop_postprocess(ctx, expr, expr->left, expr->right, exprp);
+				return;
+			}
+			break;
 		default:
 			expr_set_type(expr->right, expr->left->dtype,
 				      expr->left->byteorder);
-- 
2.35.1

[PATCH nft 3/7] proto: track full stack of seen l2 protocols, not just cumulative offset

[Florian Westphal]

For input, a cumulative size counter of all pushed l2 headers is enough,
because we have the full expression tree available to us.

For delinearization we need to track all seen l2 headers, else we lose
information that we might need at a later time.

Consider:

rule netdev nt nc set update ether saddr . vlan id

during delinearization, the vlan proto_desc replaces the ethernet one,
and by the time we try to split the concatenation apart we will search
the ether saddr offset vs. the templates for proto_vlan.

This replaces the offset with an array that stores the protocol
descriptions seen.

Then, if the payload offset is larger than our description, search the
l2 stack and adjust the offset until we're within the expected offset
boundary.

Reported-by: Eric Garver <eric@garver.life>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/proto.h           |  3 +-
 src/evaluate.c            | 15 +++++++--
 src/netlink_delinearize.c |  5 ---
 src/payload.c             | 67 ++++++++++++++++++++++++++++++++-------
 src/proto.c               |  2 --
 5 files changed, 71 insertions(+), 21 deletions(-)

diff --git a/include/proto.h b/include/proto.h
index a04240a5de81..35e760c7e16e 100644
--- a/include/proto.h
+++ b/include/proto.h
@@ -193,13 +193,14 @@ struct proto_ctx {
 	struct {
 		struct location			location;
 		const struct proto_desc		*desc;
-		unsigned int			offset;
 		struct {
 			struct location		location;
 			const struct proto_desc	*desc;
 		} protos[PROTO_CTX_NUM_PROTOS];
 		unsigned int			num_protos;
 	} protocol[PROTO_BASE_MAX + 1];
+	const struct proto_desc *stacked_ll[PROTO_CTX_NUM_PROTOS];
+	uint8_t stacked_ll_count;
 };
 
 extern void proto_ctx_init(struct proto_ctx *ctx, unsigned int family,
diff --git a/src/evaluate.c b/src/evaluate.c
index 9ae525769bc3..be9fcd5117fb 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -678,7 +678,13 @@ static int resolve_protocol_conflict(struct eval_ctx *ctx,
 	    conflict_resolution_gen_dependency(ctx, link, payload, &nstmt) < 0)
 		return 1;
 
-	payload->payload.offset += ctx->pctx.protocol[base].offset;
+	if (base == PROTO_BASE_LL_HDR) {
+		unsigned int i;
+
+		for (i = 0; i < ctx->pctx.stacked_ll_count; i++)
+			payload->payload.offset += ctx->pctx.stacked_ll[i]->length;
+	}
+
 	rule_stmt_insert_at(ctx->rule, nstmt, ctx->stmt);
 
 	return 0;
@@ -727,7 +733,12 @@ static int __expr_evaluate_payload(struct eval_ctx *ctx, struct expr *expr)
 	if (desc == payload->payload.desc) {
 		const struct proto_hdr_template *tmpl;
 
-		payload->payload.offset += ctx->pctx.protocol[base].offset;
+		if (desc->base == PROTO_BASE_LL_HDR) {
+			unsigned int i;
+
+			for (i = 0; i < ctx->pctx.stacked_ll_count; i++)
+				payload->payload.offset += ctx->pctx.stacked_ll[i]->length;
+		}
 check_icmp:
 		if (desc != &proto_icmp && desc != &proto_icmp6)
 			return 0;
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 652c4975f8a5..f26c406ddd68 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1977,11 +1977,6 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
 				      struct expr *expr,
 				      struct expr *payload)
 {
-	enum proto_bases base = payload->payload.base;
-
-	assert(payload->payload.offset >= ctx->pctx.protocol[base].offset);
-	payload->payload.offset -= ctx->pctx.protocol[base].offset;
-
 	switch (expr->op) {
 	case OP_EQ:
 	case OP_NEQ:
diff --git a/src/payload.c b/src/payload.c
index 66418cddb3b5..2c0d0ac9e8ae 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -116,8 +116,13 @@ static void payload_expr_pctx_update(struct proto_ctx *ctx,
 	if (desc->base == base->base) {
 		assert(base->length > 0);
 
-		if (!left->payload.is_raw)
-			ctx->protocol[base->base].offset += base->length;
+		if (!left->payload.is_raw) {
+			if (desc->base == PROTO_BASE_LL_HDR &&
+			    ctx->stacked_ll_count < PROTO_CTX_NUM_PROTOS) {
+				ctx->stacked_ll[ctx->stacked_ll_count] = base;
+				ctx->stacked_ll_count++;
+			}
+		}
 	}
 	proto_ctx_update(ctx, desc->base, loc, desc);
 }
@@ -869,6 +874,38 @@ void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 	}
 }
 
+static const struct proto_desc *get_stacked_desc(const struct proto_ctx *ctx,
+						 const struct proto_desc *top,
+						 const struct expr *e,
+						 unsigned int *skip)
+{
+	unsigned int i, total, payload_offset = e->payload.offset;
+
+	assert(e->etype == EXPR_PAYLOAD);
+
+	if (e->payload.base != PROTO_BASE_LL_HDR ||
+	    payload_offset < top->length) {
+		*skip = 0;
+		return top;
+	}
+
+	for (i = 0, total = 0; i < ctx->stacked_ll_count; i++) {
+		const struct proto_desc *stacked;
+
+		stacked = ctx->stacked_ll[i];
+		if (payload_offset < stacked->length) {
+			*skip = total;
+			return stacked;
+		}
+
+		payload_offset -= stacked->length;
+		total += stacked->length;
+	}
+
+	*skip = total;
+	return top;
+}
+
 /**
  * payload_expr_complete - fill in type information of a raw payload expr
  *
@@ -880,9 +917,10 @@ void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
  */
 void payload_expr_complete(struct expr *expr, const struct proto_ctx *ctx)
 {
+	unsigned int payload_offset = expr->payload.offset;
 	const struct proto_desc *desc;
 	const struct proto_hdr_template *tmpl;
-	unsigned int i;
+	unsigned int i, total;
 
 	assert(expr->etype == EXPR_PAYLOAD);
 
@@ -891,9 +929,12 @@ void payload_expr_complete(struct expr *expr, const struct proto_ctx *ctx)
 		return;
 	assert(desc->base == expr->payload.base);
 
+	desc = get_stacked_desc(ctx, desc, expr, &total);
+	payload_offset -= total;
+
 	for (i = 0; i < array_size(desc->templates); i++) {
 		tmpl = &desc->templates[i];
-		if (tmpl->offset != expr->payload.offset ||
+		if (tmpl->offset != payload_offset ||
 		    tmpl->len    != expr->len)
 			continue;
 
@@ -950,6 +991,7 @@ bool payload_expr_trim(struct expr *expr, struct expr *mask,
 	unsigned int payload_len = expr->len;
 	const struct proto_desc *desc;
 	unsigned int off, i, len = 0;
+	unsigned int total;
 
 	assert(expr->etype == EXPR_PAYLOAD);
 
@@ -959,10 +1001,8 @@ bool payload_expr_trim(struct expr *expr, struct expr *mask,
 
 	assert(desc->base == expr->payload.base);
 
-	if (ctx->protocol[expr->payload.base].offset) {
-		assert(payload_offset >= ctx->protocol[expr->payload.base].offset);
-		payload_offset -= ctx->protocol[expr->payload.base].offset;
-	}
+	desc = get_stacked_desc(ctx, desc, expr, &total);
+	payload_offset -= total;
 
 	off = round_up(mask->len, BITS_PER_BYTE) - mask_len;
 	payload_offset += off;
@@ -1009,10 +1049,11 @@ bool payload_expr_trim(struct expr *expr, struct expr *mask,
 void payload_expr_expand(struct list_head *list, struct expr *expr,
 			 const struct proto_ctx *ctx)
 {
+	unsigned int payload_offset = expr->payload.offset;
 	const struct proto_hdr_template *tmpl;
 	const struct proto_desc *desc;
+	unsigned int i, total;
 	struct expr *new;
-	unsigned int i;
 
 	assert(expr->etype == EXPR_PAYLOAD);
 
@@ -1021,13 +1062,16 @@ void payload_expr_expand(struct list_head *list, struct expr *expr,
 		goto raw;
 	assert(desc->base == expr->payload.base);
 
+	desc = get_stacked_desc(ctx, desc, expr, &total);
+	payload_offset -= total;
+
 	for (i = 1; i < array_size(desc->templates); i++) {
 		tmpl = &desc->templates[i];
 
 		if (tmpl->len == 0)
 			break;
 
-		if (tmpl->offset != expr->payload.offset)
+		if (tmpl->offset != payload_offset)
 			continue;
 
 		if (tmpl->icmp_dep && ctx->th_dep.icmp.type &&
@@ -1039,6 +1083,7 @@ void payload_expr_expand(struct list_head *list, struct expr *expr,
 			list_add_tail(&new->list, list);
 			expr->len	     -= tmpl->len;
 			expr->payload.offset += tmpl->len;
+			payload_offset       += tmpl->len;
 			if (expr->len == 0)
 				return;
 		} else if (expr->len > 0) {
@@ -1051,7 +1096,7 @@ void payload_expr_expand(struct list_head *list, struct expr *expr,
 	}
 raw:
 	new = payload_expr_alloc(&expr->location, NULL, 0);
-	payload_init_raw(new, expr->payload.base, expr->payload.offset,
+	payload_init_raw(new, expr->payload.base, payload_offset,
 			 expr->len);
 	list_add_tail(&new->list, list);
 }
diff --git a/src/proto.c b/src/proto.c
index a013a00d2c7b..2663f216860b 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -160,8 +160,6 @@ static void proto_ctx_debug(const struct proto_ctx *ctx, enum proto_bases base,
 			 proto_base_names[i],
 			 ctx->protocol[i].desc ? ctx->protocol[i].desc->name :
 						 "none");
-		if (ctx->protocol[i].offset)
-			pr_debug(" (offset: %u)", ctx->protocol[i].offset);
 		if (i == base)
 			pr_debug(" <-");
 		pr_debug("\n");
-- 
2.35.1

[PATCH nft 4/7] debug: dump the l2 protocol stack

[Florian Westphal]

Previously we used to print the cumulative size of the headers,
update this to print the tracked l2 stack.

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

diff --git a/src/proto.c b/src/proto.c
index 2663f216860b..c49648275e12 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -154,6 +154,12 @@ static void proto_ctx_debug(const struct proto_ctx *ctx, enum proto_bases base,
 	if (!(debug_mask & NFT_DEBUG_PROTO_CTX))
 		return;
 
+	if (base == PROTO_BASE_LL_HDR && ctx->stacked_ll_count) {
+		pr_debug(" saved ll headers:");
+		for (i = 0; i < ctx->stacked_ll_count; i++)
+			pr_debug(" %s", ctx->stacked_ll[i]->name);
+	}
+
 	pr_debug("update %s protocol context:\n", proto_base_names[base]);
 	for (i = PROTO_BASE_LL_HDR; i <= PROTO_BASE_MAX; i++) {
 		pr_debug(" %-20s: %s",
-- 
2.35.1

[PATCH nft 5/7] tests: add a test case for ether and vlan listing

[Florian Westphal]

before this patch series, test fails dump validation:
-               update @macset { ether saddr . vlan id timeout 5s } counter packets 0 bytes 0
-               ether saddr . vlan id @macset
+               update @macset { @ll,48,48 . @ll,112,16 & 0xfff timeout 5s } counter packets 0 bytes 0
+               @ll,48,48 . @ll,112,16 & 0xfff @macset

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/shell/testcases/sets/0070stacked_l2_headers  |  6 ++++++
 .../sets/dumps/0070stacked_l2_headers.nft          | 14 ++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100755 tests/shell/testcases/sets/0070stacked_l2_headers
 create mode 100644 tests/shell/testcases/sets/dumps/0070stacked_l2_headers.nft

diff --git a/tests/shell/testcases/sets/0070stacked_l2_headers b/tests/shell/testcases/sets/0070stacked_l2_headers
new file mode 100755
index 000000000000..07820b7c4fdd
--- /dev/null
+++ b/tests/shell/testcases/sets/0070stacked_l2_headers
@@ -0,0 +1,6 @@
+#!/bin/bash
+
+set -e
+dumpfile=$(dirname $0)/dumps/$(basename $0).nft
+
+$NFT -f "$dumpfile"
diff --git a/tests/shell/testcases/sets/dumps/0070stacked_l2_headers.nft b/tests/shell/testcases/sets/dumps/0070stacked_l2_headers.nft
new file mode 100644
index 000000000000..ef254b96879e
--- /dev/null
+++ b/tests/shell/testcases/sets/dumps/0070stacked_l2_headers.nft
@@ -0,0 +1,14 @@
+table netdev nt {
+	set macset {
+		typeof ether saddr . vlan id
+		size 1024
+		flags dynamic,timeout
+	}
+
+	chain nc {
+		update @macset { ether saddr . vlan id timeout 5s } counter packets 0 bytes 0
+		ether saddr . vlan id @macset
+		vlan pcp 1
+		ether saddr 0a:0b:0c:0d:0e:0f vlan id 42
+	}
+}
-- 
2.35.1

[PATCH nft 6/7] evaluate: search stacked header list for matching payload dep

[Florian Westphal]

"ether saddr 0:1:2:3:4:6 vlan id 2" works, but reverse fails:

"vlan id 2 ether saddr 0:1:2:3:4:6" will give
Error: conflicting protocols specified: vlan vs. ether

After "proto: track full stack of seen l2 protocols, not just cumulative offset",
we have a list of all l2 headers, so search those to see if we had this
proto base in the past before rejecting this.

Reported-by: Eric Garver <eric@garver.life>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                        | 21 +++++++---
 tests/py/bridge/vlan.t                |  3 ++
 tests/py/bridge/vlan.t.json           | 56 +++++++++++++++++++++++++++
 tests/py/bridge/vlan.t.payload        | 16 ++++++++
 tests/py/bridge/vlan.t.payload.netdev | 20 ++++++++++
 5 files changed, 110 insertions(+), 6 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index be9fcd5117fb..919c38c5604e 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -659,13 +659,22 @@ static int resolve_protocol_conflict(struct eval_ctx *ctx,
 	struct stmt *nstmt = NULL;
 	int link, err;
 
-	if (payload->payload.base == PROTO_BASE_LL_HDR &&
-	    proto_is_dummy(desc)) {
-		err = meta_iiftype_gen_dependency(ctx, payload, &nstmt);
-		if (err < 0)
-			return err;
+	if (payload->payload.base == PROTO_BASE_LL_HDR) {
+		if (proto_is_dummy(desc)) {
+			err = meta_iiftype_gen_dependency(ctx, payload, &nstmt);
+			if (err < 0)
+				return err;
 
-		rule_stmt_insert_at(ctx->rule, nstmt, ctx->stmt);
+			rule_stmt_insert_at(ctx->rule, nstmt, ctx->stmt);
+		} else {
+			unsigned int i;
+
+			/* payload desc stored in the L2 header stack? No conflict. */
+			for (i = 0; i < ctx->pctx.stacked_ll_count; i++) {
+				if (ctx->pctx.stacked_ll[i] == payload->payload.desc)
+					return 0;
+			}
+		}
 	}
 
 	assert(base <= PROTO_BASE_MAX);
diff --git a/tests/py/bridge/vlan.t b/tests/py/bridge/vlan.t
index 924ed4ed3679..49206017fff2 100644
--- a/tests/py/bridge/vlan.t
+++ b/tests/py/bridge/vlan.t
@@ -47,3 +47,6 @@ ether type ip vlan id 1 ip saddr 10.0.0.1;fail
 
 # mangling
 vlan id 1 vlan id set 2;ok
+
+ether saddr 00:01:02:03:04:05 vlan id 1;ok
+vlan id 2 ether saddr 0:1:2:3:4:6;ok;ether saddr 00:01:02:03:04:06 vlan id 2
diff --git a/tests/py/bridge/vlan.t.json b/tests/py/bridge/vlan.t.json
index e7640f9a6a37..58d4a40f5baf 100644
--- a/tests/py/bridge/vlan.t.json
+++ b/tests/py/bridge/vlan.t.json
@@ -761,3 +761,59 @@
         }
     }
 ]
+
+# ether saddr 00:01:02:03:04:05 vlan id 1
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "saddr",
+                    "protocol": "ether"
+                }
+            },
+            "op": "==",
+            "right": "00:01:02:03:04:05"
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "vlan"
+                }
+            },
+            "op": "==",
+            "right": 1
+        }
+    }
+]
+
+# vlan id 2 ether saddr 0:1:2:3:4:6
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "saddr",
+                    "protocol": "ether"
+                }
+            },
+            "op": "==",
+            "right": "00:01:02:03:04:06"
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "vlan"
+                }
+            },
+            "op": "==",
+            "right": 2
+        }
+    }
+]
diff --git a/tests/py/bridge/vlan.t.payload b/tests/py/bridge/vlan.t.payload
index 6c8d595a1aad..713670e9e721 100644
--- a/tests/py/bridge/vlan.t.payload
+++ b/tests/py/bridge/vlan.t.payload
@@ -276,3 +276,19 @@ bridge
   [ payload load 2b @ link header + 14 => reg 1 ]
   [ bitwise reg 1 = ( reg 1 & 0x000000f0 ) ^ 0x00000200 ]
   [ payload write reg 1 => 2b @ link header + 14 csum_type 0 csum_off 0 csum_flags 0x0 ]
+
+# ether saddr 00:01:02:03:04:05 vlan id 1
+bridge test-bridge input
+  [ payload load 8b @ link header + 6 => reg 1 ]
+  [ cmp eq reg 1 0x03020100 0x00810504 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0x0000ff0f ) ^ 0x00000000 ]
+  [ cmp eq reg 1 0x00000100 ]
+
+# vlan id 2 ether saddr 0:1:2:3:4:6
+bridge test-bridge input
+  [ payload load 8b @ link header + 6 => reg 1 ]
+  [ cmp eq reg 1 0x03020100 0x00810604 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0x0000ff0f ) ^ 0x00000000 ]
+  [ cmp eq reg 1 0x00000200 ]
diff --git a/tests/py/bridge/vlan.t.payload.netdev b/tests/py/bridge/vlan.t.payload.netdev
index d2c7d74a4e85..98a2a2b0f379 100644
--- a/tests/py/bridge/vlan.t.payload.netdev
+++ b/tests/py/bridge/vlan.t.payload.netdev
@@ -322,3 +322,23 @@ netdev
   [ payload load 2b @ link header + 14 => reg 1 ]
   [ bitwise reg 1 = ( reg 1 & 0x000000f0 ) ^ 0x00000200 ]
   [ payload write reg 1 => 2b @ link header + 14 csum_type 0 csum_off 0 csum_flags 0x0 ]
+
+# vlan id 2 ether saddr 0:1:2:3:4:6
+netdev test-netdev ingress
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x00000001 ]
+  [ payload load 8b @ link header + 6 => reg 1 ]
+  [ cmp eq reg 1 0x03020100 0x00810604 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0x0000ff0f ) ^ 0x00000000 ]
+  [ cmp eq reg 1 0x00000200 ]
+
+# ether saddr 00:01:02:03:04:05 vlan id 1
+netdev test-netdev ingress
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x00000001 ]
+  [ payload load 8b @ link header + 6 => reg 1 ]
+  [ cmp eq reg 1 0x03020100 0x00810504 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0x0000ff0f ) ^ 0x00000000 ]
+  [ cmp eq reg 1 0x00000100 ]
-- 
2.35.1

[PATCH nft 7/7] src: allow anon set concatenation with ether and vlan

[Florian Westphal]

vlan id uses integer type (which has a length of 0).

Using it was possible, but listing would assert:
python: mergesort.c:24: concat_expr_msort_value: Assertion `ilen > 0' failed.

There are two reasons for this.
First reason is that the udata/typeof information lacks the 'vlan id'
part, because internally this is 'payload . binop(payload AND mask)'.

binop lacks an udata store.  It makes little sense to store it,
'typeof' keyword expects normal match syntax.

So, when storing udata, store the left hand side of the binary
operation, i.e. the load of the 2-byte key.

With that resolved, delinerization could work, but concat_elem_expr()
would splice 12 bits off the elements value, but it should be 16 (on
a byte boundary).

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/expression.c                      | 17 +++++++++--
 src/netlink.c                         | 10 +++++--
 tests/py/bridge/vlan.t                |  2 ++
 tests/py/bridge/vlan.t.json           | 41 +++++++++++++++++++++++++++
 tests/py/bridge/vlan.t.payload        | 12 ++++++++
 tests/py/bridge/vlan.t.payload.netdev | 14 +++++++++
 6 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/src/expression.c b/src/expression.c
index deb649e1847b..7390089cf57d 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -879,17 +879,30 @@ static void concat_expr_print(const struct expr *expr, struct output_ctx *octx)
 #define NFTNL_UDATA_SET_KEY_CONCAT_SUB_DATA 1
 #define NFTNL_UDATA_SET_KEY_CONCAT_SUB_MAX  2
 
+static struct expr *expr_build_udata_recurse(struct expr *e)
+{
+	switch (e->etype) {
+	case EXPR_BINOP:
+		return e->left;
+	default:
+		break;
+	}
+
+	return e;
+}
+
 static int concat_expr_build_udata(struct nftnl_udata_buf *udbuf,
 				    const struct expr *concat_expr)
 {
 	struct nftnl_udata *nest;
+	struct expr *expr, *tmp;
 	unsigned int i = 0;
-	struct expr *expr;
 
-	list_for_each_entry(expr, &concat_expr->expressions, list) {
+	list_for_each_entry_safe(expr, tmp, &concat_expr->expressions, list) {
 		struct nftnl_udata *nest_expr;
 		int err;
 
+		expr = expr_build_udata_recurse(expr);
 		if (!expr_ops(expr)->build_udata || i >= NFT_REG32_SIZE)
 			return -1;
 
diff --git a/src/netlink.c b/src/netlink.c
index 89d864ed046a..799cf9b8ebef 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1114,17 +1114,21 @@ static struct expr *concat_elem_expr(struct expr *key,
 				     struct expr *data, int *off)
 {
 	const struct datatype *subtype;
+	unsigned int sub_length;
 	struct expr *expr;
 
 	if (key) {
 		(*off)--;
-		expr = constant_expr_splice(data, key->len);
+		sub_length = round_up(key->len, BITS_PER_BYTE);
+
+		expr = constant_expr_splice(data, sub_length);
 		expr->dtype = datatype_get(key->dtype);
 		expr->byteorder = key->byteorder;
 		expr->len = key->len;
 	} else {
 		subtype = concat_subtype_lookup(dtype->type, --(*off));
-		expr = constant_expr_splice(data, subtype->size);
+		sub_length = round_up(subtype->size, BITS_PER_BYTE);
+		expr = constant_expr_splice(data, sub_length);
 		expr->dtype = subtype;
 		expr->byteorder = subtype->byteorder;
 	}
@@ -1136,7 +1140,7 @@ static struct expr *concat_elem_expr(struct expr *key,
 	    expr->dtype->basetype->type == TYPE_BITMASK)
 		expr = bitmask_expr_to_binops(expr);
 
-	data->len -= netlink_padding_len(expr->len);
+	data->len -= netlink_padding_len(sub_length);
 
 	return expr;
 }
diff --git a/tests/py/bridge/vlan.t b/tests/py/bridge/vlan.t
index 49206017fff2..95bdff4f1b75 100644
--- a/tests/py/bridge/vlan.t
+++ b/tests/py/bridge/vlan.t
@@ -50,3 +50,5 @@ vlan id 1 vlan id set 2;ok
 
 ether saddr 00:01:02:03:04:05 vlan id 1;ok
 vlan id 2 ether saddr 0:1:2:3:4:6;ok;ether saddr 00:01:02:03:04:06 vlan id 2
+
+ether saddr . vlan id { 0a:0b:0c:0d:0e:0f . 42, 0a:0b:0c:0d:0e:0f . 4095 };ok
diff --git a/tests/py/bridge/vlan.t.json b/tests/py/bridge/vlan.t.json
index 58d4a40f5baf..f77756f5080a 100644
--- a/tests/py/bridge/vlan.t.json
+++ b/tests/py/bridge/vlan.t.json
@@ -817,3 +817,44 @@
         }
     }
 ]
+
+# ether saddr . vlan id { 0a:0b:0c:0d:0e:0f . 42, 0a:0b:0c:0d:0e:0f . 4095 }
+[
+    {
+        "match": {
+            "left": {
+                "concat": [
+                    {
+                        "payload": {
+                            "field": "saddr",
+                            "protocol": "ether"
+                        }
+                    },
+                    {
+                        "payload": {
+                            "field": "id",
+                            "protocol": "vlan"
+                        }
+                    }
+                ]
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    {
+                        "concat": [
+                            "0a:0b:0c:0d:0e:0f",
+                            42
+                        ]
+                    },
+                    {
+                        "concat": [
+                            "0a:0b:0c:0d:0e:0f",
+                            4095
+                        ]
+                    }
+                ]
+            }
+        }
+    }
+]
diff --git a/tests/py/bridge/vlan.t.payload b/tests/py/bridge/vlan.t.payload
index 713670e9e721..62e4b89bd0b2 100644
--- a/tests/py/bridge/vlan.t.payload
+++ b/tests/py/bridge/vlan.t.payload
@@ -292,3 +292,15 @@ bridge test-bridge input
   [ payload load 2b @ link header + 14 => reg 1 ]
   [ bitwise reg 1 = ( reg 1 & 0x0000ff0f ) ^ 0x00000000 ]
   [ cmp eq reg 1 0x00000200 ]
+
+# ether saddr . vlan id { 0a:0b:0c:0d:0e:0f . 42, 0a:0b:0c:0d:0e:0f . 4095 }
+__set%d test-bridge 3 size 2
+__set%d test-bridge 0
+	element 0d0c0b0a 00000f0e 00002a00  : 0 [end]	element 0d0c0b0a 00000f0e 0000ff0f  : 0 [end]
+bridge test-bridge input
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x00000081 ]
+  [ payload load 6b @ link header + 6 => reg 1 ]
+  [ payload load 2b @ link header + 14 => reg 10 ]
+  [ bitwise reg 10 = ( reg 10 & 0x0000ff0f ) ^ 0x00000000 ]
+  [ lookup reg 1 set __set%d ]
diff --git a/tests/py/bridge/vlan.t.payload.netdev b/tests/py/bridge/vlan.t.payload.netdev
index 98a2a2b0f379..1018d4c6588f 100644
--- a/tests/py/bridge/vlan.t.payload.netdev
+++ b/tests/py/bridge/vlan.t.payload.netdev
@@ -342,3 +342,17 @@ netdev test-netdev ingress
   [ payload load 2b @ link header + 14 => reg 1 ]
   [ bitwise reg 1 = ( reg 1 & 0x0000ff0f ) ^ 0x00000000 ]
   [ cmp eq reg 1 0x00000100 ]
+
+# ether saddr . vlan id { 0a:0b:0c:0d:0e:0f . 42, 0a:0b:0c:0d:0e:0f . 4095 }
+__set%d test-netdev 3 size 2
+__set%d test-netdev 0
+	element 0d0c0b0a 00000f0e 00002a00  : 0 [end]	element 0d0c0b0a 00000f0e 0000ff0f  : 0 [end]
+netdev test-netdev ingress
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x00000001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x00000081 ]
+  [ payload load 6b @ link header + 6 => reg 1 ]
+  [ payload load 2b @ link header + 14 => reg 10 ]
+  [ bitwise reg 10 = ( reg 10 & 0x0000ff0f ) ^ 0x00000000 ]
+  [ lookup reg 1 set __set%d ]
-- 
2.35.1

Re: [PATCH nft 2/7] netlink_delinearize: postprocess binary ands in set expressions

[Pablo Neira Ayuso]

Hi Florian,

On Wed, Jul 27, 2022 at 01:19:58PM +0200, Florian Westphal wrote:
[..]
> diff --git a/include/netlink.h b/include/netlink.h
> index e8e0f68ae1a4..2d5532387c0c 100644
> --- a/include/netlink.h
> +++ b/include/netlink.h
[...]
> @@ -2569,6 +2582,24 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
>  			expr_set_type(expr->right, &integer_type,
>  				      BYTEORDER_HOST_ENDIAN);
>  			break;
> +		case OP_AND:
> +			expr_set_type(expr->right, expr->left->dtype,
> +				      expr->left->byteorder);
> +
> +			/* Only process OP_AND if we are inside a concatenation.
> +			 *
> +			 * Else, we remove it too early, for normal contect OP_AND
> +			 * removal needs to be performed as part of the relational
> +			 * operation because the RHS constant might need to be adjusted
> +			 * (shifted).
> +			 */
> +			if ((ctx->flags & RULE_PP_IN_CONCATENATION) &&
> +			    expr->left->etype == EXPR_PAYLOAD &&
> +			    expr->right->etype == EXPR_VALUE) {
> +				__binop_postprocess(ctx, expr, expr->left, expr->right, exprp);
> +				return;
> +			}
> +			break;

Not sure this flag is enough. If I load this ruleset

 table netdev nt {
       set macset {
               typeof vlan id
               size 1024
               flags dynamic,timeout
       }
        chain y {
        }
 }
 add rule netdev nt y update @macset { vlan id timeout 5s }

listing still shows the raw expression:

 table netdev nt {
        set macset {
                typeof vlan id
                size 1024
                flags dynamic,timeout
        }

        chain y {
                update @macset { @ll,112,16 & 0xfff timeout 5s }
        }
 }

looks like the problem is related to expressions in set statements?

Remaining patches in this series LGTM.

Re: [PATCH nft 2/7] netlink_delinearize: postprocess binary ands in set expressions

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Florian,
> 
> On Wed, Jul 27, 2022 at 01:19:58PM +0200, Florian Westphal wrote:
> [..]
> > diff --git a/include/netlink.h b/include/netlink.h
> > index e8e0f68ae1a4..2d5532387c0c 100644
> > --- a/include/netlink.h
> > +++ b/include/netlink.h
> [...]
> > @@ -2569,6 +2582,24 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
> >  			expr_set_type(expr->right, &integer_type,
> >  				      BYTEORDER_HOST_ENDIAN);
> >  			break;
> > +		case OP_AND:
> > +			expr_set_type(expr->right, expr->left->dtype,
> > +				      expr->left->byteorder);
> > +
> > +			/* Only process OP_AND if we are inside a concatenation.
> > +			 *
> > +			 * Else, we remove it too early, for normal contect OP_AND
> > +			 * removal needs to be performed as part of the relational
> > +			 * operation because the RHS constant might need to be adjusted
> > +			 * (shifted).
> > +			 */
> > +			if ((ctx->flags & RULE_PP_IN_CONCATENATION) &&
> > +			    expr->left->etype == EXPR_PAYLOAD &&
> > +			    expr->right->etype == EXPR_VALUE) {
> > +				__binop_postprocess(ctx, expr, expr->left, expr->right, exprp);
> > +				return;
> > +			}
> > +			break;
> 
> Not sure this flag is enough. If I load this ruleset
> 
>  table netdev nt {
>        set macset {
>                typeof vlan id
>                size 1024
>                flags dynamic,timeout
>        }
>         chain y {
>         }
>  }
>  add rule netdev nt y update @macset { vlan id timeout 5s }
> 
> listing still shows the raw expression:
> 
>  table netdev nt {
>         set macset {
>                 typeof vlan id
>                 size 1024
>                 flags dynamic,timeout
>         }
> 
>         chain y {
>                 update @macset { @ll,112,16 & 0xfff timeout 5s }
>         }
>  }
> 
> looks like the problem is related to expressions in set statements?

Good catch, I added a patch to the series that also enabled OP_AND
removal for the set elem key case.

I took the above and added it to the test case.