[nft PATCH 00/11] Store multiple payload dependencies

[nft PATCH 00/11] Store multiple payload dependencies

[Jeremy Sowden]

The first patch in this set fixes a cut-and-paste error in an inet
Python test payload which leads to test-failures.  However, even with
this fix in place, the test-case still fails:

  inet/sets.t: WARNING: line 24: 'add rule inet test-inet input ip saddr . ip daddr . tcp dport @set3 accept': 'ip saddr . ip daddr . tcp dport @set3 accept' mismatches 'meta nfproto ipv4 ip saddr . ip daddr . tcp dport @set3 accept'
  inet/sets.t: WARNING: line 24: 'add rule bridge test-inet input ip saddr . ip daddr . tcp dport @set3 accept': 'ip saddr . ip daddr . tcp dport @set3 accept' mismatches 'meta protocol ip ip saddr . ip daddr . tcp dport @set3 accept'
  inet/sets.t: WARNING: line 24: 'add rule netdev test-netdev ingress ip saddr . ip daddr . tcp dport @set3 accept': 'ip saddr . ip daddr . tcp dport @set3 accept' mismatches 'meta protocol ip ip saddr . ip daddr . tcp dport @set3 accept'
  inet/sets.t: WARNING: line 24: 'add rule netdev test-netdev egress ip saddr . ip daddr . tcp dport @set3 accept': 'ip saddr . ip daddr . tcp dport @set3 accept' mismatches 'meta protocol ip ip saddr . ip daddr . tcp dport @set3 accept'

The expected output does not include the initial protocol matches.
Since the netdev and bridge families express these matches differently
from how inet does it, it is not possible simply to add the correct
output to the test-case, e.g.:

  -ip saddr . ip daddr . tcp dport @set3 accept;ok
  +ip saddr . ip daddr . tcp dport @set3 accept;ok;meta nfproto ipv4 ip saddr . ip daddr . tcp dport @set3 accept

and so my initial approach was to split the test-case, moving the netdev
and bridge tests into their respective directories.

However, the protocol matches are redundant and on further thought it
seemed like a better idea to improve the code that performs payload-
dependency elimination.  That is the purpose of this patch-set.

Here's the netlink dump for the test:

  [ 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.

Patches 1-3 fix mistakes in Python test-cases.  Patches 4-8 do a bit of
tidying and make some preliminary changes.  Patch 9 adds the extra
dependencies.  Patches 10 & 11 remove redundant protocol matches which
are now eliminated from test-cases.

At the end of this series all tests pass.

Jeremy Sowden (11):
  tests: py: fix inet/sets.t netdev payload
  tests: py: fix inet/ip.t payloads
  tests: py: fix inet/ip_tcp.t test
  netlink_delinearize: fix typo
  src: remove arithmetic on booleans
  src: reduce indentation
  src: simplify logic governing storing payload dependencies
  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.c                                 | 21 ++---
 src/netlink_delinearize.c                     | 53 +++++------
 src/payload.c                                 | 90 +++++++++++++------
 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/ip.t.payload.netdev             |  2 +-
 tests/py/inet/ip_tcp.t                        |  4 +-
 tests/py/inet/ip_tcp.t.json.output            | 12 +++
 tests/py/inet/sets.t.json                     | 11 ---
 tests/py/inet/sets.t.payload.netdev           |  6 +-
 .../testcases/maps/dumps/0010concat_map_0.nft |  2 +-
 .../testcases/maps/dumps/nat_addr_port.nft    |  8 +-
 14 files changed, 129 insertions(+), 108 deletions(-)

-- 
2.34.1

[nft PATCH 01/11] tests: py: fix inet/sets.t netdev payload

[Jeremy Sowden]

The netdev payload for one of the inet/sets.t tests was cut-and-pasted
from the inet payload without being properly updated.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 tests/py/inet/sets.t.payload.netdev | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/py/inet/sets.t.payload.netdev b/tests/py/inet/sets.t.payload.netdev
index 9d6f6bbd62b1..e31aeb9274e6 100644
--- a/tests/py/inet/sets.t.payload.netdev
+++ b/tests/py/inet/sets.t.payload.netdev
@@ -15,9 +15,9 @@ netdev test-netdev ingress
   [ immediate reg 0 accept ]
 
 # ip saddr . ip daddr . tcp dport @set3 accept
-inet 
-  [ meta load nfproto => reg 1 ]
-  [ cmp eq reg 1 0x00000002 ]
+netdev
+  [ meta load protocol => reg 1 ]
+  [ cmp eq reg 1 0x00000008 ]
   [ meta load l4proto => reg 1 ]
   [ cmp eq reg 1 0x00000006 ]
   [ payload load 4b @ network header + 12 => reg 1 ]
-- 
2.34.1

[nft PATCH 02/11] tests: py: fix inet/ip.t payloads

[Jeremy Sowden]

In one of the bridge payloads, the wrong command is given to load the
protocol.

The leading comment for one of the netdev payload includes a redundant
payload dependency.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 tests/py/inet/ip.t.payload.bridge | 2 +-
 tests/py/inet/ip.t.payload.netdev | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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 ]
