diff mbox series

[nft,v3] src: Support maps as left side expressions

Message ID 20191130135321.5188-1-phil@nwl.cc
State Under Review
Delegated to: Pablo Neira
Headers show
Series [nft,v3] src: Support maps as left side expressions | expand

Commit Message

Phil Sutter Nov. 30, 2019, 1:53 p.m. UTC
From: Brett Mastbergen <bmastbergen@untangle.com>

This change allows map expressions on the left side of comparisons:

nft add rule foo bar ip saddr map @map_a == 22 counter

It also allows map expressions as the left side expression of other
map expressions:

nft add rule foo bar ip saddr map @map_a map @map_b == 22 counter

To accomplish this, some additional context needs to be set during
evaluation and delinearization.  A tweak is also make to the parser
logic to allow map expressions as the left hand expression to other
map expressions.

By allowing maps as left side comparison expressions one can map
information in the packet to some arbitrary piece of data and use
the equality (or inequality) to make some decision about the traffic,
unlike today where the result of a map lookup is only usable as the
right side of a statement (like dnat or snat) that actually uses the
value as input.

Signed-off-by: Brett Mastbergen <bmastbergen@untangle.com>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
v2: Add testcases
v3: Support maps on vmap LHS

Pablo, Brett,

Reading the discussion about pending v2 of this patch I gave it a try
and extending functionality to verdict maps was easy and
straightforward, so I just went ahead and extended the code accordingly.

Cheers, Phil
---
 src/evaluate.c                                    |  2 +-
 src/expression.c                                  | 12 +++++++++++-
 src/netlink_delinearize.c                         |  2 ++
 src/parser_bison.y                                | 15 +++++++++++----
 .../testcases/maps/dumps/left_side_map_0.nft      | 10 ++++++++++
 tests/shell/testcases/maps/dumps/map_to_map_0.nft | 14 ++++++++++++++
 .../shell/testcases/maps/dumps/map_to_vmap_0.nft  | 14 ++++++++++++++
 tests/shell/testcases/maps/left_side_map_0        |  8 ++++++++
 tests/shell/testcases/maps/map_to_map_0           |  9 +++++++++
 tests/shell/testcases/maps/map_to_vmap_0          |  9 +++++++++
 10 files changed, 89 insertions(+), 6 deletions(-)
 create mode 100644 tests/shell/testcases/maps/dumps/left_side_map_0.nft
 create mode 100644 tests/shell/testcases/maps/dumps/map_to_map_0.nft
 create mode 100644 tests/shell/testcases/maps/dumps/map_to_vmap_0.nft
 create mode 100755 tests/shell/testcases/maps/left_side_map_0
 create mode 100755 tests/shell/testcases/maps/map_to_map_0
 create mode 100755 tests/shell/testcases/maps/map_to_vmap_0

Comments

Pablo Neira Ayuso Dec. 2, 2019, 7:27 p.m. UTC | #1
On Sat, Nov 30, 2019 at 02:53:21PM +0100, Phil Sutter wrote:
> From: Brett Mastbergen <bmastbergen@untangle.com>
> 
> This change allows map expressions on the left side of comparisons:
> 
> nft add rule foo bar ip saddr map @map_a == 22 counter
> 
> It also allows map expressions as the left side expression of other
> map expressions:
> 
> nft add rule foo bar ip saddr map @map_a map @map_b == 22 counter
> 
> To accomplish this, some additional context needs to be set during
> evaluation and delinearization.  A tweak is also make to the parser
> logic to allow map expressions as the left hand expression to other
> map expressions.
> 
> By allowing maps as left side comparison expressions one can map
> information in the packet to some arbitrary piece of data and use
> the equality (or inequality) to make some decision about the traffic,
> unlike today where the result of a map lookup is only usable as the
> right side of a statement (like dnat or snat) that actually uses the
> value as input.
> 
> Signed-off-by: Brett Mastbergen <bmastbergen@untangle.com>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> v2: Add testcases
> v3: Support maps on vmap LHS
> 
> Pablo, Brett,
> 
> Reading the discussion about pending v2 of this patch I gave it a try
> and extending functionality to verdict maps was easy and
> straightforward, so I just went ahead and extended the code accordingly.

Nice, thanks a lot for following up on this.

There is still one more case we have to cover and we go:

$ sudo nft add rule x z ip saddr map { 1.1.1.1 : 22 } vmap { 22 : accept }
Error: No symbol type information
add rule x z ip saddr map { 1.1.1.1 : 22 } vmap { 22 : accept }
                                      ^^

The existing syntax does not give us a chance to specify the right
hand side datatype. For named sets, this is fine because the set
reference implicit gives us the datatype. This information is not
available on implicit sets.

I think we have to update the syntax to include this information. For
the named set case, this will allow us to validate that actually the
specified selector and the set datatype are matching. For the implicit
sets, this will give us the datatype for the right hand side.

Otherwise, I'm afraid we will have to deal with variable length
integer, which will make things more complicated. At this stage, I
would start with strict typing, later on we can probably relax this in
the future as we find a way to reasonable infer information. This
means that a more verbose syntax, but I think that is fine.
diff mbox series

Patch

diff --git a/src/evaluate.c b/src/evaluate.c
index a865902c0fc7b..832af56a32a5b 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1415,6 +1415,7 @@  static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 		    !set_is_datamap(map->mappings->set->flags))
 			return expr_error(ctx->msgs, map->mappings,
 					  "Expression is not a map");
+		expr_set_context(&ctx->ectx, map->mappings->set->datatype, map->mappings->set->datalen);
 		break;
 	default:
 		BUG("invalid mapping expression %s\n",
@@ -1429,7 +1430,6 @@  static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 					 map->map->dtype->desc);
 
 	datatype_set(map, map->mappings->set->datatype);
