Re: [PATCH v5 net-next 1/1] tcp: Replace min_tso_segs() with tso_segs() CC callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing bounds check on `ca_ops->tso_segs()` return value causes an infinite loop and soft lockup
- [Medium] Exported function `tcp_tso_autosize` lacks BPF kfunc registration
--

--- Patch [1]: [PATCH v5 net-next 1/1] tcp: Replace min_tso_segs() with tso_segs() CC callback ---
commit 459bb4a577e547baf34fbc452c0b63bca5545c92
Author: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>

tcp: Replace min_tso_segs() with tso_segs() CC callback

This commit replaces the min_tso_segs() congestion control callback
with tso_segs(), passing the mss_now argument to allow the algorithm
to return an explicit TSO segment number. It bypasses tcp_tso_autosize()
when the callback is provided, pushing the sizing logic into the CC.

> As a result, BPF programs shall be updated, because retuning a small
> constan will now directly limit tso_segs instead of the minimum.

This isn't a bug, but there are typos in the commit message here: "retuning"
and "constan".

> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index d7c1444b5e304..101ca765a7a92 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c

[ ... ]

> @@ -2269,6 +2269,7 @@ static u32 tcp_tso_autosize(const struct sock *sk, unsigned int mss_now,
>  
>  	return max_t(u32, bytes / mss_now, min_tso_segs);
>  }
> +EXPORT_SYMBOL(tcp_tso_autosize);

[Severity: Medium]
Is this missing a BTF_ID_FLAGS entry for BPF congestion control algorithms?

The commit notes that BPF programs need to be updated to directly limit
tso_segs, which implies they might need to call tcp_tso_autosize() to
replicate the original behavior. While this EXPORT_SYMBOL makes it available
for loadable kernel modules, without adding it to the
bpf_tcp_ca_check_kfunc_ids BTF set, the BPF verifier will reject programs
attempting to use it.

>  /* Return the number of segments we want in the skb we are transmitting.
>   * See if congestion control module wants to decide; otherwise, autosize.
> @@ -2278,11 +2279,11 @@ static u32 tcp_tso_segs(struct sock *sk, unsigned int mss_now)
>  	const struct tcp_congestion_ops *ca_ops = inet_csk(sk)->icsk_ca_ops;
>  	u32 min_tso, tso_segs;
>  
> -	min_tso = ca_ops->min_tso_segs ?
> -			ca_ops->min_tso_segs(sk) :
> -			READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_min_tso_segs);
> +	min_tso = READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_min_tso_segs);
>  
> -	tso_segs = tcp_tso_autosize(sk, mss_now, min_tso);
> +	tso_segs = ca_ops->tso_segs ?
> +			ca_ops->tso_segs(sk, mss_now) :
> +			tcp_tso_autosize(sk, mss_now, min_tso);
>  	return min_t(u32, tso_segs, sk->sk_gso_max_segs);
>  }

[Severity: High]
What happens if the custom congestion control or BPF program returns 0 here?

If ca_ops->tso_segs() returns 0, tcp_tso_segs() will return 0, which gets
passed back to tcp_write_xmit() as max_segs.

In tcp_write_xmit(), this sets cwnd_quota to 0. When tcp_mss_split_point()
is called with a 0 cwnd_quota, it returns a limit of 0. This causes
tso_fragment() to chop 0 bytes off the SKB, resulting in a 0-byte packet
being sent.

Because the outer loop in tcp_write_xmit() doesn't re-evaluate max_segs,
won't this get stuck in an infinite loop chopping 0 bytes off the remainder
SKB, leading to a CPU soft lockup? Does this need a bounds check to clamp
the minimum return value to 1?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728152425.649600-1-chia-yu.chang@nokia-bell-labs.com?part=1