diff --git a/tests/py/inet/ip.t.payload.netdev b/tests/py/inet/ip.t.payload.netdev
index 38ed0ad316e4..f11225396d35 100644
--- a/tests/py/inet/ip.t.payload.netdev
+++ b/tests/py/inet/ip.t.payload.netdev
@@ -12,7 +12,7 @@ netdev test-netdev ingress
   [ payload load 6b @ link header + 6 => reg 10 ]
   [ lookup reg 1 set __set%d ]
 
-# meta protocol ip ip saddr . ip daddr . ether saddr { 1.1.1.1 . 2.2.2.2 . ca:fe:ca:fe:ca:fe }
+# ip saddr . ip daddr . ether saddr { 1.1.1.1 . 2.2.2.2 . ca:fe:ca:fe:ca:fe }
 __set%d test-netdev 3
 __set%d test-netdev 0
 	element 01010101 02020202 fecafeca 0000feca  : 0 [end]
-- 
2.34.1

[nft PATCH 03/11] tests: py: fix inet/ip_tcp.t test

[Jeremy Sowden]

Contrary to the comment and expected output, nft does _not_ eliminate
the redundant `ip protocol` expression from the second test.  Dependency
elimination requires a higher level expression.  `ip saddr` cannot lead
to the elimination of `ip protocol` since they are both L3 expressions.
`tcp dport` cannot because although `ip saddr` and `ip protocol` both
imply that the L3 protocol is `ip`, only protocol matches are stored as
dependencies, so the redundancy is not apparent, and in fact,
`payload_may_dependency_kill` explicitly checks for the combination of
inet, bridge or netdev family, L4 expression and L3 ipv4 or ipv6
dependency and returns false.

Correct the expected output and comment.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 tests/py/inet/ip_tcp.t             |  4 ++--
 tests/py/inet/ip_tcp.t.json.output | 12 ++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tests/py/inet/ip_tcp.t b/tests/py/inet/ip_tcp.t
index ab76ffa90a9c..03bafc098536 100644
--- a/tests/py/inet/ip_tcp.t
+++ b/tests/py/inet/ip_tcp.t
@@ -9,8 +9,8 @@
 # must not remove ip dependency -- ONLY ipv4 packets should be matched
 ip protocol tcp tcp dport 22;ok;ip protocol 6 tcp dport 22
 
-# can remove it here, ip protocol is implied via saddr.
-ip protocol tcp ip saddr 1.2.3.4 tcp dport 22;ok;ip saddr 1.2.3.4 tcp dport 22
+# could in principle remove it here since ipv4 is implied via saddr.
+ip protocol tcp ip saddr 1.2.3.4 tcp dport 22;ok;ip protocol 6 ip saddr 1.2.3.4 tcp dport 22
 
 # but not here.
 ip protocol tcp counter ip saddr 1.2.3.4 tcp dport 22;ok;ip protocol 6 counter ip saddr 1.2.3.4 tcp dport 22
