[nft PATCH v2 0/5] Store multiple payload dependencies

[nft PATCH v2 0/5] Store multiple payload dependencies

[Jeremy Sowden]

The purpose of this patch-set is to eliminate more redundant
payload-dependencies.

Here's the netlink dump for a test where such a dependency is not
eliminated.

  [ meta load nfproto => reg 1 ]
  [ cmp eq reg 1 0x00000002 ]
  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 4b @ network header + 12 => reg 1 ]
  [ payload load 4b @ network header + 16 => reg 9 ]
  [ payload load 2b @ transport header + 2 => reg 10 ]
  [ lookup reg 1 set set3 ]
  [ immediate reg 0 accept ]

The reason the `meta nfproto` match is not eliminated is that it is
overwritten in the dependency context by the `meta l4proto` match before
we get to the `ip saddr` and `ip daddr` expressions which would have
caused it to be eliminated.  By contrast, the `meta l4proto` match _is_
eliminated because it is still present in the context we get to the `tcp
dport` expression.  Therefore, this patch-set extends the payload-
dependency context to store not just a single dependency, but one per
protocol layer.

Patch 1 fixes a mistake in a Python test-case.  Patch 2 makes some
preliminary changes.  Patch 3 adds the extra dependencies.  Patches 4 &
5 remove redundant protocol matches which are now eliminated from
test-cases.

At the end of this series all tests pass.

Changes since v1.

  * The first seven v1 patches have been merged.  The remaining four
    form patches 2-5 in this series.
  * Patch 1 is new: it fixes a test-case that was supposed to be fixed
    by patch 2 in v1.
  * The helper added by patch 2 has been changed to return the
    expression from the payload dependency statement, not the statement
    itself.  The removal of the redundant `ctx->pbase` check is new.

Jeremy Sowden (5):
  tests: py: fix inet/ip.t bridge payload
  src: add a helper that returns a payload dependency for a particular
    base
  src: store more than one payload dependency
  tests: py: remove redundant payload expressions
  tests: shell: remove redundant payload expressions

 include/payload.h                             | 15 ++--
 src/netlink_delinearize.c                     | 18 +++--
 src/payload.c                                 | 72 ++++++++++++++-----
 tests/py/inet/icmpX.t                         |  2 +-
 tests/py/inet/icmpX.t.json.output             |  9 ---
 tests/py/inet/ip.t.payload.bridge             |  2 +-
 tests/py/inet/sets.t.json                     | 11 ---
 .../testcases/maps/dumps/0010concat_map_0.nft |  2 +-
 .../testcases/maps/dumps/nat_addr_port.nft    |  8 +--
 9 files changed, 79 insertions(+), 60 deletions(-)

-- 
2.34.1

[nft PATCH v2 1/5] tests: py: fix inet/ip.t bridge payload

[Jeremy Sowden]

Correct the statement used to load the protocol in the bridge payload
of one of the ip tests.

A previous commit was supposed, in part, to do this, but the update got
lost.

Fixes: 4b8e51ea5fc8 ("tests: py: fix inet/ip.t payloads")
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 tests/py/inet/ip.t.payload.bridge | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/py/inet/ip.t.payload.bridge b/tests/py/inet/ip.t.payload.bridge
index a422ed76c2de..57dbc9eb42e7 100644
--- a/tests/py/inet/ip.t.payload.bridge
+++ b/tests/py/inet/ip.t.payload.bridge
@@ -3,7 +3,7 @@ __set%d test-bridge 3
 __set%d test-bridge 0
 	element 01010101 02020202 fecafeca 0000feca  : 0 [end]
 bridge test-bridge input
-  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ meta load protocol => reg 1 ]
   [ cmp eq reg 1 0x00000008 ]
   [ payload load 4b @ network header + 12 => reg 1 ]
   [ payload load 4b @ network header + 16 => reg 9 ]
-- 
2.34.1