-	map->flags |= EXPR_F_CONSTANT;
 
 	/* Data for range lookups needs to be in big endian order */
 	if (map->mappings->set->flags & NFT_SET_INTERVAL &&
diff --git a/src/expression.c b/src/expression.c
index 5070b1014392c..b3ff4ff1e1edb 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -1081,8 +1081,18 @@  static const struct expr_ops set_ref_expr_ops = {
 struct expr *set_ref_expr_alloc(const struct location *loc, struct set *set)
 {
 	struct expr *expr;
+	const struct datatype *dtype;
+	unsigned int len;
 
-	expr = expr_alloc(loc, EXPR_SET_REF, set->key->dtype, 0, 0);
+	if (set->flags & NFT_SET_MAP) {
+		dtype = set->datatype;
+		len = set->datalen;
+	} else {
+		dtype = set->key->dtype;
+		len = set->key->len;
+	}
+
+	expr = expr_alloc(loc, EXPR_SET_REF, dtype, 0, len);
 	expr->set = set_get(set);
 	expr->flags |= EXPR_F_CONSTANT;
 	return expr;
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 154353b8161a0..a972adc2d4cdc 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -341,6 +341,8 @@  static void netlink_parse_lookup(struct netlink_parse_ctx *ctx,
 	if (nftnl_expr_is_set(nle, NFTNL_EXPR_LOOKUP_DREG)) {
 		dreg = netlink_parse_register(nle, NFTNL_EXPR_LOOKUP_DREG);
 		expr = map_expr_alloc(loc, left, right);
+		expr_set_type(expr, right->dtype, right->byteorder);
+		expr->len = right->len;
 		if (dreg != NFT_REG_VERDICT)
 			return netlink_set_register(ctx, dreg, expr);
 	} else {
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 707f46716ed3f..ad5d5911383b0 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -671,8 +671,8 @@  int nft_lex(void *, void *, void *);
 %type <expr>			concat_expr
 %destructor { expr_free($$); }	concat_expr
 
-%type <expr>			map_expr
-%destructor { expr_free($$); }	map_expr
+%type <expr>			map_expr map_lhs_expr verdict_map_lhs_expr
+%destructor { expr_free($$); }	map_expr map_lhs_expr verdict_map_lhs_expr
 
 %type <expr>			verdict_map_stmt
 %destructor { expr_free($$); }	verdict_map_stmt
@@ -2430,7 +2430,10 @@  verdict_stmt		:	verdict_expr
 			}
 			;
 
-verdict_map_stmt	:	concat_expr	VMAP	verdict_map_expr
+verdict_map_lhs_expr	:	concat_expr
+			|	map_expr
+
+verdict_map_stmt	:	verdict_map_lhs_expr	VMAP	verdict_map_expr
 			{
 				$$ = map_expr_alloc(&@$, $1, $3);
 			}
@@ -3559,7 +3562,11 @@  multiton_rhs_expr	:	prefix_rhs_expr
 			|	wildcard_expr
 			;
 
-map_expr		:	concat_expr	MAP	rhs_expr
+map_lhs_expr		:	concat_expr
+			|	map_expr
+			;
+
+map_expr		:	map_lhs_expr	MAP	rhs_expr
 			{
 				$$ = map_expr_alloc(&@$, $1, $3);
 			}
diff --git a/tests/shell/testcases/maps/dumps/left_side_map_0.nft b/tests/shell/testcases/maps/dumps/left_side_map_0.nft
new file mode 100644
index 0000000000000..b93948fc6561b
--- /dev/null
+++ b/tests/shell/testcases/maps/dumps/left_side_map_0.nft
@@ -0,0 +1,10 @@ 
+table ip x {
+	map y {
+		type ipv4_addr : inet_service
+	}
+
+	chain z {
+		type filter hook output priority filter; policy accept;
+		ip saddr map @y 22 counter packets 0 bytes 0
+	}
+}
diff --git a/tests/shell/testcases/maps/dumps/map_to_map_0.nft b/tests/shell/testcases/maps/dumps/map_to_map_0.nft
new file mode 100644
index 0000000000000..fd7339f99b4ae
--- /dev/null
+++ b/tests/shell/testcases/maps/dumps/map_to_map_0.nft
@@ -0,0 +1,14 @@ 
+table ip x {
+	map y {
+		type ipv4_addr : mark
+	}
+
+	map yy {
+		type mark : inet_service
+	}
+
+	chain z {
+		type filter hook output priority filter; policy accept;
+		ip saddr map @y map @yy 22 counter packets 0 bytes 0
+	}
+}
diff --git a/tests/shell/testcases/maps/dumps/map_to_vmap_0.nft b/tests/shell/testcases/maps/dumps/map_to_vmap_0.nft
new file mode 100644
index 0000000000000..0802284dd8ef8
--- /dev/null
+++ b/tests/shell/testcases/maps/dumps/map_to_vmap_0.nft
@@ -0,0 +1,14 @@ 
+table ip x {
+	map y {
+		type ipv4_addr : mark
+	}
+
+	map yy {
+		type mark : verdict
+	}
+
+	chain z {
+		type filter hook output priority filter; policy accept;
+		ip saddr map @y vmap @yy
+	}
+}
diff --git a/tests/shell/testcases/maps/left_side_map_0 b/tests/shell/testcases/maps/left_side_map_0
new file mode 100755
index 0000000000000..93c721b72b361
--- /dev/null
+++ b/tests/shell/testcases/maps/left_side_map_0
@@ -0,0 +1,8 @@ 
+#!/bin/bash
+
+set -e
+
+$NFT add table x
+$NFT add map x y { type ipv4_addr : inet_service\; }
+$NFT add chain ip x z { type filter hook output priority 0 \; }
+$NFT add rule x z ip saddr map @y 22 counter
diff --git a/tests/shell/testcases/maps/map_to_map_0 b/tests/shell/testcases/maps/map_to_map_0
new file mode 100755
index 0000000000000..95a45f622173e
--- /dev/null
+++ b/tests/shell/testcases/maps/map_to_map_0
@@ -0,0 +1,9 @@ 
+#!/bin/bash
+
+set -e
+
+$NFT add table x
+$NFT add map x y { type ipv4_addr : mark\; }
+$NFT add map x yy { type mark : inet_service\; }
+$NFT add chain ip x z { type filter hook output priority 0 \; }
+$NFT add rule x z ip saddr map @y map@yy 22 counter
diff --git a/tests/shell/testcases/maps/map_to_vmap_0 b/tests/shell/testcases/maps/map_to_vmap_0
new file mode 100755
index 0000000000000..dc3aefaf66528
--- /dev/null
+++ b/tests/shell/testcases/maps/map_to_vmap_0
@@ -0,0 +1,9 @@ 
+#!/bin/bash
+
+set -e
+
+$NFT add table x
+$NFT add map x y { type ipv4_addr : mark\; }
+$NFT add map x yy { type mark : verdict\; }
+$NFT add chain ip x z { type filter hook output priority 0 \; }
+$NFT add rule x z ip saddr map @y vmap @yy