Re: [PATCH] net/sched: act_ct: fix skb leak on fragment check failure

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net/sched: act_ct: fix skb leak on fragment check failure

This commit fixes a socket buffer leak in the act_ct traffic control action.
When tcf_ct_handle_fragments() encounters malformed fragments and returns an
error, the skb was not being freed, leading to memory exhaustion under
sustained traffic. The fix changes the return value to TC_ACT_SHOT to ensure
proper cleanup.

> net/sched: act_ct: fix skb leak on fragment check failure
>
> tcf_ct_handle_fragments() returns TC_ACT_CONSUMED when
> tcf_ct_ipv4/6_is_fragment() fails. This causes the caller to

The commit message contains a technical inaccuracy. It states
'tcf_ct_handle_fragments() returns TC_ACT_CONSUMED when
tcf_ct_ipv4/6_is_fragment() fails', but tcf_ct_handle_fragments() returns
integer error codes (like -EINVAL, -ENOMEM, -EPROTO), not TC action codes.

The function that returns TC_ACT_CONSUMED is tcf_ct_act(), which interprets
the error codes from tcf_ct_handle_fragments(). The code fix itself is
correct - only the commit message attribution is wrong.

More accurate would be: 'tcf_ct_act() returns TC_ACT_CONSUMED when
tcf_ct_handle_fragments() fails'.

> believe the skb was consumed, but it was not freed. Each
> malformed fragment leaks one skb, leading to OOM DoS under
> sustained traffic.
>
> Change the return value to TC_ACT_SHOT so the skb is properly
> freed by the caller.
>
> Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")

The existing Fixes: tag points to commit b57dc7c13ea9 ("net/sched: Introduce
action ct"), but the bug was actually introduced by commit 3f14b377d01d
("net/sched: act_ct: fix skb leak and crash on ooo frags").

The original commit b57dc7c13ea9 had correct error handling where fragment
check failures went to the 'drop' label. Commit 3f14b377d01d introduced the
'out_frag' label that always returns TC_ACT_CONSUMED, causing the skb leak
when fragment checks fail with errors other than -EINPROGRESS.

Suggested: Fixes: 3f14b377d01d ("net/sched: act_ct: fix skb leak and crash on ooo frags")

> Signed-off-by: Dudu Lu <phx0fer@gmail.com>
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 7d5e50c921a0..870655f682bd 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -1107,8 +1107,10 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  	return retval;
>
>  out_frag:
> -	if (err != -EINPROGRESS)
> +	if (err != -EINPROGRESS) {
>  		tcf_action_inc_drop_qstats(&c->common);
> +		return TC_ACT_SHOT;
> +	}
>  	return TC_ACT_CONSUMED;
>
>  drop: