store expression instead of data type as set key

PATCH In review Florian Westphal 2 participants 8 messages Sep 19, 2017 – Sep 27, 2017 netfilter

Share & tools

Florian Westphal

author maintainer

2017-09-19 12:49 UTC #0 lore raw

cover store expression instead of data type as set key

This is a prerequisite to later support dynamic data types, e.g. string, as a set key. The string datatype (and integer) do not have a fixed size, instead the size is determined by the expression.

This keeps the expression all the way down to set construction. Furthermore, in case of concatenations we can use EXPR_CONCAT to allow the netlink linearization step to determine the original sizes of the keys components.

Both changes are preparation work, they add no feature.

Florian Westphal

author maintainer

2017-09-19 12:49 UTC #1 lore raw

PATCH 1/2 evaluate: prepare to store expr key rather than datatype

currently set definitions store a datatype rather than an expression.

In order to support use of unqualified data types (string in particular), this prepares implicit set definition helper to expect an expression instead of plain data type. This also has the advantage that we can use EXPR_CONCAT to retain the original expressions when key concatentation is used, e.g. 'meta iifname . tcp dport'. The netlink serialization code can use this info to store individual key lengths independently of data types.

Would also allow later on to store the original names of the expressions, e.g. "ip daddr", in the kernel to support a future typeof keyword, e.g. 'type typeof(ip daddr)' instead of 'type ipv4_addr'.

Signed-off-by Florian Westphal <[email protected]>

---
 src/evaluate.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

src/evaluate.c +23 ~~-19~~

