[PATCH nft 0/6] rework dependency removal (v2)

[PATCH nft 0/6] rework dependency removal (v2)

[Pablo Neira Ayuso]

Hi,

This patchset aims to address what Florian reported time ago [1]. This
is skipping removal of protocol key payload expressions at network base
for the netdev, bridge and inet.

It would better to annotate all redundant expressions and add a later
stage, where we can do smarter simplifications by looking globally at
what we have, instead of just looking at current protocol key expression
and last one that we have annotated in the context structure to perform
removals. But I would prefer to have a fix now upstream then look at
this larger rework later on since it would require to review a bit of
the postprocess code.

The initial 4 patches in this batch are just cleanup/preparation patches
for patches 5/6 and 6/6.

There is still a few warning in the tests/py/ infrastructure, some of
them I think need to be fixed, and at least one can remain there to
remind us that we can do better.

Let me know if you have any concern with this.

Thanks.

[1] https://www.spinics.net/lists/netfilter-devel/msg50078.html

Pablo Neira Ayuso (6):
  src: pass family to payload_dependency_kill()
  payload: add payload_dependency_release() helper function
  src: add payload_dependency_exists()
  src: get rid of __payload_dependency_kill()
  payload: add payload_may_dependency_kill()
  netlink_delinearize: add meta_may_dependency_kill()

 include/payload.h         |   7 +--
 src/netlink.c             |   2 +-
 src/netlink_delinearize.c | 106 +++++++++++++++++++++++++++++++++++++++++-----
 src/payload.c             |  85 ++++++++++++++++++++++++++++---------
 4 files changed, 165 insertions(+), 35 deletions(-)

-- 
2.11.0

[PATCH nft 1/6] src: pass family to payload_dependency_kill()

[Pablo Neira Ayuso]

This context information is very relevant when deciding if a redundant
dependency needs to be removed or not, specifically for the inet, bridge
and netdev families. This new parameter is used by follow up patch
entitled ("payload: add payload_should_dependency_kill()").

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/payload.h         |  7 ++++---
 src/netlink.c             |  2 +-
 src/netlink_delinearize.c | 18 +++++++++++-------
 src/payload.c             | 14 ++++++++------
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/payload.h b/include/payload.h
index 8e357aef461e..294ff2706e30 100644
--- a/include/payload.h
+++ b/include/payload.h
@@ -41,11 +41,12 @@ extern void payload_dependency_store(struct payload_dep_ctx *ctx,
 				     struct stmt *stmt,
 				     enum proto_bases base);
 extern void __payload_dependency_kill(struct payload_dep_ctx *ctx,
-				      enum proto_bases base);
+				      enum proto_bases base,
+				      unsigned int family);
 extern void payload_dependency_kill(struct payload_dep_ctx *ctx,
-				    struct expr *expr);
+				    struct expr *expr, unsigned int family);
 extern void exthdr_dependency_kill(struct payload_dep_ctx *ctx,
-				   struct expr *expr);
+				   struct expr *expr, unsigned int family);
 
 extern bool payload_can_merge(const struct expr *e1, const struct expr *e2);
 extern struct expr *payload_expr_join(const struct expr *e1,
diff --git a/src/netlink.c b/src/netlink.c
index 488ae6f3971f..233bfd2df764 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -2768,7 +2768,7 @@ next:
 		    pctx->pbase == PROTO_BASE_INVALID) {
 			payload_dependency_store(pctx, stmt, base - stacked);
 		} else {
-			payload_dependency_kill(pctx, lhs);
+			payload_dependency_kill(pctx, lhs, ctx->family);
 			if (lhs->flags & EXPR_F_PROTOCOL)
 				payload_dependency_store(pctx, stmt, base - stacked);
 		}
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 256552b5b46e..8d11969e0fb1 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1352,7 +1352,8 @@ static void payload_match_expand(struct rule_pp_ctx *ctx,
 		    left->flags & EXPR_F_PROTOCOL) {
 			payload_dependency_store(&ctx->pdctx, nstmt, base - stacked);
 		} else {
-			payload_dependency_kill(&ctx->pdctx, nexpr->left);
+			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);
 		}
