[RFC] concat with dynamically sized fields like vlan id

[RFC] concat with dynamically sized fields like vlan id

[Michael Braun]

This enables commands like

 nft set bridge t s4 '{typeof vlan id . ip daddr; elements = { 3567 .
1.2.3.4 }; }'

Which would previously fail with
  Error: can not use variable sized data types (integer) in concat
  expressions

Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
---
 include/datatype.h                            |  6 ++-
 src/datatype.c                                | 32 +++++++++++++--
 src/evaluate.c                                | 39 ++++++++++++++-----
 src/expression.c                              | 11 +++++-
 src/netlink.c                                 | 33 ++++++++++++++--
 src/netlink_delinearize.c                     |  9 ++++-
 .../testcases/sets/dumps/typeof_sets_0.nft    |  9 +++++
 tests/shell/testcases/sets/typeof_sets_0      |  9 +++++
 8 files changed, 127 insertions(+), 21 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index 04b4892b..1d1cd286 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -138,6 +138,7 @@ struct parse_ctx;
  * @flags:	flags
  * @size:	type size (fixed sized non-basetypes only)
  * @subtypes:	number of subtypes (concat type)
+ * @subsizes:	sizes of subtypes (concat type)
  * @name:	type name
  * @desc:	type description
  * @basetype:	basetype for subtypes, determines type compatibility
