Re: [PATCH v2 net] tcp: cubic: avoid looping in bictcp_update() TCP friendliness code

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] tcp_cong_avoid_ai() (net/ipv4/tcp_cong.c:480) runs `u32 delta =
  tp->snd_cwnd_cnt / w;` and never checks that w is non-zero.
- [Medium] net/ipv4/tcp_roccet.c has its own copy of bictcp_update(),
  derived from CUBIC, with the same TCP-friendliness loop this patch…
- [Low] cubictcp_register() (net/ipv4/tcp_cubic.c:527-528) has the
  comment `/* bictcp_update() needs (cwnd * beta_scale) >> 3 to be >= 1…

--

tcp: cubic: avoid looping in bictcp_update() TCP friendliness code

This commit replaces the subtract-and-increment loop in the TCP
friendliness part of bictcp_update() with one division, and clamps delta
to at least 1. It addresses a CPU hang that happens when a BPF congestion
control stores arbitrary values in snd_cwnd and icsk_ca_priv and then
calls cubictcp_cong_avoid() or switches the socket to CUBIC.

Link: https://lore.kernel.org/netdev/179363987528.3131.2987776489427079887@kernel.org/

> diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
> index a88e4bf86150f..e74dabde34324 100644
> --- a/net/ipv4/tcp_cubic.c
> +++ b/net/ipv4/tcp_cubic.c
> @@ -298,10 +298,16 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked)
>  	if (tcp_friendliness) {
>  		u32 scale = beta_scale;
>  
> -		delta = (cwnd * scale) >> 3;
> -		while (ca->ack_cnt > delta) {		/* update tcp cwnd */
> -			ca->ack_cnt -= delta;
> -			ca->tcp_cwnd++;

[Severity: Medium]
net/ipv4/tcp_roccet.c has its own CUBIC-derived bictcp_update(), and it
still has the same loop:

net/ipv4/tcp_roccet.c:bictcp_update() {
	...
		delta = (cwnd * scale) >> 3;
		if (delta > 0) {
			while (ca->ack_cnt > delta) { /* update tcp cwnd */
				ca->ack_cnt -= delta;
				ca->tcp_cwnd++;
			}
		}
	...
}

The delta > 0 check rules out a zero delta. Does the huge ack_cnt case
from the commit message still apply there?

sol_tcp_sockopt_congestion() in net/core/filter.c blocks only "cdg". A
BPF CC can therefore call bpf_setsockopt(TCP_CONGESTION, "roccet") and
then overwrite icsk_ca_priv with epoch_start != 0 and
ack_cnt = 0xFFFFFFFF. With the default beta, beta_scale is 15, so
snd_cwnd = 1 gives delta == 1.

The next ACK reaches roccettcp_cong_avoid()->bictcp_update(), and the loop
runs about 2^32 times in ACK processing context.

Should the same max(..., 1U) plus division change go into the ROCCET copy,
perhaps as a companion patch?

> +		/* BPF congestion controls can call us with any cwnd
> +		 * and ack_cnt: cwnd * scale can wrap and give a zero delta,
> +		 * and a loop could run up to 2^32 times.
> +		 */
> +		delta = max((cwnd * scale) >> 3, 1U);

[Severity: Low]
This isn't a bug, but now that delta is clamped here, is the comment in
cubictcp_register() still accurate?

net/ipv4/tcp_cubic.c:cubictcp_register() {
	...
	/* bictcp_update() needs (cwnd * beta_scale) >> 3 to be >= 1 */
	beta_scale = max(beta_scale, 8U);
	...
}

bictcp_update() now guarantees delta >= 1 on its own. The new comment also
says cwnd * scale can wrap to zero anyway.

The registration clamp is no longer needed for loop safety. It still
overrides the beta module parameter when beta < 512, which changes the
Reno emulation rate.

Could the clamp and its comment be dropped or reworded, or mentioned in
the new comment here?

> +		if (ca->ack_cnt > delta) {		/* update tcp cwnd */
> +			u32 n = (ca->ack_cnt - 1) / delta;
> +
> +			ca->ack_cnt -= n * delta;
> +			ca->tcp_cwnd += n;
>  		}
>  
>  		if (ca->tcp_cwnd > cwnd) {	/* if bic is slower than tcp */

[Severity: High]
Under the threat model in the commit message, can this still divide by
zero in tcp_cong_avoid_ai()? In that model, a BPF congestion control
writes arbitrary values into icsk_ca_priv and snd_cwnd and then calls the
cubictcp_cong_avoid() kfunc.

The early return in bictcp_update() skips the final clamp on ca->cnt:

net/ipv4/tcp_cubic.c:bictcp_update() {
	...
	if (ca->last_cwnd == cwnd &&
	    (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32)
		return;
	...
	ca->cnt = max(ca->cnt, 2U);
}

A BPF CC can set ca->cnt = 0, ca->last_cwnd = snd_cwnd and ca->last_time
to a future jiffies value. It can do this in its own priv area before
calling the kfunc, or after bpf_setsockopt(TCP_CONGESTION, "cubic").
cubictcp_cong_avoid() then passes the unclamped value as the divisor:

	bictcp_update(ca, tcp_snd_cwnd(tp), acked);
	tcp_cong_avoid_ai(tp, ca->cnt, acked);

tcp_cong_avoid_ai() divides by w without checking it:

net/ipv4/tcp_cong.c:tcp_cong_avoid_ai() {
	...
	tp->snd_cwnd_cnt += acked;
	if (tp->snd_cwnd_cnt >= w) {
		u32 delta = tp->snd_cwnd_cnt / w;
	...
}

With w == 0, both snd_cwnd_cnt >= w tests are always true, so the
division runs for any acked value. In NET_RX softirq context, would this
oops and end in a panic?

The ROCCET code already guards the same call:

net/ipv4/tcp_roccet.c:roccettcp_cong_avoid() {
	...
	tcp_cong_avoid_ai(tp, max(1, ca->cnt), acked);
}

There are also more direct routes to the same division:

- The tcp_cong_avoid_ai kfunc is registered in
  bpf_tcp_ca_check_kfunc_ids, so it can be called directly with w = 0.
  The verifier only checks that w is a scalar.
- tcp_reno_cong_avoid() passes the BPF-writable tcp_snd_cwnd(tp) as w.

Would a guard such as max(ca->cnt, 1U) in cubictcp_cong_avoid(), or a
clamp on w inside tcp_cong_avoid_ai(), be needed to fully close this bug
class?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109172356.3113732-1-edumazet%40kernel.org