diff --git a/tests/py/inet/ip_tcp.t.json.output b/tests/py/inet/ip_tcp.t.json.output
index 4a6a05d7f10a..acad8b1fae76 100644
--- a/tests/py/inet/ip_tcp.t.json.output
+++ b/tests/py/inet/ip_tcp.t.json.output
@@ -28,6 +28,18 @@
 
 # ip protocol tcp ip saddr 1.2.3.4 tcp dport 22
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "protocol",
+                    "protocol": "ip"
+                }
+            },
+	    "op": "==",
+            "right": 6
+        }
+    },
     {
         "match": {
             "left": {
-- 
2.34.1

[nft PATCH 04/11] netlink_delinearize: fix typo

[Jeremy Sowden]

Correct spelling in comment.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/netlink_delinearize.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 323e9150cdf6..83be2fec441d 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2057,7 +2057,7 @@ static bool __meta_dependency_may_kill(const struct expr *dep, uint8_t *nfproto)
 }
 
 /* We have seen a protocol key expression that restricts matching at the network
- * base, leave it in place since this is meaninful in bridge, inet and netdev
+ * base, leave it in place since this is meaningful in bridge, inet and netdev
  * families. Exceptions are ICMP and ICMPv6 where this code assumes that can
  * only happen with IPv4 and IPv6.
  */
-- 
2.34.1

[nft PATCH 05/11] src: remove arithmetic on booleans

[Jeremy Sowden]

Instead of subtracting a boolean from the protocol base for stacked
payloads, just decrement the base variable itself.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/netlink.c             | 10 ++++++----
 src/netlink_delinearize.c |  8 ++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/netlink.c b/src/netlink.c
index 359d801c29d3..5aad865955db 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1868,7 +1868,6 @@ static void trace_gen_stmts(struct list_head *stmts,
 	const void *hdr;
 	uint32_t hlen;
 	unsigned int n;
-	bool stacked;
 
 	if (!nftnl_trace_is_set(nlt, attr))
 		return;
@@ -1923,6 +1922,8 @@ restart:
 	n = 0;
 next:
 	list_for_each_entry(stmt, &unordered, list) {
+		enum proto_bases b = base;
+
 		rel = stmt->expr;
 		lhs = rel->left;
 
@@ -1935,17 +1936,18 @@ next:
 		list_move_tail(&stmt->list, stmts);
 		n++;
 
-		stacked = payload_is_stacked(desc, rel);
+		if (payload_is_stacked(desc, rel))
+			b--;
 
 		if (lhs->flags & EXPR_F_PROTOCOL &&
 		    pctx->pbase == PROTO_BASE_INVALID) {
-			payload_dependency_store(pctx, stmt, base - stacked);
+			payload_dependency_store(pctx, stmt, b);
 		} else {
 			/* Don't strip 'icmp type' from payload dump. */
 			if (pctx->icmp_type == 0)
 				payload_dependency_kill(pctx, lhs, ctx->family);
 			if (lhs->flags & EXPR_F_PROTOCOL)
-				payload_dependency_store(pctx, stmt, base - stacked);
+				payload_dependency_store(pctx, stmt, b);
 		}
 
 		goto next;
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 83be2fec441d..39b0574e38c8 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1867,7 +1867,6 @@ static void payload_match_expand(struct rule_pp_ctx *ctx,
 	struct stmt *nstmt;
 	struct expr *nexpr = NULL;
 	enum proto_bases base = left->payload.base;
-	bool stacked;
 
 	payload_expr_expand(&list, left, &ctx->pctx);
 
@@ -1893,7 +1892,8 @@ static void payload_match_expand(struct rule_pp_ctx *ctx,
 		assert(left->payload.base);
 		assert(base == left->payload.base);
 
-		stacked = payload_is_stacked(ctx->pctx.protocol[base].desc, nexpr);
+		if (payload_is_stacked(ctx->pctx.protocol[base].desc, nexpr))
+			base--;
 
 		/* Remember the first payload protocol expression to
 		 * kill it later on if made redundant by a higher layer
@@ -1902,12 +1902,12 @@ static void payload_match_expand(struct rule_pp_ctx *ctx,
 		if (ctx->pdctx.pbase == PROTO_BASE_INVALID &&
 		    expr->op == OP_EQ &&
 		    left->flags & EXPR_F_PROTOCOL) {
-			payload_dependency_store(&ctx->pdctx, nstmt, base - stacked);
+			payload_dependency_store(&ctx->pdctx, nstmt, base);
 		} else {
 			payload_dependency_kill(&ctx->pdctx, nexpr->left,
 						ctx->pctx.family);
 			if (expr->op == OP_EQ && left->flags & EXPR_F_PROTOCOL)
-				payload_dependency_store(&ctx->pdctx, nstmt, base - stacked);
+				payload_dependency_store(&ctx->pdctx, nstmt, base);
 		}
 	}
 	list_del(&ctx->stmt->list);
-- 
2.34.1

[nft PATCH 06/11] src: reduce indentation

[Jeremy Sowden]

Re-arrange some switch-cases and conditionals to reduce levels of
indentation.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/netlink_delinearize.c | 10 +++-------
 src/payload.c             | 18 +++++++++++-------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 39b0574e38c8..36ead8029691 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2079,14 +2079,10 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
 	case NFPROTO_NETDEV:
 	case NFPROTO_BRIDGE:
 		break;
+	case NFPROTO_IPV4:
+	case NFPROTO_IPV6:
+		return family == nfproto;
 	default:
-		if (family == NFPROTO_IPV4 &&
-		    nfproto != NFPROTO_IPV4)
-			return false;
-		else if (family == NFPROTO_IPV6 &&
-			 nfproto != NFPROTO_IPV6)
-			return false;
-
 		return true;
 	}
 
diff --git a/src/payload.c b/src/payload.c
index 79008762825f..576eb149f71d 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -733,13 +733,17 @@ static bool payload_may_dependency_kill(struct payload_dep_ctx *ctx,
 		break;
 	}
 
-	if (expr->payload.base == PROTO_BASE_TRANSPORT_HDR &&
-	    dep->left->payload.base == PROTO_BASE_TRANSPORT_HDR) {
-		if (dep->left->payload.desc == &proto_icmp)
-			return payload_may_dependency_kill_icmp(ctx, expr);
-		if (dep->left->payload.desc == &proto_icmp6)
-			return payload_may_dependency_kill_icmp(ctx, expr);
-	}
+	if (expr->payload.base != PROTO_BASE_TRANSPORT_HDR)
+		return true;
+
+	if (dep->left->payload.base != PROTO_BASE_TRANSPORT_HDR)
+		return true;
+
+	if (dep->left->payload.desc == &proto_icmp)
+		return payload_may_dependency_kill_icmp(ctx, expr);
+
+	if (dep->left->payload.desc == &proto_icmp6)
+		return payload_may_dependency_kill_icmp(ctx, expr);
 
 	return true;
 }
-- 
2.34.1

[nft PATCH 07/11] src: simplify logic governing storing payload dependencies

[Jeremy Sowden]

There are several places where we check whether `ctx->pdctx.pbase`
equal to `PROTO_BASE_INVALID` and don't bother trying to free the
dependency if so.  However, these checks are redundant.

In `payload_match_expand` and `trace_gen_stmts`, we skip a call to
`payload_dependency_kill`, but that calls `payload_dependency_exists` to check a
dependency exists before doing anything else.

In `ct_meta_common_postprocess`, we skip an open-coded equivalent to
`payload_dependency_kill` which performs some different checks, but the
first is the same: a call to `payload_dependency_exists`.

Therefore, we can drop the redundant checks and simplify the flow-
control in the functions.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/netlink.c             | 13 ++++---------
 src/netlink_delinearize.c | 17 ++++-------------
 2 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/src/netlink.c b/src/netlink.c
index 5aad865955db..15b8878eb488 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1939,16 +1939,11 @@ next:
 		if (payload_is_stacked(desc, rel))
 			b--;
 
-		if (lhs->flags & EXPR_F_PROTOCOL &&
-		    pctx->pbase == PROTO_BASE_INVALID) {
+		/* Don't strip 'icmp type' from payload dump. */
+		if (pctx->icmp_type == 0)
+			payload_dependency_kill(pctx, lhs, ctx->family);
+		if (lhs->flags & EXPR_F_PROTOCOL)
 			payload_dependency_store(pctx, stmt, b);
-		} else {
-			/* Don't strip 'icmp type' from payload dump. */
-			if (pctx->icmp_type == 0)
-				payload_dependency_kill(pctx, lhs, ctx->family);
-			if (lhs->flags & EXPR_F_PROTOCOL)
-				payload_dependency_store(pctx, stmt, b);
-		}
 
 		goto next;
 	}
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 36ead8029691..fd81e07151c2 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1899,16 +1899,10 @@ static void payload_match_expand(struct rule_pp_ctx *ctx,
 		 * kill it later on if made redundant by a higher layer
 		 * payload expression.
 		 */
-		if (ctx->pdctx.pbase == PROTO_BASE_INVALID &&
-		    expr->op == OP_EQ &&
-		    left->flags & EXPR_F_PROTOCOL) {
+		payload_dependency_kill(&ctx->pdctx, nexpr->left,
+					ctx->pctx.family);
+		if (expr->op == OP_EQ && left->flags & EXPR_F_PROTOCOL)
 			payload_dependency_store(&ctx->pdctx, nstmt, base);
-		} else {
-			payload_dependency_kill(&ctx->pdctx, nexpr->left,
-						ctx->pctx.family);
-			if (expr->op == OP_EQ && left->flags & EXPR_F_PROTOCOL)
-				payload_dependency_store(&ctx->pdctx, nstmt, base);
-		}
 	}
 	list_del(&ctx->stmt->list);
 	stmt_free(ctx->stmt);
@@ -2125,10 +2119,7 @@ static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx,
 
 		relational_expr_pctx_update(&ctx->pctx, expr);
 
-		if (ctx->pdctx.pbase == PROTO_BASE_INVALID &&
-		    left->flags & EXPR_F_PROTOCOL) {
-			payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
-		} else if (ctx->pdctx.pbase < PROTO_BASE_TRANSPORT_HDR) {
+		if (ctx->pdctx.pbase < PROTO_BASE_TRANSPORT_HDR) {
 			if (payload_dependency_exists(&ctx->pdctx, base) &&
 			    meta_may_dependency_kill(&ctx->pdctx,
 						     ctx->pctx.family, expr))
-- 
2.34.1

[nft PATCH 08/11] 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.

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

diff --git a/include/payload.h b/include/payload.h
index 8bc3fb9a8a54..10ae9fe4f9c5 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 stmt *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..2a62b309be1d 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2060,11 +2060,13 @@ 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;
 
 	if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
 		return true;
 
+	dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;
+
 	if (__meta_dependency_may_kill(dep, &nfproto))
 		return true;
 
diff --git a/src/payload.c b/src/payload.c
index 576eb149f71d..902b318ae23a 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 stmt *payload_dependency_get(struct payload_dep_ctx *ctx,
+				    enum proto_bases base)
+{
+	if (ctx->pbase == base)
+		return ctx->pdep;
+
+	if (base == PROTO_BASE_TRANSPORT_HDR &&
+	    ctx->pbase == PROTO_BASE_INNER_HDR)
+		return ctx->pdep;
+
+	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)->expr;
 	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)->expr;
 
-	/* 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)->expr;
 
 	/* 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

Re: [nft PATCH 08/11] src: add a helper that returns a payload dependency for a particular base

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> 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.

> +	dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;

This new helper can return NULL, would you mind reworking this to add
error checks here?

I've applied all patches up to this one.

Re: [nft PATCH 08/11] src: add a helper that returns a payload dependency for a particular base

[Jeremy Sowden]

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

On 2022-01-15, at 17:48:24 +0100, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > 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.
>
> > +	dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;
>
> This new helper can return NULL, would you mind reworking this to add
> error checks here?

Yup.

> I've applied all patches up to this one.

Thanks.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [nft PATCH 08/11] src: add a helper that returns a payload dependency for a particular base

[Jeremy Sowden]

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

On 2022-01-15, at 16:57:07 +0000, Jeremy Sowden wrote:
> On 2022-01-15, at 17:48:24 +0100, Florian Westphal wrote:
> > Jeremy Sowden <jeremy@azazel.net> wrote:
> > > 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.
> >
> > > +	dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;
> >
> > This new helper can return NULL, would you mind reworking this to
> > add error checks here?
>
> Yup.

Actually, let me provide a bit more context:

  @@ -2060,11 +2060,13 @@ 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;

          if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
                  return true;

  +       dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;
  +

We check that there is a PROTO_BASE_NETWORK_HDR dependency immediately
before calling the helper.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [nft PATCH 08/11] src: add a helper that returns a payload dependency for a particular base

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> On 2022-01-15, at 16:57:07 +0000, Jeremy Sowden wrote:
> > On 2022-01-15, at 17:48:24 +0100, Florian Westphal wrote:
> > > Jeremy Sowden <jeremy@azazel.net> wrote:
> > > > 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.
> > >
> > > > +	dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;
> > >
> > > This new helper can return NULL, would you mind reworking this to
> > > add error checks here?
> >
> > Yup.
> 
> Actually, let me provide a bit more context:
> 
>   @@ -2060,11 +2060,13 @@ 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;
> 
>           if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
>                   return true;
> 
>   +       dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;
>   +
> 
> We check that there is a PROTO_BASE_NETWORK_HDR dependency immediately
> before calling the helper.

Perhaps remove the check?

bla()->foo looks weird, esp. given bla() last line is "return NULL".

Re: [nft PATCH 08/11] src: add a helper that returns a payload dependency for a particular base

[Jeremy Sowden]

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

On 2022-01-15, at 18:09:01 +0100, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > On 2022-01-15, at 16:57:07 +0000, Jeremy Sowden wrote:
> > > On 2022-01-15, at 17:48:24 +0100, Florian Westphal wrote:
> > > > Jeremy Sowden <jeremy@azazel.net> wrote:
> > > > > 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.
> > > >
> > > > > +	dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;
> > > >
> > > > This new helper can return NULL, would you mind reworking this
> > > > to add error checks here?
> > >
> > > Yup.
> >
> > Actually, let me provide a bit more context:
> >
> >   @@ -2060,11 +2060,13 @@ 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;
> >
> >           if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
> >                   return true;
> >
> >   +       dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;
> >   +
> >
> > We check that there is a PROTO_BASE_NETWORK_HDR dependency
> > immediately before calling the helper.
>
> Perhaps remove the check?
>
> bla()->foo looks weird, esp. given bla() last line is "return NULL".

Righto.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[nft PATCH 09/11] 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 | 16 ++++++++-----
 src/payload.c             | 49 ++++++++++++++++++++++++---------------
 3 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/include/payload.h b/include/payload.h
index 10ae9fe4f9c5..8341f4e67183 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 stmt *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 2a62b309be1d..10b1e5fbd000 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2062,7 +2062,7 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
 	uint8_t l4proto, nfproto = NFPROTO_UNSPEC;
 	struct expr *dep;
 
-	if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
+	if (!payload_dependency_exists(ctx, PROTO_BASE_NETWORK_HDR))
 		return true;
 
 	dep = payload_dependency_get(ctx, PROTO_BASE_NETWORK_HDR)->expr;
@@ -2121,11 +2121,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);
 		}
@@ -2655,7 +2656,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;
@@ -2663,7 +2665,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:
@@ -2697,7 +2700,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 902b318ae23a..91eec112351d 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 stmt *payload_dependency_get(struct payload_dep_ctx *ctx,
 				    enum proto_bases base)
 {
-	if (ctx->pbase == base)
-		return ctx->pdep;
+	if (ctx->pdeps[base])
+		return ctx->pdeps[base];
 
 	if (base == PROTO_BASE_TRANSPORT_HDR &&
-	    ctx->pbase == PROTO_BASE_INNER_HDR)
-		return ctx->pdep;
+	    ctx->pdeps[PROTO_BASE_INNER_HDR])
+		return ctx->pdeps[PROTO_BASE_INNER_HDR];
 
 	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 10/11] 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 11/11] 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