[nft PATCH v2 2/5] src: add a helper that returns a payload dependency for a particular base

[Jeremy Sowden]

Currently, with only one base and dependency stored this is superfluous,
but it will become more useful when the next commit adds support for
storing a payload for every base.

Remove redundant `ctx->pbase` check.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/payload.h         |  2 ++
 src/netlink_delinearize.c |  4 ++--
 src/payload.c             | 31 +++++++++++++++++++++++++++----
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/include/payload.h b/include/payload.h
index 8bc3fb9a8a54..af6fa4782706 100644
--- a/include/payload.h
+++ b/include/payload.h
@@ -47,6 +47,8 @@ extern void payload_dependency_store(struct payload_dep_ctx *ctx,
 				     enum proto_bases base);
 extern bool payload_dependency_exists(const struct payload_dep_ctx *ctx,
 				      enum proto_bases base);
+extern struct expr *payload_dependency_get(struct payload_dep_ctx *ctx,
+					   enum proto_bases base);
 extern void payload_dependency_release(struct payload_dep_ctx *ctx);
 extern void payload_dependency_kill(struct payload_dep_ctx *ctx,
 				    struct expr *expr, unsigned int family);
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index fd81e07151c2..5e474b321379 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2060,9 +2060,9 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
 				     const struct expr *expr)
 {
 	uint8_t l4proto, nfproto = NFPROTO_UNSPEC;
-	struct expr *dep = ctx->pdep->expr;
+	struct expr *dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR);
 
-	if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
+	if (!dep)
 		return true;
 
 	if (__meta_dependency_may_kill(dep, &nfproto))
diff --git a/src/payload.c b/src/payload.c
index 576eb149f71d..accbe0ab6066 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -631,6 +631,27 @@ bool payload_dependency_exists(const struct payload_dep_ctx *ctx,
 	       (ctx->pbase == base || (base == PROTO_BASE_TRANSPORT_HDR && ctx->pbase == base + 1));
 }
 
