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

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

[Xiao Liang]

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.

---
 src/payload.c                     |  1 +
 tests/py/any/rawpayload.t         |  1 +
 tests/py/any/rawpayload.t.json    | 31 +++++++++++++++++++++++++++++++
 tests/py/any/rawpayload.t.payload |  8 ++++++++
 4 files changed, 41 insertions(+)

diff --git a/src/payload.c b/src/payload.c
index f8b192b5..a039e242 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -920,6 +920,7 @@ void payload_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 			     unsigned int family)
 {
 	if (expr->payload.desc != &proto_unknown &&
+	    expr->payload.desc != &proto_th &&
 	    payload_dependency_exists(ctx, expr->payload.base) &&
 	    payload_may_dependency_kill(ctx, family, expr))
 		payload_dependency_release(ctx, expr->payload.base);
diff --git a/tests/py/any/rawpayload.t b/tests/py/any/rawpayload.t
index 745b4a61..4ef53f82 100644
--- a/tests/py/any/rawpayload.t
+++ b/tests/py/any/rawpayload.t
@@ -21,6 +21,7 @@ meta l4proto tcp @th,16,16 { 22, 23, 80};ok;tcp dport { 22, 23, 80}
 @ll,0,128 0xfedcba987654321001234567890abcde;ok
 
 meta l4proto 91 @th,400,16 0x0 accept;ok
+meta l4proto 91 @th,0,16 0x0 accept;ok;meta l4proto 91 th sport 0 accept
 
 @ih,32,32 0x14000000;ok
 @ih,58,6 set 0 @ih,86,6 set 0 @ih,170,22 set 0;ok;@ih,58,6 set 0x0 @ih,86,6 set 0x0 @ih,170,22 set 0x0
diff --git a/tests/py/any/rawpayload.t.json b/tests/py/any/rawpayload.t.json
index 4a06c598..2d3c7904 100644
--- a/tests/py/any/rawpayload.t.json
+++ b/tests/py/any/rawpayload.t.json
@@ -187,6 +187,37 @@
     }
 ]
 
+# meta l4proto 91 @th,0,16 0x0 accept
+[
+    {
+        "match": {
+            "left": {
+                "meta": {
+                    "key": "l4proto"
+                }
+            },
+            "op": "==",
+            "right": 91
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sport",
+                    "protocol": "th"
+                }
+            },
+            "op": "==",
+            "right": 0
+        }
+    },
+    {
+        "accept": null
+    }
+]
+
+
 # @ih,32,32 0x14000000
 [
     {
diff --git a/tests/py/any/rawpayload.t.payload b/tests/py/any/rawpayload.t.payload
index 8984eef6..c093d5d8 100644
--- a/tests/py/any/rawpayload.t.payload
+++ b/tests/py/any/rawpayload.t.payload
@@ -56,6 +56,14 @@ inet test-inet input
   [ cmp eq reg 1 0x00000000 ]
   [ immediate reg 0 accept ]
 
+# meta l4proto 91 @th,0,16 0x0 accept
+inet test-inet input
+  [ meta load l4proto => reg 1 ]
+  [ cmp eq reg 1 0x0000005b ]
+  [ payload load 2b @ transport header + 0 => reg 1 ]
+  [ cmp eq reg 1 0x00000000 ]
+  [ immediate reg 0 accept ]
+
 # @ih,32,32 0x14000000
 inet test-inet input
   [ payload load 4b @ inner header + 4 => reg 1 ]
-- 
2.48.1

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

[Florian Westphal]

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'

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

[Xiao Liang]

On Wed, Feb 26, 2025 at 4:38 AM Florian Westphal <fw@strlen.de> wrote:
>
> 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)?

Looks good. I think this can cover most of the use cases. We don't
usually mix protocols when matching L4 header fields other than ports.
Thanks!

>
> 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'
>