index e767542a868e..86159cf55f94 100644
@@ -62,9 +62,7 @@ static int __fmtstring(3, 4) set_error(struct eval_ctx *ctx,
 
 static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
 					     const char *name,
-					     const struct datatype *keytype,
-					     unsigned int keylen,
-					     unsigned int keybyteorder,
+					     struct expr *key,
 					     struct expr *expr)
 {
 	struct cmd *cmd;

@@ -74,8 +72,8 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
 	set = set_alloc(&expr->location);
 	set->flags	= NFT_SET_ANONYMOUS | expr->set_flags;
 	set->handle.set = xstrdup(name),
-	set->keytype 	= set_datatype_alloc(keytype, keybyteorder);
-	set->keylen	= keylen;
+	set->keytype 	= set_datatype_alloc(key->dtype, key->byteorder);
+	set->keylen	= key->len;
 	set->init	= expr;
 
 	if (ctx->table != NULL)

@@ -1184,6 +1182,7 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 {
 	struct expr_ctx ectx = ctx->ectx;
 	struct expr *map = *expr, *mappings;
+	struct expr *key;
 
 	expr_set_context(&ctx->ectx, NULL, 0);
 	if (expr_evaluate(ctx, &map->map) < 0)

@@ -1197,11 +1196,15 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 
 	switch (map->mappings->ops->type) {
 	case EXPR_SET:
+		key = constant_expr_alloc(&map->location,
+				 ctx->ectx.dtype,
+				 ctx->ectx.byteorder,
+				 ctx->ectx.len, NULL);
+
 		mappings = implicit_set_declaration(ctx, "__map%d",
-						    ctx->ectx.dtype,
-						    ctx->ectx.len,
-						    ctx->ectx.byteorder,
+						    key,
 						    mappings);
+		expr_free(key);
 
 		mappings->set->datatype = set_datatype_alloc(ectx.dtype,
 							     ectx.byteorder);

@@ -1539,8 +1542,7 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		if (right->ops->type == EXPR_SET)
 			right = rel->right =
 				implicit_set_declaration(ctx, "__set%d",
-							 left->dtype, left->len,
-							 left->byteorder, right);
+							 left, right);
 		else if (!datatype_equal(left->dtype, right->dtype))
 			return expr_binary_error(ctx->msgs, right, left,
 						 "datatype mismatch, expected %s, "

@@ -1597,9 +1599,7 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		case EXPR_SET:
 			assert(rel->op == OP_NEQ);
 			right = rel->right =
-				implicit_set_declaration(ctx, "__set%d",
-							 left->dtype, left->len,
-							 left->byteorder, right);
+				implicit_set_declaration(ctx, "__set%d", left, right);
 			/* fall through */
 		case EXPR_SET_REF:
 			assert(rel->op == OP_NEQ);

@@ -1978,8 +1978,7 @@ static int stmt_evaluate_flow(struct eval_ctx *ctx, struct stmt *stmt)
 		set->set_flags |= NFT_SET_TIMEOUT;
 
 	setref = implicit_set_declaration(ctx, stmt->flow.table ?: "__ft%d",
-					  key->dtype, key->len,
-					  key->dtype->byteorder, set);
+					  key, set);
 
 	stmt->flow.set = setref;

@@ -2609,6 +2608,7 @@ static int stmt_evaluate_objref_map(struct eval_ctx *ctx, struct stmt *stmt)
 {
 	struct expr *map = stmt->objref.expr;
 	struct expr *mappings;
+	struct expr *key;
 
 	expr_set_context(&ctx->ectx, NULL, 0);
 	if (expr_evaluate(ctx, &map->map) < 0)

@@ -2622,11 +2622,15 @@ static int stmt_evaluate_objref_map(struct eval_ctx *ctx, struct stmt *stmt)
 
 	switch (map->mappings->ops->type) {
 	case EXPR_SET:
+		key = constant_expr_alloc(&stmt->location,
+					  ctx->ectx.dtype,
+					  ctx->ectx.byteorder,
+					  ctx->ectx.len, NULL);
+
 		mappings = implicit_set_declaration(ctx, "__objmap%d",
-						    ctx->ectx.dtype,
-						    ctx->ectx.len,
-						    ctx->ectx.byteorder,
-						    mappings);
+						    key, mappings);
+		expr_free(key);
+
 		mappings->set->datatype = &string_type;
 		mappings->set->datalen  = NFT_OBJ_MAXNAMELEN * BITS_PER_BYTE;
 		mappings->set->objtype  = stmt->objref.type;

Signature · 1 line

2.13.5

Pablo Neira Ayuso

maintainer Acked-by on v1 1/2

2017-09-27 12:19 UTC #2 lore raw

Re: [PATCH nft 1/2] evaluate: prepare to store expr key rather than datatype

Quoted 10 lines · Florian Westphal

currently set definitions store a datatype rather than an expression.
In order to support use of unqualified data types (string in particular), this prepares implicit set definition helper to expect an expression instead of plain data type. This also has the advantage that we can use EXPR_CONCAT to retain the original expressions when key concatentation is used, e.g. 'meta iifname . tcp dport'. The netlink serialization code can use this info to store individual key lengths independently of data types.

Quoted 5 lines

Would also allow later on to store the original names of the expressions, e.g. "ip daddr", in the kernel to support a future typeof keyword, e.g. 'type typeof(ip daddr)' instead of 'type ipv4_addr'.
Signed-off-by Florian Westphal <[email protected]>

Acked-by Pablo Neira Ayuso <[email protected]>

Florian Westphal

author maintainer

2017-09-19 12:49 UTC #3 lore raw

PATCH 2/2 src: store expression as set key instead of data type

In reply to Florian Westphal

Doing so retains legth information in case of unqualified data types, e.g. we now have 'meta iifname' expression instead of an (unqualified) string type.

This allows to eventually use iifnames as set keys without adding yet another special data type for them.

Signed-off-by Florian Westphal <[email protected]>

---
 include/rule.h            |  6 ++---
 src/evaluate.c            | 69 ++++++++++++++++++++++++++++-------------------
 src/expression.c          |  4 +--
 src/netlink.c             | 27 ++++++++++---------
 src/netlink_delinearize.c | 12 ++++-----
 src/parser_bison.y        | 65 ++++++++++++++++++++++----------------------
 src/rule.c                |  4 +--
 src/segtree.c             |  4 +--
 8 files changed, 104 insertions(+), 87 deletions(-)

include/rule.h +2 -4

index 631a1bcdf84e..48b3d731b8a1 100644
@@ -212,8 +212,7 @@ extern struct rule *rule_lookup(const struct chain *chain, uint64_t handle);
  * @flags:	bitmask of set flags
  * @gc_int:	garbage collection interval
  * @timeout:	default timeout value
- * @keytype:	key data type
- * @keylen:	key length
+ * @key:	key expression (data type, length))
  * @datatype:	mapping data type
  * @datalen:	mapping data len
  * @objtype:	mapping object type

@@ -230,8 +229,7 @@ struct set {
 	uint32_t		flags;
 	uint32_t		gc_int;
 	uint64_t		timeout;
-	const struct datatype	*keytype;
-	unsigned int		keylen;
+	struct expr		*key;
 	const struct datatype	*datatype;
 	unsigned int		datalen;
 	uint32_t		objtype;

src/evaluate.c +42 ~~-27~~

163 lines not on this page · raw message

src/expression.c +2 -2

index d41ada39cc0f..ff3550c7cd85 100644
@@ -832,7 +832,7 @@ struct expr *set_expr_alloc(const struct location *loc, const struct set *set)
 		return set_expr;
 
 	set_expr->set_flags = set->flags;
-	set_expr->dtype = set->keytype;
+	set_expr->dtype = set->key->dtype;
 
 	return set_expr;
 }

@@ -960,7 +960,7 @@ struct expr *set_ref_expr_alloc(const struct location *loc, struct set *set)
 {
 	struct expr *expr;
 
-	expr = expr_alloc(loc, &set_ref_expr_ops, set->keytype, 0, 0);
+	expr = expr_alloc(loc, &set_ref_expr_ops, set->key->dtype, 0, 0);
 	expr->set = set_get(set);
 	expr->flags |= EXPR_F_CONSTANT;
 	return expr;

src/netlink.c +15 ~~-12~~

index 291bbdeeaa68..e414718ba1b9 100644
@@ -1118,8 +1118,11 @@ static struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 	set->handle.table  = xstrdup(nftnl_set_get_str(nls, NFTNL_SET_TABLE));
 	set->handle.set    = xstrdup(nftnl_set_get_str(nls, NFTNL_SET_NAME));
 
-	set->keytype = set_datatype_alloc(keytype, keybyteorder);
-	set->keylen  = nftnl_set_get_u32(nls, NFTNL_SET_KEY_LEN) * BITS_PER_BYTE;
+	set->key     = constant_expr_alloc(&netlink_location,
+					   set_datatype_alloc(keytype, keybyteorder),
+					   keybyteorder,
+					   nftnl_set_get_u32(nls, NFTNL_SET_KEY_LEN) * BITS_PER_BYTE,
+					   NULL);
 	set->flags   = nftnl_set_get_u32(nls, NFTNL_SET_FLAGS);
 
 	set->objtype = objtype;

@@ -1158,9 +1161,9 @@ static int netlink_add_set_compat(struct netlink_ctx *ctx,
 	nls = alloc_nftnl_set(h);
 	nftnl_set_set_u32(nls, NFTNL_SET_FLAGS, set->flags);
 	nftnl_set_set_u32(nls, NFTNL_SET_KEY_TYPE,
-			  dtype_map_to_kernel(set->keytype));
+			  dtype_map_to_kernel(set->key->dtype));
 	nftnl_set_set_u32(nls, NFTNL_SET_KEY_LEN,
-			  div_round_up(set->keylen, BITS_PER_BYTE));
+			  div_round_up(set->key->len, BITS_PER_BYTE));
 	if (set->flags & NFT_SET_MAP) {
 		nftnl_set_set_u32(nls, NFTNL_SET_DATA_TYPE,
 				  dtype_map_to_kernel(set->datatype));

@@ -1192,9 +1195,9 @@ static int netlink_add_set_batch(struct netlink_ctx *ctx,
 	nls = alloc_nftnl_set(h);
 	nftnl_set_set_u32(nls, NFTNL_SET_FLAGS, set->flags);
 	nftnl_set_set_u32(nls, NFTNL_SET_KEY_TYPE,
-			  dtype_map_to_kernel(set->keytype));
+			  dtype_map_to_kernel(set->key->dtype));
 	nftnl_set_set_u32(nls, NFTNL_SET_KEY_LEN,
-			  div_round_up(set->keylen, BITS_PER_BYTE));
+			  div_round_up(set->key->len, BITS_PER_BYTE));
 	if (set->flags & NFT_SET_MAP) {
 		nftnl_set_set_u32(nls, NFTNL_SET_DATA_TYPE,
 				  dtype_map_to_kernel(set->datatype));

@@ -1226,7 +1229,7 @@ static int netlink_add_set_batch(struct netlink_ctx *ctx,
 	if (!udbuf)
 		memory_allocation_error();
 	if (!nftnl_udata_put_u32(udbuf, UDATA_SET_KEYBYTEORDER,
-				 set->keytype->byteorder))
+				 set->key->byteorder))
 		memory_allocation_error();
 
 	if (set->flags & NFT_SET_MAP &&

@@ -1537,10 +1540,10 @@ static int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
 		flags = nftnl_set_elem_get_u32(nlse, NFTNL_SET_ELEM_FLAGS);
 
 	key = netlink_alloc_value(&netlink_location, &nld);
-	key->dtype	= set->keytype;
-	key->byteorder	= set->keytype->byteorder;
-	if (set->keytype->subtypes)
-		key = netlink_parse_concat_elem(set->keytype, key);
+	key->dtype	= set->key->dtype;
+	key->byteorder	= set->key->byteorder;
+	if (set->key->dtype->subtypes)
+		key = netlink_parse_concat_elem(set->key->dtype, key);
 
 	if (!(set->flags & NFT_SET_INTERVAL) &&
 	    key->byteorder == BYTEORDER_HOST_ENDIAN)

@@ -2197,7 +2200,7 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 		 * used by named sets, so use a dummy set.
 		 */
 		dummyset = set_alloc(monh->loc);
-		dummyset->keytype = set->keytype;
+		dummyset->key = expr_clone(set->key);
 		dummyset->datatype = set->datatype;
 		dummyset->flags = set->flags;
 		dummyset->init = set_expr_alloc(monh->loc, set);

src/netlink_delinearize.c +6 -6

index 3f42d092d204..42206ebcfff2 100644
@@ -307,8 +307,8 @@ static void netlink_parse_lookup(struct netlink_parse_ctx *ctx,
 		return netlink_error(ctx, loc,
 				     "Lookup expression has no left hand side");
 
-	if (left->len < set->keylen) {
-		left = netlink_parse_concat_expr(ctx, loc, sreg, set->keylen);
+	if (left->len < set->key->len) {
+		left = netlink_parse_concat_expr(ctx, loc, sreg, set->key->len);
 		if (left == NULL)
 			return;
 	}

@@ -1122,8 +1122,8 @@ static void netlink_parse_dynset(struct netlink_parse_ctx *ctx,
 		return netlink_error(ctx, loc,
 				     "Dynset statement has no key expression");
 
-	if (expr->len < set->keylen) {
-		expr = netlink_parse_concat_expr(ctx, loc, sreg, set->keylen);
+	if (expr->len < set->key->len) {
+		expr = netlink_parse_concat_expr(ctx, loc, sreg, set->key->len);
 		if (expr == NULL)
 			return;
 	}

@@ -1193,8 +1193,8 @@ static void netlink_parse_objref(struct netlink_parse_ctx *ctx,
 			return netlink_error(ctx, loc,
 					     "objref expression has no left hand side");
 
-		if (left->len < set->keylen) {
-			left = netlink_parse_concat_expr(ctx, loc, sreg, set->keylen);
+		if (left->len < set->key->len) {
+			left = netlink_parse_concat_expr(ctx, loc, sreg, set->key->len);
 			if (left == NULL)
 				return;
 		}

src/parser_bison.y +33 ~~-32~~

114 lines not on this page · raw message

src/rule.c +2 -2

index 1bb7b4756171..a417c307dfe8 100644
@@ -215,7 +215,7 @@ void set_free(struct set *set)
 	if (set->init != NULL)
 		expr_free(set->init);
 	handle_free(&set->handle);
-	set_datatype_destroy(set->keytype);
+	expr_free(set->key);
 	set_datatype_destroy(set->datatype);
 	xfree(set);
 }

@@ -296,7 +296,7 @@ static void set_print_declaration(const struct set *set,
 
 	printf(" %s {%s", set->handle.set, opts->nl);
 
-	printf("%s%stype %s", opts->tab, opts->tab, set->keytype->name);
+	printf("%s%stype %s", opts->tab, opts->tab, set->key->dtype->name);
 	if (set->flags & NFT_SET_MAP)
 		printf(" : %s", set->datatype->name);
 	else if (set->flags & NFT_SET_OBJECT)

src/segtree.c +2 -2

index f81e117421a1..f0efd155f0be 100644
@@ -76,8 +76,8 @@ static void seg_tree_init(struct seg_tree *tree, const struct set *set,
 
 	first = list_entry(init->expressions.next, struct expr, list);
 	tree->root	= RB_ROOT;
-	tree->keytype	= set->keytype;
-	tree->keylen	= set->keylen;
+	tree->keytype	= set->key->dtype;
+	tree->keylen	= set->key->len;
 	tree->datatype	= set->datatype;
 	tree->datalen	= set->datalen;
 	tree->byteorder	= first->byteorder;

Signature · 1 line

2.13.5

Pablo Neira Ayuso

maintainer

2017-09-27 12:58 UTC #4 lore raw

Re: [PATCH nft 2/2] src: store expression as set key instead of data type

src/evaluate.c static int list_member_evaluate(struct eval_ctx *ctx, struct expr **expr) · line 1040

Quoted 7 lines · Florian Westphal

src/evaluate.c +2 -1

@@ -1023,7 +1036,8 @@ static int list_member_evaluate(struct eval_ctx *ctx, struct expr **expr)
 	return err;
 }
 
-static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
+static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr,
+				bool evaluate)

Hm, why this boolean as parameter?

Quoted 12 lines

src/evaluate.c +1 -1

 {
 	const struct datatype *dtype = ctx->ectx.dtype, *tmp;
 	uint32_t type = dtype ? dtype->type : 0, ntype = 0;

@@ -1044,7 +1058,7 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
 			tmp = concat_subtype_lookup(type, --off);
 		expr_set_context(&ctx->ectx, tmp, tmp->size);
 
-		if (list_member_evaluate(ctx, &i) < 0)
+		if (evaluate && list_member_evaluate(ctx, &i) < 0)
 			return -1;
 		flags &= i->flags;

Florian Westphal

author maintainer

2017-09-27 13:05 UTC #5 lore raw

Re: [PATCH nft 2/2] src: store expression as set key instead of data type

Quoted 11 lines · Pablo Neira Ayuso

src/evaluate.c static int list_member_evaluate(struct eval_ctx *ctx, struct expr **expr) · line 1040
Quoted 7 lines · Florian Westphal
src/evaluate.c +2 -1
@@ -1023,7 +1036,8 @@ static int list_member_evaluate(struct eval_ctx *ctx, struct expr **expr)
 	return err;
 }
 
-static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
+static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr,
+				bool evaluate)
Hm, why this boolean as parameter?

Quoted 3 lines

src/evaluate.c static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr) · line 1062
Quoted 3 lines
src/evaluate.c +1 -1
-		if (list_member_evaluate(ctx, &i) < 0)
+		if (evaluate && list_member_evaluate(ctx, &i) < 0)
 			return -1;

We choke here because payload expressions don't have a base.

Hence this gets supressed in case we evaluate key.

Florian Westphal

author maintainer

2017-09-27 13:16 UTC #6 lore raw

Re: [PATCH nft 2/2] src: store expression as set key instead of data type

Quoted 12 lines · Florian Westphal

Quoted 10 lines · Pablo Neira Ayuso
src/evaluate.c static int list_member_evaluate(struct eval_ctx *ctx, struct expr **expr) · line 1040
Quoted 7 lines · Florian Westphal
src/evaluate.c +2 -1
@@ -1023,7 +1036,8 @@ static int list_member_evaluate(struct eval_ctx *ctx, struct expr **expr)
 	return err;
 }
 
-static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
+static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr,
+				bool evaluate)
Hm, why this boolean as parameter?

Quoted 7 lines

Quoted 3 lines
src/evaluate.c static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr) · line 1062
Quoted 3 lines
src/evaluate.c +1 -1
-		if (list_member_evaluate(ctx, &i) < 0)
+		if (evaluate && list_member_evaluate(ctx, &i) < 0)
 			return -1;
We choke here because payload expressions don't have a base.
Hence this gets supressed in case we evaluate key.

Ah wait, you are right, its not needed at the moment because we don't have 'typeof' keyword yet.

I'll push the patch without this.

Pablo Neira Ayuso

maintainer Acked-by

2017-09-27 13:59 UTC #7 lore raw

Re: [PATCH nft 2/2] src: store expression as set key instead of data type

Quoted 19 lines · Florian Westphal

Quoted 18 lines · Florian Westphal
Quoted 11 lines · Pablo Neira Ayuso
src/evaluate.c static int list_member_evaluate(struct eval_ctx *ctx, struct expr **expr) · line 1040
Quoted 7 lines · Florian Westphal
src/evaluate.c +2 -1
@@ -1023,7 +1036,8 @@ static int list_member_evaluate(struct eval_ctx *ctx, struct expr **expr)
 	return err;
 }
 
-static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
+static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr,
+				bool evaluate)
Hm, why this boolean as parameter?
Quoted 3 lines
src/evaluate.c static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr) · line 1062
Quoted 3 lines
src/evaluate.c +1 -1
-		if (list_member_evaluate(ctx, &i) < 0)
+		if (evaluate && list_member_evaluate(ctx, &i) < 0)
 			return -1;
We choke here because payload expressions don't have a base.

Quoted 6 lines

Quoted 1 line
Hence this gets supressed in case we evaluate key.
Ah wait, you are right, its not needed at the moment because we don't have 'typeof' keyword yet.
I'll push the patch without this.

Ah, that explains why, thanks.

Acked-by Pablo Neira Ayuso <[email protected]>