Re: [RFC PATCH nft] payload: Don't kill dependency for proto_th

[Florian Westphal <fw@strlen.de>]

Xiao Liang <shaw.leon@gmail.com> wrote:
> Since proto_th carries no information about the proto number, we need to
> preserve the L4 protocol expression.
> 
> For example, if "meta l4proto 91 @th,0,16 0" is simplified to
> "th sport 0", the information of protocol number is lost. This patch
> changes it to "meta l4proto 91 th sport 0".
> 
> Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
> ---
> 
> Technically, if port is not defined for the L4 protocol, it's better to
> keep "@th,0,16" as raw payload expressions rather than "th sport". But
> it's not easy to figure out the context.

Yes, this is also because we don't store dependencies like
'meta l4proto { tcp, udp }', so we don't have access to this info when
we see the @th,0,16 load.

What do you make of this (it will display @th syntax for your test case)?

diff --git a/src/payload.c b/src/payload.c
--- a/src/payload.c
+++ b/src/payload.c
@@ -851,6 +851,58 @@ static bool payload_may_dependency_kill_ll(struct payload_dep_ctx *ctx, struct e
 	return true;
 }
 
+static bool l4proto_has_ports(struct payload_dep_ctx *ctx, struct expr *expr)
+{
+	uint8_t v;
+
+	assert(expr->etype == EXPR_VALUE);
+
+	if (expr->len != 8)
+		return false;
+
+	v = mpz_get_uint8(expr->value);
+
+	switch (v) {
+	case IPPROTO_UDPLITE:
+	case IPPROTO_UDP:
+	case IPPROTO_TCP:
+	case IPPROTO_DCCP:
+	case IPPROTO_SCTP:
+		return true;
+	}
+
+	return false;
+}
+
+static bool payload_may_dependency_kill_th(struct payload_dep_ctx *ctx, struct expr *expr)
+{
+	struct expr *dep = payload_dependency_get(ctx, expr->payload.base);
+	enum proto_bases b;
+
+	switch (dep->left->etype) {
+	case EXPR_PAYLOAD:
+		b = dep->left->payload.base;
+		break;
+	case EXPR_META:
+		b = dep->left->meta.base;
+		break;
+	default:
+		return false;
+	}
+
+	if (b != PROTO_BASE_NETWORK_HDR)
+		return false;
+
+	switch (dep->right->etype) {
+	case EXPR_VALUE:
+		return l4proto_has_ports(ctx, dep->right);
+	default:
+		break;
+	}
+
+	return false;
+}
+
 static bool payload_may_dependency_kill(struct payload_dep_ctx *ctx,
 					unsigned int family, struct expr *expr)
 {
@@ -893,6 +945,15 @@ static bool payload_may_dependency_kill(struct payload_dep_ctx *ctx,
 	if (expr->payload.base != PROTO_BASE_TRANSPORT_HDR)
 		return true;
 
+	if (expr->payload.desc == &proto_th) {
+		if (payload_may_dependency_kill_th(ctx, expr))
+			return true;
+
+		/* prefer @th syntax, we don't have a 'source/destination port' protocol */
+		expr->payload.desc = &proto_unknown;
+		return false;
+	}
+
 	if (dep->left->etype != EXPR_PAYLOAD ||
 	    dep->left->payload.base != PROTO_BASE_TRANSPORT_HDR)
 		return true;


It would make sense to NOT set &proto_th from payload_init_raw(), but
that would place significant burden on netlink_delinearize to
pretty-print the typical use case for this, i.e.

'meta l4proto { tcp, udp } th dport 53 accept'