On Wed, Feb 26, 2025 at 4:38 AM Florian Westphal <fw@xxxxxxxxx> wrote:
>
> Xiao Liang <shaw.leon@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >
> > 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'
>