@@ -153,6 +154,7 @@ struct datatype {
 	unsigned int			flags;
 	unsigned int			size;
 	unsigned int			subtypes;
+	unsigned int			*subsizes;
 	const char			*name;
 	const char			*desc;
 	const struct datatype		*basetype;
@@ -273,7 +275,9 @@ extern const struct datatype policy_type;
 
 void inet_service_type_print(const struct expr *expr, struct output_ctx *octx);
 
-extern const struct datatype *concat_type_alloc(uint32_t type);
+extern const struct datatype *concat_type_alloc(uint32_t type,
+						unsigned int numsizes,
+						unsigned int *sizes);
 
 static inline uint32_t concat_subtype_add(uint32_t type, uint32_t subtype)
 {
diff --git a/src/datatype.c b/src/datatype.c
index 0110846f..52053a42 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -1116,22 +1116,30 @@ void datatype_free(const struct datatype *ptr)
 
 	xfree(dtype->name);
 	xfree(dtype->desc);
+	xfree(dtype->subsizes);
 	xfree(dtype);
 }
 
-const struct datatype *concat_type_alloc(uint32_t type)
+/**
+ * @type: bitshifted TYPEID | TYPEID | ...
+ * @numsizes: size of sizes argument (number of entries)
+ * @sizes: for dtype with size==0, this gives the respective dynamic length
+ */
+const struct datatype *concat_type_alloc(uint32_t type, unsigned int numsizes,
+					 unsigned int *sizes)
 {
 	const struct datatype *i;
 	struct datatype *dtype;
 	char desc[256] = "concatenation of (";
 	char name[256] = "";
-	unsigned int size = 0, subtypes = 0, n;
+	unsigned int size = 0, subtypes = 0, n, k = 0, *subsizes;
 
 	n = div_round_up(fls(type), TYPE_BITS);
+	subsizes = xzalloc(n * sizeof(subsizes[0]));
 	while (n > 0 && concat_subtype_id(type, --n)) {
 		i = concat_subtype_lookup(type, n);
 		if (i == NULL)
-			return NULL;
+			goto err;
 
 		if (subtypes != 0) {
 			strncat(desc, ", ", sizeof(desc) - strlen(desc) - 1);
@@ -1140,7 +1148,19 @@ const struct datatype *concat_type_alloc(uint32_t type)
 		strncat(desc, i->desc, sizeof(desc) - strlen(desc) - 1);
 		strncat(name, i->name, sizeof(name) - strlen(name) - 1);
 
-		size += netlink_padded_len(i->size);
+		subsizes[n] = i->size;
+		if (subsizes[n] == 0 && k < numsizes) {
+			char ssize[20] = "";
+			subsizes[n] = sizes[k++];
+			snprintf(ssize, sizeof(ssize), "(%u)", subsizes[n]);
+			strncat(desc, ssize, sizeof(desc) - strlen(desc) - 1);
+			strncat(name, ssize, sizeof(name) - strlen(name) - 1);
+		}
+
+		if (subsizes[n] == 0)
+			goto err;
+
+		size += netlink_padded_len(subsizes[n]);
 		subtypes++;
 	}
 	strncat(desc, ")", sizeof(desc) - strlen(desc) - 1);
@@ -1149,11 +1169,15 @@ const struct datatype *concat_type_alloc(uint32_t type)
 	dtype->type	= type;
 	dtype->size	= size;
 	dtype->subtypes = subtypes;
+	dtype->subsizes = subsizes;
 	dtype->name	= xstrdup(name);
 	dtype->desc	= xstrdup(desc);
 	dtype->parse	= concat_type_parse;
 
 	return dtype;
+err:
+	xfree(subsizes);
+	return NULL;
 }
 
 const struct datatype *set_datatype_alloc(const struct datatype *orig_dtype,
diff --git a/src/evaluate.c b/src/evaluate.c
index 59714131..ec96dd58 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1227,42 +1227,63 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr,
 	int off = dtype ? dtype->subtypes : 0;
 	unsigned int flags = EXPR_F_CONSTANT | EXPR_F_SINGLETON;
 	struct expr *i, *next;
+	unsigned int* sizes = NULL;
+	unsigned int numsizes = 0;
 
 	list_for_each_entry_safe(i, next, &(*expr)->expressions, list) {
 		unsigned dsize_bytes;
+		unsigned int subtype_size;
 
-		if (expr_is_constant(*expr) && dtype && off == 0)
+		if (expr_is_constant(*expr) && dtype && off == 0) {
+			xfree(sizes);
 			return expr_binary_error(ctx->msgs, i, *expr,
 						 "unexpected concat component, "
 						 "expecting %s",
 						 dtype->desc);
-
+		}
 		if (dtype == NULL)
 			tmp = datatype_lookup(TYPE_INVALID);
 		else
 			tmp = concat_subtype_lookup(type, --off);
-		expr_set_context(&ctx->ectx, tmp, tmp->size);
-
-		if (eval && list_member_evaluate(ctx, &i) < 0)
+		subtype_size = tmp->size;
+		if (subtype_size == 0 && dtype && dtype->subtypes > 0)
+			subtype_size = dtype->subsizes[off];
+		else if (subtype_size == 0)
+			subtype_size = i->len;
+		expr_set_context(&ctx->ectx, tmp, subtype_size);
+
+		if (eval && list_member_evaluate(ctx, &i) < 0) {
+			xfree(sizes);
 			return -1;
+		}
 		flags &= i->flags;
 
-		if (dtype == NULL && i->dtype->size == 0)
+		if (dtype == NULL && i->dtype->size == 0 && i->len == 0) {
+			xfree(sizes);
 			return expr_binary_error(ctx->msgs, i, *expr,
 						 "can not use variable sized "
 						 "data types (%s) in concat "
 						 "expressions",
 						 i->dtype->name);
+		}
 
 		ntype = concat_subtype_add(ntype, i->dtype->type);
 
-		dsize_bytes = div_round_up(i->dtype->size, BITS_PER_BYTE);
+		if (i->dtype->size == 0) {
+			numsizes++;
+			sizes = xrealloc(sizes, numsizes * sizeof(sizes[0]));
+			sizes[numsizes - 1] = i->len;
+			dsize_bytes = div_round_up(i->len, BITS_PER_BYTE);
+		} else {
+			dsize_bytes = div_round_up(i->dtype->size, BITS_PER_BYTE);
+		}
 		(*expr)->field_len[(*expr)->field_count++] = dsize_bytes;
 	}
 
 	(*expr)->flags |= flags;
-	datatype_set(*expr, concat_type_alloc(ntype));
+	datatype_set(*expr, concat_type_alloc(ntype, numsizes, sizes));
 	(*expr)->len   = (*expr)->dtype->size;
+	xfree(sizes); sizes = NULL;
 
 	if (off > 0)
 		return expr_error(ctx->msgs, *expr,
@@ -2918,7 +2939,7 @@ static int stmt_evaluate_nat_map(struct eval_ctx *ctx, struct stmt *stmt)
 	default:
 		return -1;
 	}
-	dtype = concat_type_alloc((addr_type << TYPE_BITS) | TYPE_INET_SERVICE);
+	dtype = concat_type_alloc((addr_type << TYPE_BITS) | TYPE_INET_SERVICE, 0, NULL);
 
 	expr_set_context(&ctx->ectx, dtype, dtype->size);
 	if (expr_evaluate(ctx, &stmt->nat.addr))
diff --git a/src/expression.c b/src/expression.c
index a6bde70f..20d06374 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -909,7 +909,7 @@ static struct expr *concat_expr_parse_udata(const struct nftnl_udata *attr)
 	const struct datatype *dtype;
 	struct expr *concat_expr;
 	uint32_t dt = 0;
-	unsigned int i;
+	unsigned int i, numsizes = 0, *sizes = NULL;
 	int err;
 
 	err = nftnl_udata_parse(nftnl_udata_get(attr), nftnl_udata_len(attr),
@@ -949,19 +949,26 @@ static struct expr *concat_expr_parse_udata(const struct nftnl_udata *attr)
 
 		dt = concat_subtype_add(dt, expr->dtype->type);
 		compound_expr_add(concat_expr, expr);
+		if (expr->dtype->size == 0) {
+			numsizes++;
+			sizes = xrealloc(sizes, numsizes * sizeof(sizes[0]));
+			sizes[numsizes-1] = expr->len;
+		}
 	}
 
-	dtype = concat_type_alloc(dt);
+	dtype = concat_type_alloc(dt, numsizes, sizes);
 	if (!dtype)
 		goto err_free;
 
 	concat_expr->dtype = datatype_get(dtype);
 	concat_expr->len = dtype->size;
+	xfree(sizes);
 
 	return concat_expr;
 
 err_free:
 	expr_free(concat_expr);
+	xfree(sizes);
 	return NULL;
 }
 
diff --git a/src/netlink.c b/src/netlink.c
index bb014320..531819d0 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -630,7 +630,10 @@ static const struct datatype *dtype_map_from_kernel(enum nft_data_types type)
 		return &verdict_type;
 	default:
 		if (type & ~TYPE_MASK)
-			return concat_type_alloc(type);
+			// This won't work for dynamically sized types,
+			// but in case of typeof dtype_map_from_kernel
+			// won't be called.
+			return concat_type_alloc(type, 0, NULL);
 		return datatype_lookup(type);
 	}
 }