@@ -1383,7 +1384,7 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
 		payload_expr_complete(payload, &ctx->pctx);
 		expr_set_type(expr->right, payload->dtype,
 			      payload->byteorder);
-		payload_dependency_kill(&ctx->pdctx, payload);
+		payload_dependency_kill(&ctx->pdctx, payload, ctx->pctx.family);
 		break;
 	}
 }
@@ -1406,7 +1407,8 @@ static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx,
 		    left->flags & EXPR_F_PROTOCOL) {
 			payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
 		} else if (ctx->pdctx.pbase < PROTO_BASE_TRANSPORT_HDR) {
-			__payload_dependency_kill(&ctx->pdctx, base);
+			__payload_dependency_kill(&ctx->pdctx, base,
+						  ctx->pctx.family);
 			if (left->flags & EXPR_F_PROTOCOL)
 				payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
 		}
@@ -1814,7 +1816,7 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 		break;
 	case EXPR_PAYLOAD:
 		payload_expr_complete(expr, &ctx->pctx);
-		payload_dependency_kill(&ctx->pdctx, expr);
+		payload_dependency_kill(&ctx->pdctx, expr, ctx->pctx.family);
 		break;
 	case EXPR_VALUE:
 		// FIXME
@@ -1837,7 +1839,7 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 		expr_postprocess(ctx, &expr->key);
 		break;
 	case EXPR_EXTHDR:
-		exthdr_dependency_kill(&ctx->pdctx, expr);
+		exthdr_dependency_kill(&ctx->pdctx, expr, ctx->pctx.family);
 		break;
 	case EXPR_SET_REF:
 	case EXPR_META:
@@ -1870,14 +1872,16 @@ static void stmt_reject_postprocess(struct rule_pp_ctx *rctx)
 		stmt->reject.expr->dtype = &icmp_code_type;
 		if (stmt->reject.type == NFT_REJECT_TCP_RST)
 			__payload_dependency_kill(&rctx->pdctx,
-						  PROTO_BASE_TRANSPORT_HDR);
+						  PROTO_BASE_TRANSPORT_HDR,
+						  rctx->pctx.family);
 		break;
 	case NFPROTO_IPV6:
 		stmt->reject.family = rctx->pctx.family;
 		stmt->reject.expr->dtype = &icmpv6_code_type;
 		if (stmt->reject.type == NFT_REJECT_TCP_RST)
 			__payload_dependency_kill(&rctx->pdctx,
-						  PROTO_BASE_TRANSPORT_HDR);
+						  PROTO_BASE_TRANSPORT_HDR,
+						  rctx->pctx.family);
 		break;
 	case NFPROTO_INET:
 		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH) {
diff --git a/src/payload.c b/src/payload.c
index 60090accbcd8..df3c8136c88c 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -438,7 +438,7 @@ void payload_dependency_store(struct payload_dep_ctx *ctx,
  * implies its existance.
  */
 void __payload_dependency_kill(struct payload_dep_ctx *ctx,
-			       enum proto_bases base)
+			       enum proto_bases base, unsigned int family)
 {
 	if (ctx->pbase != PROTO_BASE_INVALID &&
 	    ctx->pbase == base &&
@@ -453,19 +453,21 @@ void __payload_dependency_kill(struct payload_dep_ctx *ctx,
 	}
 }
 
-void payload_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr)
+void payload_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
+			     unsigned int family)
 {
-	__payload_dependency_kill(ctx, expr->payload.base);
+	__payload_dependency_kill(ctx, expr->payload.base, family);
 }
 