+/**
+ * payload_dependency_get - return a payload dependency if available
+ * @ctx: payload dependency context
+ * @base: payload protocol base
+ *
+ * If we have seen a protocol key payload expression for this base, we return
+ * it.
+ */
+struct expr *payload_dependency_get(struct payload_dep_ctx *ctx,
+				    enum proto_bases base)
+{
+	if (ctx->pbase == base)
+		return ctx->pdep->expr;
+
+	if (base == PROTO_BASE_TRANSPORT_HDR &&
+	    ctx->pbase == PROTO_BASE_INNER_HDR)
+		return ctx->pdep->expr;
+
+	return NULL;
+}
+
 void payload_dependency_release(struct payload_dep_ctx *ctx)
 {
 	list_del(&ctx->pdep->list);
@@ -661,7 +682,7 @@ static uint8_t icmp_dep_to_type(enum icmp_hdr_field_type t)
 
 static bool payload_may_dependency_kill_icmp(struct payload_dep_ctx *ctx, struct expr *expr)
 {
-	const struct expr *dep = ctx->pdep->expr;
+	const struct expr *dep = payload_dependency_get(ctx, expr->payload.base);
 	uint8_t icmp_type;
 
 	icmp_type = expr->payload.tmpl->icmp_dep;
@@ -678,9 +699,11 @@ static bool payload_may_dependency_kill_icmp(struct payload_dep_ctx *ctx, struct
 
 static bool payload_may_dependency_kill_ll(struct payload_dep_ctx *ctx, struct expr *expr)
 {
-	const struct expr *dep = ctx->pdep->expr;
+	const struct expr *dep = payload_dependency_get(ctx, expr->payload.base);
 
-	/* Never remove a 'vlan type 0x...' expression, they are never added implicitly */
+	/* Never remove a 'vlan type 0x...' expression, they are never added
+	 * implicitly
+	 */
 	if (dep->left->payload.desc == &proto_vlan)
 		return false;
 
@@ -697,7 +720,7 @@ static bool payload_may_dependency_kill_ll(struct payload_dep_ctx *ctx, struct e
 static bool payload_may_dependency_kill(struct payload_dep_ctx *ctx,
 					unsigned int family, struct expr *expr)
 {
-	struct expr *dep = ctx->pdep->expr;
+	struct expr *dep = payload_dependency_get(ctx, expr->payload.base);
 
 	/* Protocol key payload expression at network base such as 'ip6 nexthdr'
 	 * need to be left in place since it implicitly restricts matching to
-- 
2.34.1

[nft PATCH v2 3/5] src: store more than one payload dependency

[Jeremy Sowden]

Change the payload-dependency context to store a dependency for every
protocol layer.  This allows us to eliminate more redundant protocol
expressions.
---
 include/payload.h         | 13 +++++------
 src/netlink_delinearize.c | 14 +++++++----
 src/payload.c             | 49 ++++++++++++++++++++++++---------------
 3 files changed, 45 insertions(+), 31 deletions(-)

diff --git a/include/payload.h b/include/payload.h
index af6fa4782706..378699283c0a 100644
--- a/include/payload.h
+++ b/include/payload.h
@@ -25,16 +25,14 @@ extern int exthdr_gen_dependency(struct eval_ctx *ctx, const struct expr *expr,
 /**
  * struct payload_dep_ctx - payload protocol dependency tracking
  *
- * @pbase: protocol base of last dependency match
  * @icmp_type: extra info for icmp(6) decoding
- * @pdep: last dependency match
  * @prev: previous statement
+ * @pdeps: last dependency match per protocol layer
  */
 struct payload_dep_ctx {
-	enum proto_bases	pbase:8;
-	uint8_t			icmp_type;
-	struct stmt		*pdep;
-	struct stmt		*prev;
+	uint8_t		icmp_type;
+	struct stmt	*prev;
+	struct stmt	*pdeps[PROTO_BASE_MAX + 1];
 };
 
 extern bool payload_is_known(const struct expr *expr);
@@ -49,7 +47,8 @@ extern bool payload_dependency_exists(const struct payload_dep_ctx *ctx,
 				      enum proto_bases base);
 extern struct expr *payload_dependency_get(struct payload_dep_ctx *ctx,
 					   enum proto_bases base);
-extern void payload_dependency_release(struct payload_dep_ctx *ctx);
+extern void payload_dependency_release(struct payload_dep_ctx *ctx,
+				       enum proto_bases base);
 extern void payload_dependency_kill(struct payload_dep_ctx *ctx,
 				    struct expr *expr, unsigned int family);
 extern void exthdr_dependency_kill(struct payload_dep_ctx *ctx,
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 5e474b321379..86b7006aaa71 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2119,11 +2119,12 @@ static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx,
 
 		relational_expr_pctx_update(&ctx->pctx, expr);
 
-		if (ctx->pdctx.pbase < PROTO_BASE_TRANSPORT_HDR) {
+		if (base < PROTO_BASE_TRANSPORT_HDR) {
 			if (payload_dependency_exists(&ctx->pdctx, base) &&
 			    meta_may_dependency_kill(&ctx->pdctx,
 						     ctx->pctx.family, expr))
-				payload_dependency_release(&ctx->pdctx);
+				payload_dependency_release(&ctx->pdctx, base);
+
 			if (left->flags & EXPR_F_PROTOCOL)
 				payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
 		}
@@ -2653,7 +2654,8 @@ static void stmt_reject_postprocess(struct rule_pp_ctx *rctx)
 		if (stmt->reject.type == NFT_REJECT_TCP_RST &&
 		    payload_dependency_exists(&rctx->pdctx,
 					      PROTO_BASE_TRANSPORT_HDR))
-			payload_dependency_release(&rctx->pdctx);
+			payload_dependency_release(&rctx->pdctx,
+						   PROTO_BASE_TRANSPORT_HDR);
 		break;
 	case NFPROTO_IPV6:
 		stmt->reject.family = rctx->pctx.family;
@@ -2661,7 +2663,8 @@ static void stmt_reject_postprocess(struct rule_pp_ctx *rctx)
 		if (stmt->reject.type == NFT_REJECT_TCP_RST &&
 		    payload_dependency_exists(&rctx->pdctx,
 					      PROTO_BASE_TRANSPORT_HDR))
-			payload_dependency_release(&rctx->pdctx);
+			payload_dependency_release(&rctx->pdctx,
+						   PROTO_BASE_TRANSPORT_HDR);
 		break;
 	case NFPROTO_INET:
 	case NFPROTO_BRIDGE:
@@ -2695,7 +2698,8 @@ static void stmt_reject_postprocess(struct rule_pp_ctx *rctx)
 		}
 
 		if (payload_dependency_exists(&rctx->pdctx, PROTO_BASE_NETWORK_HDR))
-			payload_dependency_release(&rctx->pdctx);
+			payload_dependency_release(&rctx->pdctx,
+						   PROTO_BASE_NETWORK_HDR);
 		break;
 	default:
 		break;
diff --git a/src/payload.c b/src/payload.c
index accbe0ab6066..f433c38421a4 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -610,8 +610,7 @@ void payload_dependency_store(struct payload_dep_ctx *ctx,
 	if (ignore_dep)
 		return;
 
-	ctx->pdep  = stmt;
-	ctx->pbase = base + 1;
+	ctx->pdeps[base + 1] = stmt;
 }
 
 /**
@@ -626,9 +625,11 @@ void payload_dependency_store(struct payload_dep_ctx *ctx,
 bool payload_dependency_exists(const struct payload_dep_ctx *ctx,
 			       enum proto_bases base)
 {
-	return ctx->pbase != PROTO_BASE_INVALID &&
-	       ctx->pdep != NULL &&
-	       (ctx->pbase == base || (base == PROTO_BASE_TRANSPORT_HDR && ctx->pbase == base + 1));
+	if (ctx->pdeps[base])
+		return true;
+
+	return	base == PROTO_BASE_TRANSPORT_HDR &&
+		ctx->pdeps[PROTO_BASE_INNER_HDR];
 }
 
 /**
@@ -642,25 +643,35 @@ bool payload_dependency_exists(const struct payload_dep_ctx *ctx,
 struct expr *payload_dependency_get(struct payload_dep_ctx *ctx,
 				    enum proto_bases base)
 {
-	if (ctx->pbase == base)
-		return ctx->pdep->expr;
+	if (ctx->pdeps[base])
+		return ctx->pdeps[base]->expr;
 
 	if (base == PROTO_BASE_TRANSPORT_HDR &&
-	    ctx->pbase == PROTO_BASE_INNER_HDR)
-		return ctx->pdep->expr;
+	    ctx->pdeps[PROTO_BASE_INNER_HDR])
+		return ctx->pdeps[PROTO_BASE_INNER_HDR]->expr;
 
 	return NULL;
 }
 
-void payload_dependency_release(struct payload_dep_ctx *ctx)
+static void __payload_dependency_release(struct payload_dep_ctx *ctx,
+					 enum proto_bases base)
 {
-	list_del(&ctx->pdep->list);
-	stmt_free(ctx->pdep);
+	list_del(&ctx->pdeps[base]->list);
+	stmt_free(ctx->pdeps[base]);
 
-	ctx->pbase = PROTO_BASE_INVALID;
-	if (ctx->pdep == ctx->prev)
+	if (ctx->pdeps[base] == ctx->prev)
 		ctx->prev = NULL;
-	ctx->pdep  = NULL;
+	ctx->pdeps[base] = NULL;
+}
+
+void payload_dependency_release(struct payload_dep_ctx *ctx,
+				enum proto_bases base)
+{
+	if (ctx->pdeps[base])
+		__payload_dependency_release(ctx, base);
+	else if (base == PROTO_BASE_TRANSPORT_HDR &&
+		 ctx->pdeps[PROTO_BASE_INNER_HDR])
+		__payload_dependency_release(ctx, PROTO_BASE_INNER_HDR);
 }
 
 static uint8_t icmp_dep_to_type(enum icmp_hdr_field_type t)
@@ -786,7 +797,7 @@ void payload_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 {
 	if (payload_dependency_exists(ctx, expr->payload.base) &&
 	    payload_may_dependency_kill(ctx, family, expr))
-		payload_dependency_release(ctx);
+		payload_dependency_release(ctx, expr->payload.base);
 }
 
 void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
@@ -795,15 +806,15 @@ void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 	switch (expr->exthdr.op) {
 	case NFT_EXTHDR_OP_TCPOPT:
 		if (payload_dependency_exists(ctx, PROTO_BASE_TRANSPORT_HDR))
-			payload_dependency_release(ctx);
+			payload_dependency_release(ctx, PROTO_BASE_TRANSPORT_HDR);
 		break;
 	case NFT_EXTHDR_OP_IPV6:
 		if (payload_dependency_exists(ctx, PROTO_BASE_NETWORK_HDR))
-			payload_dependency_release(ctx);
+			payload_dependency_release(ctx, PROTO_BASE_NETWORK_HDR);
 		break;
 	case NFT_EXTHDR_OP_IPV4:
 		if (payload_dependency_exists(ctx, PROTO_BASE_NETWORK_HDR))
-			payload_dependency_release(ctx);
+			payload_dependency_release(ctx, PROTO_BASE_NETWORK_HDR);
 		break;
 	default:
 		break;
-- 
2.34.1

[nft PATCH v2 4/5] tests: py: remove redundant payload expressions

[Jeremy Sowden]

Now that we keep track of more payload dependencies, more redundant
payloads are eliminated.  Remove these from the Python test-cases.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 tests/py/inet/icmpX.t             |  2 +-
 tests/py/inet/icmpX.t.json.output |  9 ---------
 tests/py/inet/sets.t.json         | 11 -----------
 3 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/tests/py/inet/icmpX.t b/tests/py/inet/icmpX.t
index 97ff96d0cf0e..9430b3d3d579 100644
--- a/tests/py/inet/icmpX.t
+++ b/tests/py/inet/icmpX.t
@@ -7,4 +7,4 @@ icmp type echo-request;ok
 ip6 nexthdr icmpv6 icmpv6 type echo-request;ok;ip6 nexthdr 58 icmpv6 type echo-request
 icmpv6 type echo-request;ok
 # must not remove 'ip protocol' dependency, this explicitly matches icmpv6-in-ipv4.
-ip protocol ipv6-icmp meta l4proto ipv6-icmp icmpv6 type 1;ok;ip protocol 58 meta l4proto 58 icmpv6 type destination-unreachable
+ip protocol ipv6-icmp meta l4proto ipv6-icmp icmpv6 type 1;ok;ip protocol 58 icmpv6 type destination-unreachable
diff --git a/tests/py/inet/icmpX.t.json.output b/tests/py/inet/icmpX.t.json.output
index 9b0bf9f75ed5..7765cd908e24 100644
--- a/tests/py/inet/icmpX.t.json.output
+++ b/tests/py/inet/icmpX.t.json.output
@@ -68,15 +68,6 @@
             "right": 58
         }
     },
-    {
-        "match": {
-            "left": {
-                "meta": { "key": "l4proto" }
-            },
-	    "op": "==",
-            "right": 58
-        }
-    },
     {
         "match": {
             "left": {
diff --git a/tests/py/inet/sets.t.json b/tests/py/inet/sets.t.json
index ef0cedca8159..b44ffc20d70d 100644
--- a/tests/py/inet/sets.t.json
+++ b/tests/py/inet/sets.t.json
@@ -73,17 +73,6 @@
 
 # ip daddr . tcp dport { 10.0.0.0/8 . 10-23, 192.168.1.1-192.168.3.8 . 80-443 } accept
 [
-    {
-        "match": {
-            "left": {
-                "meta": {
-                    "key": "nfproto"
-                }
-            },
-            "op": "==",
-            "right": "ipv4"
-        }
-    },
     {
         "match": {
             "left": {
-- 
2.34.1

[nft PATCH v2 5/5] tests: shell: remove redundant payload expressions

[Jeremy Sowden]

Now that we keep track of more payload dependencies, more redundant
payloads are eliminated.  Remove these from the shell test-cases.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 tests/shell/testcases/maps/dumps/0010concat_map_0.nft | 2 +-
 tests/shell/testcases/maps/dumps/nat_addr_port.nft    | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/shell/testcases/maps/dumps/0010concat_map_0.nft b/tests/shell/testcases/maps/dumps/0010concat_map_0.nft
index b6bc338c55b7..2f796b51d46b 100644
--- a/tests/shell/testcases/maps/dumps/0010concat_map_0.nft
+++ b/tests/shell/testcases/maps/dumps/0010concat_map_0.nft
@@ -6,6 +6,6 @@ table inet x {
 
 	chain y {
 		type nat hook prerouting priority dstnat; policy accept;
-		meta nfproto ipv4 dnat ip to ip saddr . ip protocol . tcp dport map @z
+		dnat ip to ip saddr . ip protocol . tcp dport map @z
 	}
 }
diff --git a/tests/shell/testcases/maps/dumps/nat_addr_port.nft b/tests/shell/testcases/maps/dumps/nat_addr_port.nft
index cf6b957f0a9b..c8493b3adbf2 100644
--- a/tests/shell/testcases/maps/dumps/nat_addr_port.nft
+++ b/tests/shell/testcases/maps/dumps/nat_addr_port.nft
@@ -114,15 +114,15 @@ table inet inetfoo {
 		dnat ip to ip daddr map @x4
 		ip saddr 10.1.1.1 dnat ip to 10.2.3.4
 		ip saddr 10.1.1.2 tcp dport 42 dnat ip to 10.2.3.4:4242
-		meta l4proto tcp meta nfproto ipv4 dnat ip to ip saddr map @y4
-		meta nfproto ipv4 dnat ip to ip saddr . tcp dport map @z4
+		meta l4proto tcp dnat ip to ip saddr map @y4
+		dnat ip to ip saddr . tcp dport map @z4
 		dnat ip to numgen inc mod 2 map @t1v4
 		meta l4proto tcp dnat ip to numgen inc mod 2 map @t2v4
 		dnat ip6 to ip6 daddr map @x6
 		ip6 saddr dead::1 dnat ip6 to feed::1
 		ip6 saddr dead::2 tcp dport 42 dnat ip6 to [c0::1a]:4242
-		meta l4proto tcp meta nfproto ipv6 dnat ip6 to ip6 saddr map @y6
-		meta nfproto ipv6 dnat ip6 to ip6 saddr . tcp dport map @z6
+		meta l4proto tcp dnat ip6 to ip6 saddr map @y6
+		dnat ip6 to ip6 saddr . tcp dport map @z6
 		dnat ip6 to numgen inc mod 2 map @t1v6
 		meta l4proto tcp dnat ip6 to numgen inc mod 2 map @t2v6
 	}
-- 
2.34.1

Re: [nft PATCH v2 0/5] Store multiple payload dependencies

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> Patch 1 fixes a mistake in a Python test-case.

Sorry, I fat-fingered this when mangling your patch to delete the
duplicated content.

96 test files, 96 files passed, 1887 unit tests,
0 error, 0 warning

... Can't remember ever seeing all test cases pass with 0 warnings.

Applied, thanks a lot!