@@ -773,7 +776,12 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 	}
 
 	key = nftnl_set_get_u32(nls, NFTNL_SET_KEY_TYPE);
-	keytype = dtype_map_from_kernel(key);
+	if (typeof_expr_key && typeof_expr_key->dtype->type == key)
+		keytype = typeof_expr_key->dtype;
+	else
+		/* Parsing CONCAT type would possibly require expr context
+		 * information (integer len) */
+		keytype = dtype_map_from_kernel(key);
 	if (keytype == NULL) {
 		netlink_io_error(ctx, NULL, "Unknown data type in set key %u",
 				 key);
@@ -785,7 +793,12 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 		uint32_t data;
 
 		data = nftnl_set_get_u32(nls, NFTNL_SET_DATA_TYPE);
-		datatype = dtype_map_from_kernel(data);
+		if (typeof_expr_data && typeof_expr_data->dtype->type == data)
+			datatype = typeof_expr_data->dtype;
+		else
+			/* Parsing CONCAT type would possibly require expr
+			 * context information (integer len) */
+			datatype = dtype_map_from_kernel(data);
 		if (datatype == NULL) {
 			netlink_io_error(ctx, NULL,
 					 "Unknown data type in set key %u",
@@ -1001,9 +1014,21 @@ static struct expr *netlink_parse_concat_elem(const struct datatype *dtype,
 
 	concat = concat_expr_alloc(&data->location);
 	while (off > 0) {
+		unsigned int dsize_bytes;
 		subtype = concat_subtype_lookup(dtype->type, --off);
 
-		expr		= constant_expr_splice(data, subtype->size);
+		if (subtype->size != 0)
+			assert(subtype->size == dtype->subsizes[off]);
+		/**
+		 * for vlan id 3567, data looks like [ 0d ef 00 00 ... ]
+		 * constant_expr_splice will mask it with ff f0 00 00 ..., so expr will contain 0d e0
+		 * which is not what we need. vlan id (without concat) also puts 0d ef into its reg,
+		 * so the leading zero should be preserved.
+		 * This is all due to vlan id having 12 bits which is not a multiple of BITS_PER_BYTE
+		 * and the way expr_evaluate_integer handle these.
+		 */
+		dsize_bytes     = div_round_up(dtype->subsizes[off], BITS_PER_BYTE);
+		expr		= constant_expr_splice(data, BITS_PER_BYTE * dsize_bytes);
 		expr->dtype     = subtype;
 		expr->byteorder = subtype->byteorder;
 
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index f721d15c..3946ed5f 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2216,6 +2216,7 @@ 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;
+		unsigned int numsizes = 0, *sizes = NULL;
 
 		list_for_each_entry(i, &expr->expressions, list) {
 			if (type) {
@@ -2225,8 +2226,14 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 			expr_postprocess(ctx, &i);
 
 			ntype = concat_subtype_add(ntype, i->dtype->type);
+			if (i->dtype->size == 0) {
+				numsizes++;
+				sizes = xrealloc(sizes, numsizes * sizeof(sizes[0]));
+				sizes[numsizes-1] = i->len;
+			}
 		}
-		datatype_set(expr, concat_type_alloc(ntype));
+		datatype_set(expr, concat_type_alloc(ntype, numsizes, sizes));
+		xfree(sizes);
 		break;
 	}
 	case EXPR_UNARY:
diff --git a/tests/shell/testcases/sets/dumps/typeof_sets_0.nft b/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
index 565369fb..fb133987 100644
--- a/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
+++ b/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
@@ -14,6 +14,11 @@ table inet t {
 		elements = { 2, 3, 103 }
 	}
 
+	set s4 {
+		type integer . ipv4_addr
+		elements = { 0 . 13.239.0.0 }
+	}
+
 	chain c1 {
 		osf name @s1 accept
 	}
@@ -21,4 +26,8 @@ table inet t {
 	chain c2 {
 		vlan id @s2 accept
 	}
+
+	chain c4 {
+		ether type vlan @ll,112,16 & 4095 . ip saddr @s4 accept
+	}
 }
diff --git a/tests/shell/testcases/sets/typeof_sets_0 b/tests/shell/testcases/sets/typeof_sets_0
index 9b2712e5..7cfbd53a 100755
--- a/tests/shell/testcases/sets/typeof_sets_0
+++ b/tests/shell/testcases/sets/typeof_sets_0
@@ -20,6 +20,11 @@ EXPECTED="table inet t {
 		elements = { 2, 3, 103 }
 	}
 
+	set s4 {
+		typeof vlan id . ip saddr
+		elements = { 3567 . 1.2.3.4 }
+	}
+
 	chain c1 {
 		osf name @s1 accept
 	}
@@ -27,6 +32,10 @@ EXPECTED="table inet t {
 	chain c2 {
 		ether type vlan vlan id @s2 accept
 	}
+
+	chain c4 {
+		ether type vlan vlan id . ip saddr @s4 accept
+	}
 }"
 
 set -e
-- 
2.20.1

Re: [RFC] concat with dynamically sized fields like vlan id

[Pablo Neira Ayuso]

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

On Fri, May 01, 2020 at 10:59:15PM +0200, Michael Braun wrote:
> This enables commands like
> 
>  nft set bridge t s4 '{typeof vlan id . ip daddr; elements = { 3567 .
> 1.2.3.4 }; }'
> 
> Which would previously fail with
>   Error: can not use variable sized data types (integer) in concat
>   expressions

Now that typeof is in place, the integer_type can be set to 32-bits
(word size).

I would prefer to not expose the integer type definition to sets:

> +	set s4 {
> +		type integer . ipv4_addr
> +		elements = { 0 . 13.239.0.0 }
> +	}
> +

Users do not need to know that an 8-bit payload field is actually
aligned to 32-bits. Or that osf name is actually and 32-bit id number.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 379 bytes --]