-void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr)
+void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
+			    unsigned int family)
 {
 	switch (expr->exthdr.op) {
 	case NFT_EXTHDR_OP_TCPOPT:
-		__payload_dependency_kill(ctx, PROTO_BASE_TRANSPORT_HDR);
+		__payload_dependency_kill(ctx, PROTO_BASE_TRANSPORT_HDR, family);
 		break;
 	case NFT_EXTHDR_OP_IPV6:
-		__payload_dependency_kill(ctx, PROTO_BASE_NETWORK_HDR);
+		__payload_dependency_kill(ctx, PROTO_BASE_NETWORK_HDR, family);
 		break;
 	default:
 		break;
-- 
2.11.0

[PATCH nft 2/6] payload: add payload_dependency_release() helper function

[Pablo Neira Ayuso]

Wrap code that releases existing dependencies that we have just
annotated in the context structure.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/payload.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/payload.c b/src/payload.c
index df3c8136c88c..21c2842807cd 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -428,6 +428,17 @@ void payload_dependency_store(struct payload_dep_ctx *ctx,
 	ctx->pdep  = stmt;
 }
 
+static void payload_dependency_release(struct payload_dep_ctx *ctx)
+{
+	list_del(&ctx->pdep->list);
+	stmt_free(ctx->pdep);
+
+	ctx->pbase = PROTO_BASE_INVALID;
+	if (ctx->pdep == ctx->prev)
+		ctx->prev = NULL;
+	ctx->pdep  = NULL;
+}
+
 /**
  * __payload_dependency_kill - kill a redundant payload depedency
  *
@@ -442,15 +453,8 @@ void __payload_dependency_kill(struct payload_dep_ctx *ctx,
 {
 	if (ctx->pbase != PROTO_BASE_INVALID &&
 	    ctx->pbase == base &&
-	    ctx->pdep != NULL) {
-		list_del(&ctx->pdep->list);
-		stmt_free(ctx->pdep);
-
-		ctx->pbase = PROTO_BASE_INVALID;
-		if (ctx->pdep == ctx->prev)
-			ctx->prev = NULL;
-		ctx->pdep  = NULL;
-	}
+	    ctx->pdep != NULL)
+		payload_dependency_release(ctx);
 }
 
 void payload_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
-- 
2.11.0

[PATCH nft 3/6] src: add payload_dependency_exists()

[Pablo Neira Ayuso]

This helper function tells us if there is already a protocol key payload
expression, ie. those with EXPR_F_PROTOCOL flag set on, that we might
want to remove since we can infer from another expression in the upper
protocol base, eg.

	ip protocol tcp tcp dport 22

'ip protocol tcp' can be removed in the ip family since it is redundant,
but not in the netdev, bridge and inet families, where we cannot make
assumptions on the layer 3 protocol.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/payload.h         |  3 ++-
 src/netlink_delinearize.c | 15 +++++++++------
 src/payload.c             | 34 +++++++++++++++++++++++++---------
 3 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/include/payload.h b/include/payload.h
index 294ff2706e30..dec4647aad9f 100644
--- a/include/payload.h
+++ b/include/payload.h
@@ -40,8 +40,9 @@ void payload_dependency_reset(struct payload_dep_ctx *ctx);
 extern void payload_dependency_store(struct payload_dep_ctx *ctx,
 				     struct stmt *stmt,
 				     enum proto_bases base);
+extern bool payload_dependency_exists(const struct payload_dep_ctx *ctx,
+				      enum proto_bases base);
 extern void __payload_dependency_kill(struct payload_dep_ctx *ctx,
-				      enum proto_bases base,
 				      unsigned int family);
 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 8d11969e0fb1..f4b943964fc9 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1407,8 +1407,9 @@ static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx,
 		    left->flags & EXPR_F_PROTOCOL) {
 			payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
 		} else if (ctx->pdctx.pbase < PROTO_BASE_TRANSPORT_HDR) {
-			__payload_dependency_kill(&ctx->pdctx, base,
-						  ctx->pctx.family);
+			if (payload_dependency_exists(&ctx->pdctx, base))
+				__payload_dependency_kill(&ctx->pdctx,
+							  ctx->pctx.family);
 			if (left->flags & EXPR_F_PROTOCOL)
 				payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
 		}
@@ -1870,17 +1871,19 @@ static void stmt_reject_postprocess(struct rule_pp_ctx *rctx)
 	case NFPROTO_IPV4:
 		stmt->reject.family = rctx->pctx.family;
 		stmt->reject.expr->dtype = &icmp_code_type;
-		if (stmt->reject.type == NFT_REJECT_TCP_RST)
+		if (stmt->reject.type == NFT_REJECT_TCP_RST &&
+		    payload_dependency_exists(&rctx->pdctx,
+					      PROTO_BASE_TRANSPORT_HDR))
 			__payload_dependency_kill(&rctx->pdctx,
-						  PROTO_BASE_TRANSPORT_HDR,
 						  rctx->pctx.family);
 		break;
 	case NFPROTO_IPV6:
 		stmt->reject.family = rctx->pctx.family;
 		stmt->reject.expr->dtype = &icmpv6_code_type;
-		if (stmt->reject.type == NFT_REJECT_TCP_RST)
+		if (stmt->reject.type == NFT_REJECT_TCP_RST &&
+		    payload_dependency_exists(&rctx->pdctx,
+					      PROTO_BASE_TRANSPORT_HDR))
 			__payload_dependency_kill(&rctx->pdctx,
-						  PROTO_BASE_TRANSPORT_HDR,
 						  rctx->pctx.family);
 		break;
 	case NFPROTO_INET:
diff --git a/src/payload.c b/src/payload.c
index 21c2842807cd..df59ac8adcac 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -428,6 +428,23 @@ void payload_dependency_store(struct payload_dep_ctx *ctx,
 	ctx->pdep  = stmt;
 }
 
+/**
+ * payload_dependency_exists - there is a payload dependency in place
+ * @ctx: payload dependency context
+ * @base: payload protocol base
+ *
+ * Check if we have seen a protocol key payload expression for this base, we can
+ * usually remove it if we can infer it from another payload expression in the
+ * upper base.
+ */
+bool payload_dependency_exists(const struct payload_dep_ctx *ctx,
+			       enum proto_bases base)
+{
+	return ctx->pbase != PROTO_BASE_INVALID &&
+	       ctx->pbase == base &&
+	       ctx->pdep != NULL;
+}
+
 static void payload_dependency_release(struct payload_dep_ctx *ctx)
 {
 	list_del(&ctx->pdep->list);
@@ -448,19 +465,16 @@ static void payload_dependency_release(struct payload_dep_ctx *ctx)
  * Kill a redundant payload expression if a higher layer payload expression
  * implies its existance.
  */
-void __payload_dependency_kill(struct payload_dep_ctx *ctx,
-			       enum proto_bases base, unsigned int family)
+void __payload_dependency_kill(struct payload_dep_ctx *ctx, unsigned int family)
 {
-	if (ctx->pbase != PROTO_BASE_INVALID &&
-	    ctx->pbase == base &&
-	    ctx->pdep != NULL)
-		payload_dependency_release(ctx);
+	payload_dependency_release(ctx);
 }
 
 void payload_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 			     unsigned int family)
 {
-	__payload_dependency_kill(ctx, expr->payload.base, family);
+	if (payload_dependency_exists(ctx, expr->payload.base))
+		__payload_dependency_kill(ctx, family);
 }
 
 void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
@@ -468,10 +482,12 @@ void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 {
 	switch (expr->exthdr.op) {
 	case NFT_EXTHDR_OP_TCPOPT:
-		__payload_dependency_kill(ctx, PROTO_BASE_TRANSPORT_HDR, family);
+		if (payload_dependency_exists(ctx, PROTO_BASE_TRANSPORT_HDR))
+			__payload_dependency_kill(ctx, family);
 		break;
 	case NFT_EXTHDR_OP_IPV6:
-		__payload_dependency_kill(ctx, PROTO_BASE_NETWORK_HDR, family);
+		if (payload_dependency_exists(ctx, PROTO_BASE_NETWORK_HDR))
+			__payload_dependency_kill(ctx, family);
 		break;
 	default:
 		break;
-- 
2.11.0

[PATCH nft 4/6] src: get rid of __payload_dependency_kill()

[Pablo Neira Ayuso]

Use payload_dependency_release() instead.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/payload.h         |  3 +--
 src/netlink_delinearize.c |  9 +++------
 src/payload.c             | 15 +++++----------
 3 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/include/payload.h b/include/payload.h
index dec4647aad9f..161c64aedf11 100644
--- a/include/payload.h
+++ b/include/payload.h
@@ -42,8 +42,7 @@ 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 void __payload_dependency_kill(struct payload_dep_ctx *ctx,
-				      unsigned int family);
+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);
 extern void exthdr_dependency_kill(struct payload_dep_ctx *ctx,
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index f4b943964fc9..b9f195b07d63 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1408,8 +1408,7 @@ static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx,
 			payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
 		} else if (ctx->pdctx.pbase < PROTO_BASE_TRANSPORT_HDR) {
 			if (payload_dependency_exists(&ctx->pdctx, base))
-				__payload_dependency_kill(&ctx->pdctx,
-							  ctx->pctx.family);
+				payload_dependency_release(&ctx->pdctx);
 			if (left->flags & EXPR_F_PROTOCOL)
 				payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
 		}
@@ -1874,8 +1873,7 @@ 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_kill(&rctx->pdctx,
-						  rctx->pctx.family);
+			payload_dependency_release(&rctx->pdctx);
 		break;
 	case NFPROTO_IPV6:
 		stmt->reject.family = rctx->pctx.family;
@@ -1883,8 +1881,7 @@ 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_kill(&rctx->pdctx,
-						  rctx->pctx.family);
+			payload_dependency_release(&rctx->pdctx);
 		break;
 	case NFPROTO_INET:
 		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH) {
diff --git a/src/payload.c b/src/payload.c
index df59ac8adcac..383aed039a5e 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -445,7 +445,7 @@ bool payload_dependency_exists(const struct payload_dep_ctx *ctx,
 	       ctx->pdep != NULL;
 }
 
-static void payload_dependency_release(struct payload_dep_ctx *ctx)
+void payload_dependency_release(struct payload_dep_ctx *ctx)
 {
 	list_del(&ctx->pdep->list);
 	stmt_free(ctx->pdep);
@@ -457,7 +457,7 @@ static void payload_dependency_release(struct payload_dep_ctx *ctx)
 }
 
 /**
- * __payload_dependency_kill - kill a redundant payload depedency
+ * payload_dependency_kill - kill a redundant payload depedency
  *
  * @ctx: payload dependency context
  * @expr: higher layer payload expression
@@ -465,16 +465,11 @@ static void payload_dependency_release(struct payload_dep_ctx *ctx)
  * Kill a redundant payload expression if a higher layer payload expression
  * implies its existance.
  */
-void __payload_dependency_kill(struct payload_dep_ctx *ctx, unsigned int family)
-{
-	payload_dependency_release(ctx);
-}
-
 void payload_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 			     unsigned int family)
 {
 	if (payload_dependency_exists(ctx, expr->payload.base))
-		__payload_dependency_kill(ctx, family);
+		payload_dependency_release(ctx);
 }
 
 void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
@@ -483,11 +478,11 @@ 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_kill(ctx, family);
+			payload_dependency_release(ctx);
 		break;
 	case NFT_EXTHDR_OP_IPV6:
 		if (payload_dependency_exists(ctx, PROTO_BASE_NETWORK_HDR))
-			__payload_dependency_kill(ctx, family);
+			payload_dependency_release(ctx);
 		break;
 	default:
 		break;
-- 
2.11.0

[PATCH nft 5/6] payload: add payload_may_dependency_kill()

[Pablo Neira Ayuso]

Payload protocol key expressions at network base are meaningful in the
netdev, bridge and inet families, do not exercise the redundant
dependency removal in those cases since it breaks rule semantics.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/payload.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/src/payload.c b/src/payload.c
index 383aed039a5e..15d055b6be7e 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -456,6 +456,31 @@ void payload_dependency_release(struct payload_dep_ctx *ctx)
 	ctx->pdep  = NULL;
 }
 
+static bool payload_may_dependency_kill(struct payload_dep_ctx *ctx,
+					unsigned int family, struct expr *expr)
+{
+	struct expr *dep = ctx->pdep->expr;
+
+	/* Protocol key payload expression at network base such as 'ip6 nexthdr'
+	 * need to be left in place since it implicitly restricts matching to
+	 * IPv6 for the bridge, inet and netdev families.
+	 */
+	switch (family) {
+	case NFPROTO_BRIDGE:
+	case NFPROTO_NETDEV:
+	case NFPROTO_INET:
+		if (dep->left->ops->type == EXPR_PAYLOAD &&
+		    dep->left->payload.base == PROTO_BASE_NETWORK_HDR &&
+		    (dep->left->payload.desc == &proto_ip ||
+		     dep->left->payload.desc == &proto_ip6) &&
+		    expr->payload.base == PROTO_BASE_TRANSPORT_HDR)
+			return false;
+		break;
+	}
+
+	return true;
+}
+
 /**
  * payload_dependency_kill - kill a redundant payload depedency
  *
@@ -463,12 +488,14 @@ void payload_dependency_release(struct payload_dep_ctx *ctx)
  * @expr: higher layer payload expression
  *
  * Kill a redundant payload expression if a higher layer payload expression
- * implies its existance.
+ * implies its existance. Skip this if the dependency is a network payload and
+ * we are in bridge, netdev and inet families.
  */
 void payload_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 			     unsigned int family)
 {
-	if (payload_dependency_exists(ctx, expr->payload.base))
+	if (payload_dependency_exists(ctx, expr->payload.base) &&
+	    payload_may_dependency_kill(ctx, family, expr))
 		payload_dependency_release(ctx);
 }
 
