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

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

[Eric Dumazet]

The TCP friendliness part of bictcp_update() does:

	delta = (cwnd * scale) >> 3;
	while (ca->ack_cnt > delta) {
		ca->ack_cnt -= delta;
		ca->tcp_cwnd++;
	}

A BPF congestion control can store any value in tp->snd_cwnd,
tp->snd_ssthresh and icsk_ca_priv, then call the cubictcp_cong_avoid()
kfunc or switch the socket to CUBIC. A zero delta (or a huge ack_cnt)
then hangs the CPU in ACK processing.

Make sure delta is at least 1, and replace the loop with a division,
done only when ack_cnt > delta (at most about once per RTT).

Fixes: 0baf26b0fcd7 ("bpf: tcp: Support tcp_congestion_ops in bpf")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/netdev/179363987528.3131.2987776489427079887@kernel.org/
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
v2: fix the loop itself instead of sanitizing snd_cwnd at entry points.
v1: https://lore.kernel.org/netdev/20261106133748.2181133-1-edumazet@kernel.org/

Discovered by an AI code review agent (Sashiko), confirmed by
code inspection only, not triggered.

 net/ipv4/tcp_cubic.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index a88e4bf86150f06eb3c0d3da99761e5d45c8f6a3..e74dabde34324d4d4764afda7022fafcdbcecbfe 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++;
+		/* 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);
+		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 */
-- 
2.53.0

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

[netdev-bot+sashiko]

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

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

[Eric Dumazet]

Le mar. 10 nov. 2026 à 21:24, <netdev-bot+sashiko@kernel.org> a écrit :
>
> 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

Ok, I do not think we can fix this then.

eBPF is what it is.

pw-bot: cr