diff --git a/src/datatype.c b/src/datatype.c
index 723ac649ea42..6a9b0239d37c 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -397,6 +397,7 @@ const struct datatype integer_type = {
 	.type		= TYPE_INTEGER,
 	.name		= "integer",
 	.desc		= "integer",
+	.size		= 4 * BITS_PER_BYTE,
 	.print		= integer_type_print,
 	.json		= integer_type_json,
 	.parse		= integer_type_parse,

Re: [RFC] concat with dynamically sized fields like vlan id

[michael-dev]

Am 04.05.2020 14:45, schrieb Pablo Neira Ayuso:
> On Fri, May 01, 2020 at 10:59:15PM +0200, Michael Braun wrote:
>> This enables commands like
>> 
>>  nft set bridge t s4 '{typeof vlan id . ip daddr; elements = { 3567 .
>> 1.2.3.4 }; }'
>> 
>> Which would previously fail with
>>   Error: can not use variable sized data types (integer) in concat
>>   expressions
> 
> Now that typeof is in place, the integer_type can be set to 32-bits
> (word size).

When just changing the datatype definition of integer_type to .size = 
32,
the resulting set elements are broken:

# ether type vlan @ll,112,16 & 4095 . ip daddr { 4095 . 1.0.0.1} accept
__set%d test-bridge 3 size 1
__set%d test-bridge 0
         element ff0f0000 01000001  : 0 [end]