-- 
2.11.0

[PATCH nft 6/6] netlink_delinearize: add meta_may_dependency_kill()

[Pablo Neira Ayuso]

Do not exercise dependency removal for protocol key network payload
expressions in bridge, netdev and inet families from meta expressions,
more specifically:

* inet: nfproto and ether type.
* netdev and bridge: meta protocol and ether type.

need to be left in place.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/netlink_delinearize.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index b9f195b07d63..82eefa61d4a4 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1389,6 +1389,74 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
 	}
 }
 
+/* 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
+ * families. Exceptions are ICMP and ICMPv6 where this code assumes that can
+ * only happen with IPv4 and IPv6.
+ */
+static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
+				     unsigned int family,
+				     const struct expr *expr)
+{
+	struct expr *dep = ctx->pdep->expr;
+
+	if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
+		return true;
+
+	switch (family) {
+	case NFPROTO_INET:
+		switch (dep->left->ops->type) {
+		case EXPR_META:
+			if (dep->left->meta.key == NFT_META_NFPROTO &&
+			    (mpz_get_uint16(dep->right->value) == NFPROTO_IPV4 ||
+			     mpz_get_uint16(dep->right->value) == NFPROTO_IPV6) &&
+			    expr->left->meta.key == NFT_META_L4PROTO &&
+			    mpz_get_uint8(expr->right->value) != IPPROTO_ICMP &&
+			    mpz_get_uint8(expr->right->value) != IPPROTO_ICMPV6)
+				return false;
+			break;
+		case EXPR_PAYLOAD:
+			if (dep->left->payload.base == PROTO_BASE_LL_HDR &&
+			    (mpz_get_uint16(dep->right->value) == ETH_P_IP ||
+			     mpz_get_uint16(dep->right->value) == ETH_P_IPV6) &&
+			    expr->left->meta.key == NFT_META_L4PROTO &&
+			    mpz_get_uint8(expr->right->value) != IPPROTO_ICMP &&
+			    mpz_get_uint8(expr->right->value) != IPPROTO_ICMPV6)
+				return false;
+			break;
+		default:
+			break;
+		}
+		break;
+	case NFPROTO_NETDEV:
+	case NFPROTO_BRIDGE:
+		switch (dep->left->ops->type) {
+		case EXPR_META:
+			if (dep->left->meta.key == NFT_META_PROTOCOL &&
+			    (mpz_get_uint16(dep->right->value) == ETH_P_IP ||
+			     mpz_get_uint16(dep->right->value) == ETH_P_IPV6) &&
+			    expr->left->meta.key == NFT_META_L4PROTO &&
+			    mpz_get_uint8(expr->right->value) != IPPROTO_ICMP &&
+			    mpz_get_uint8(expr->right->value) != IPPROTO_ICMPV6)
+				return false;
+			break;
+		case EXPR_PAYLOAD:
+			if (dep->left->payload.base == PROTO_BASE_LL_HDR &&
+			    (mpz_get_uint16(dep->right->value) == ETH_P_IP ||
+			     mpz_get_uint16(dep->right->value) == ETH_P_IPV6) &&
+			    expr->left->meta.key == NFT_META_L4PROTO &&
+			    mpz_get_uint8(expr->right->value) != IPPROTO_ICMP &&
+			    mpz_get_uint8(expr->right->value) != IPPROTO_ICMPV6)
+				return false;
+			break;
+		default:
+			break;
+		}
+		break;
+	}
+	return true;
+}
+
 static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx,
 				       const struct expr *expr,
 				       enum proto_bases base)