bridge
   [ payload load 2b @ link header + 12 => reg 1 ]
   [ cmp eq reg 1 0x00000081 ]
   [ payload load 2b @ link header + 16 => reg 1 ]
   [ cmp eq reg 1 0x00000008 ]
   [ payload load 2b @ link header + 14 => reg 1 ]
   [ bitwise reg 1 = (reg=1 & 0x0000ff0f ) ^ 0x00000000 ]
   [ payload load 4b @ network header + 16 => reg 9 ]
   [ lookup reg 1 set __set%d ]
   [ immediate reg 0 accept ]

I also verified that such a rule really does not match
                 ether type vlan id . ip daddr { 501 . 141.24.44.2 } log 
prefix "trace "
                 vlan id 501 ip daddr 141.24.44.2 log prefix "trace2
Results in dmesg entries only for trace2.
Whereas it worked with my last version.

I cannot really find the place to fix this. Any hint?

> I would prefer to not expose the integer type definition to sets:
> 
>> +	set s4 {
>> +		type integer . ipv4_addr
>> +		elements = { 0 . 13.239.0.0 }
>> +	}
>> +
> 
> Users do not need to know that an 8-bit payload field is actually
> aligned to 32-bits. Or that osf name is actually and 32-bit id number.

That was already fixed with e10356a4 ("tests: dump generated use new nft 
tool").

Regards,
M. Braun

Re: [RFC] concat with dynamically sized fields like vlan id

[Florian Westphal]

Michael Braun <michael-dev@fami-braun.de> wrote:
> This enables commands like
> 
>  nft set bridge t s4 '{typeof vlan id . ip daddr; elements = { 3567 .
> 1.2.3.4 }; }'
> 
> Which would previously fail with
>   Error: can not use variable sized data types (integer) in concat
>   expressions

I've cleaned up the testcase added in this patch and applied it to git.