@@ -1407,7 +1475,9 @@ static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx,
 		    left->flags & EXPR_F_PROTOCOL) {
 			payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
 		} else if (ctx->pdctx.pbase < PROTO_BASE_TRANSPORT_HDR) {
-			if (payload_dependency_exists(&ctx->pdctx, base))
+			if (payload_dependency_exists(&ctx->pdctx, base) &&
+			    meta_may_dependency_kill(&ctx->pdctx,
+						     ctx->pctx.family, expr))
 				payload_dependency_release(&ctx->pdctx);
 			if (left->flags & EXPR_F_PROTOCOL)
 				payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
-- 
2.11.0

Re: [PATCH nft 0/6] rework dependency removal (v2)

[Pablo Neira Ayuso]

On Wed, Feb 14, 2018 at 04:22:23PM +0100, Pablo Neira Ayuso wrote:
> Hi,
> 
> This patchset aims to address what Florian reported time ago [1]. This
> is skipping removal of protocol key payload expressions at network base
> for the netdev, bridge and inet.
> 
> It would better to annotate all redundant expressions and add a later
> stage, where we can do smarter simplifications by looking globally at
> what we have, instead of just looking at current protocol key expression
> and last one that we have annotated in the context structure to perform
> removals. But I would prefer to have a fix now upstream then look at
> this larger rework later on since it would require to review a bit of
> the postprocess code.
> 
> The initial 4 patches in this batch are just cleanup/preparation patches
> for patches 5/6 and 6/6.
> 
> There is still a few warning in the tests/py/ infrastructure, some of
> them I think need to be fixed, and at least one can remain there to
> remind us that we can do better.

I'm going to push out this so we start giving it some testing.