[PATCH net] tcp: cubic: do not trust snd_cwnd set by BPF congestion controls

[PATCH net] tcp: cubic: do not trust snd_cwnd set by BPF congestion controls

[Eric Dumazet]

Follow up of 475ff14ba558 ("tcp: bound snd_cwnd_clamp to TCP_MAX_SND_CWND")

The TCP friendliness part of bictcp_update() does:

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

beta_scale is at least 8, so delta is 0 only if cwnd is 0,
or if the u32 product cwnd * scale wraps. Native paths keep
cwnd in [1, TCP_MAX_SND_CWND], where this can not happen,
even with the largest valid beta (beta_scale = 5458).

A BPF congestion control (struct_ops) can store any u32 value
in tp->snd_cwnd and tp->snd_ssthresh, then:

- call the cubictcp_cong_avoid() kfunc.

- switch the socket to CUBIC with TCP_CONGESTION,
  as tcp_reinit_congestion_control() keeps snd_cwnd.

With snd_cwnd = 0 (or 0x88888889 with the default beta_scale of 15)
and snd_ssthresh low enough to leave slow start, delta is 0 and
the loop never ends, hanging the CPU in ACK processing.

Adding a check in bictcp_update() would cost native CUBIC a
conditional branch in its fast path.

Instead, fix the two entry points:

- The cubictcp_cong_avoid() kfunc becomes a wrapper ignoring
  calls with a cwnd outside [1, TCP_MAX_SND_CWND].
  Native CUBIC uses __cubictcp_cong_avoid(), marked noinline
  so that its generated code does not change.

- tcp_reinit_congestion_control() bounds snd_cwnd to
  [1, snd_cwnd_clamp]. This also protects other native
  congestion controls, whose ->ssthresh() handlers do u32
  multiplications on cwnd.

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>
---
 net/ipv4/tcp_cong.c  |  7 +++++++
 net/ipv4/tcp_cubic.c | 18 ++++++++++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index e3f462f161dd6c3f4e9eeae99b45a93391c87079..2c20524489168da0062bf7ff1b8bfc69e2c8b6de 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -251,8 +251,15 @@ static void tcp_reinit_congestion_control(struct sock *sk,
 					  const struct tcp_congestion_ops *ca)
 {
 	struct inet_connection_sock *icsk = inet_csk(sk);
+	struct tcp_sock *tp = tcp_sk(sk);
 
 	tcp_cleanup_congestion_control(sk);
+
+	/* A BPF congestion control could have stored any value
+	 * in tp->snd_cwnd. Native ones expect a sane value.
+	 */
+	tcp_snd_cwnd_set(tp, clamp(tcp_snd_cwnd(tp), 1U, tp->snd_cwnd_clamp));
+
 	WRITE_ONCE(icsk->icsk_ca_ops, ca);
 	icsk->icsk_ca_setsockopt = 1;
 	memset(icsk->icsk_ca_priv, 0, sizeof(icsk->icsk_ca_priv));
diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index a88e4bf86150f06eb3c0d3da99761e5d45c8f6a3..9595bdc9589c255b673744ce501f50c418d82e2b 100644
--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c
@@ -318,7 +318,7 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked)
 	ca->cnt = max(ca->cnt, 2U);
 }
 
-__bpf_kfunc static void cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
+static noinline void __cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct bictcp *ca = inet_csk_ca(sk);
@@ -335,6 +335,20 @@ __bpf_kfunc static void cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
 	tcp_cong_avoid_ai(tp, ca->cnt, acked);
 }
 
+/* BPF congestion controls can store any value in tp->snd_cwnd.
+ * bictcp_update() would loop forever with cwnd == 0, or with a cwnd
+ * large enough for cwnd * beta_scale to wrap.
+ * Native callers do not need this check.
+ */
+__bpf_kfunc static void cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
+{
+	u32 cwnd = tcp_snd_cwnd(tcp_sk(sk));
+
+	if (unlikely(!cwnd || cwnd > TCP_MAX_SND_CWND))
+		return;
+	__cubictcp_cong_avoid(sk, ack, acked);
+}
+
 __bpf_kfunc static u32 cubictcp_recalc_ssthresh(struct sock *sk)
 {
 	const struct tcp_sock *tp = tcp_sk(sk);
@@ -475,7 +489,7 @@ __bpf_kfunc static void cubictcp_acked(struct sock *sk, const struct ack_sample
 static struct tcp_congestion_ops cubictcp __read_mostly = {
 	.init		= cubictcp_init,
 	.ssthresh	= cubictcp_recalc_ssthresh,
-	.cong_avoid	= cubictcp_cong_avoid,
+	.cong_avoid	= __cubictcp_cong_avoid,
 	.set_state	= cubictcp_state,
 	.undo_cwnd	= tcp_reno_undo_cwnd,
 	.cwnd_event_tx_start = cubictcp_cwnd_event_tx_start,
-- 
2.53.0

Re: [PATCH net] tcp: cubic: do not trust snd_cwnd set by BPF congestion controls

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] tcp: cubic: do not trust snd_cwnd set by BPF congestion controls

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (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] The fix is incomplete: a BPF congestion control can still stall
  the CPU in the TCP-friendliness loop of bictcp_update() through the…
- [High] tcp: unsanitized snd_ssthresh in tcp_reinit_congestion_control()
  hangs CUBIC
- [Medium] tcp: BPF CC can overwrite snd_cwnd after
  bpf_setsockopt(TCP_CONGESTION)
- [Low] The new comment above the cubictcp_cong_avoid() wrapper
  (net/ipv4/tcp_cubic.c) says bictcp_update() 'would loop forever ...…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is in the
  same 'BPF can store any value' threat model and is High severity.

--

tcp: cubic: do not trust snd_cwnd set by BPF congestion controls

This patch turns the cubictcp_cong_avoid() kfunc into a wrapper. The
wrapper ignores calls when snd_cwnd is outside [1, TCP_MAX_SND_CWND],
while native CUBIC calls __cubictcp_cong_avoid() directly. It also
clamps snd_cwnd to [1, snd_cwnd_clamp] in
tcp_reinit_congestion_control(), to stop the TCP friendliness loop in
bictcp_update() from hanging the CPU.

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..9595bdc9589c2 100644
> --- a/net/ipv4/tcp_cubic.c
> +++ b/net/ipv4/tcp_cubic.c
> @@ -318,7 +318,7 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked)
>  	ca->cnt = max(ca->cnt, 2U);
>  }
>  
> -__bpf_kfunc static void cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
> +static noinline void __cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
>  {
>  	struct tcp_sock *tp = tcp_sk(sk);
>  	struct bictcp *ca = inet_csk_ca(sk);
> @@ -335,6 +335,20 @@ __bpf_kfunc static void cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
>  	tcp_cong_avoid_ai(tp, ca->cnt, acked);

[Severity: High]
Does clamping only snd_cwnd here still leave the same hang reachable
through tp->snd_ssthresh?

A BPF congestion control can write any u32 to tp->snd_ssthresh via
bpf_tcp_ca_btf_struct_access(). That includes its .release()
callback, which tcp_cleanup_congestion_control() runs just before
this clamp. It can also return any value from .ssthresh(), which
tcp_init_cwnd_reduction() and tcp_enter_loss() store into
tp->snd_ssthresh. A BPF .ssthresh() can simply return 0.

tcp_reinit_congestion_control() does not touch snd_ssthresh,
icsk_ca_state or tp->undo_marker. With the default hystart and
initial_ssthresh parameters, cubictcp_init() leaves snd_ssthresh
alone as well.

Suppose the socket is in TCP_CA_CWR or TCP_CA_Recovery when it is
moved to CUBIC. The next ACK that advances snd_una past high_seq then
goes through tcp_fastretrans_alert()->tcp_end_cwnd_reduction():

net/ipv4/tcp_input.c:tcp_end_cwnd_reduction() {
    ...
	if (tp->snd_ssthresh < TCP_INFINITE_SSTHRESH &&
	    (inet_csk(sk)->icsk_ca_state == TCP_CA_CWR || tp->undo_marker)) {
		tcp_snd_cwnd_set(tp, tp->snd_ssthresh);
		tp->snd_cwnd_stamp = tcp_jiffies32;
	}
    ...
}

This copies snd_ssthresh into snd_cwnd without any bound.
tcp_snd_cwnd_set() warns once on 0 but still stores it.

Later in the same tcp_ack(), tcp_cong_control() calls
__cubictcp_cong_avoid() with snd_cwnd == 0 and snd_ssthresh == 0.
tcp_in_slow_start() is false, so bictcp_update(ca, 0, acked)
computes delta = 0 and the TCP friendliness loop never terminates.

A snd_ssthresh between TCP_MAX_SND_CWND and TCP_INFINITE_SSTHRESH
would instead leave snd_cwnd above TCP_MAX_SND_CWND. There
cwnd * beta_scale wraps and the u32 multiplications in native
->ssthresh() handlers overflow, which is the state the commit message
says native congestion controls must not see.

Should tcp_reinit_congestion_control() also bound snd_ssthresh, for
example to snd_cwnd_clamp or by resetting it to
TCP_INFINITE_SSTHRESH? Or should it reset the CWR/Recovery state
when switching away from a BPF congestion control? Or is there
something that keeps tcp_end_cwnd_reduction() from running after
such a switch that I am missing?

[Severity: High]

This isn't a bug introduced by this patch, but it falls under the same
"BPF can store any value" threat model. tcp_cong_avoid_ai() is exposed as
a kfunc through bpf_tcp_ca_check_kfunc_ids. It divides by w without
checking for zero:

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;
    ...
}

When w == 0 the guard is always true.

Can a BPF congestion control reach this division by zero in any of these
ways?

- Call tcp_cong_avoid_ai(tp, 0, acked) directly. The verifier does not
  range check scalar kfunc arguments.

- Store snd_cwnd = 0 and snd_ssthresh = 0, then call the
  tcp_reno_cong_avoid() kfunc. It passes tcp_snd_cwnd(tp) as w and does
  not check cwnd.

- Use the new cubictcp_cong_avoid() wrapper with a valid snd_cwnd, after
  writing ca->cnt = 0, ca->last_cwnd = snd_cwnd and ca->last_time = a
  recent tcp_jiffies32 value (readable from fields such as
  tp->lsndtime). bictcp_update() then returns early, before
  ca->cnt = max(ca->cnt, 2U), and __cubictcp_cong_avoid() calls
  tcp_cong_avoid_ai(tp, 0, acked).

On x86 this would raise a divide error during ACK processing, usually in
softirq context. Native callers never pass w == 0.

>  }
>  
> +/* BPF congestion controls can store any value in tp->snd_cwnd.
> + * bictcp_update() would loop forever with cwnd == 0, or with a cwnd
> + * large enough for cwnd * beta_scale to wrap.

[Severity: Low]

This isn't a bug, but is this comment accurate? It suggests that the
TCP_MAX_SND_CWND check rules out every cwnd for which cwnd * beta_scale
wraps.

cubictcp_register() accepts beta up to 1023, which gives
beta_scale = 8 * 2047 / 3 / 1 = 5458. With that value the u32 product
wraps for any cwnd above 786912. So every cwnd in [786913, 1048576]
passes the wrapper and still wraps.

In that range the product wraps only once and delta stays at least 482,
so the infinite loop is still prevented. The comment, though, treats
"wraps" as if it meant "loops forever", and the wrap still produces a
much smaller delta than intended.

The TCP_MAX_SND_CWND comment in include/net/tcp.h (from 475ff14ba558)
says multipliers go "up to 2047", which does not account for beta_scale
in bictcp_update().

> + * Native callers do not need this check.
> + */
> +__bpf_kfunc static void cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
> +{
> +	u32 cwnd = tcp_snd_cwnd(tcp_sk(sk));
> +
> +	if (unlikely(!cwnd || cwnd > TCP_MAX_SND_CWND))
> +		return;
> +	__cubictcp_cong_avoid(sk, ack, acked);

[Severity: High]

Does this wrapper fully close the hang in the bictcp_update() TCP
friendliness loop? It checks only snd_cwnd. The loop runs about
ack_cnt / delta times, and ack_cnt comes either from the acked argument,
which the BPF program chooses, or from struct bictcp state in
icsk_ca_priv. BPF is allowed to write that state:

net/ipv4/bpf_tcp_ca.c:bpf_tcp_ca_btf_struct_access() {
    ...
	case bpf_ctx_range(struct inet_connection_sock, icsk_ca_priv):
		end = offsetofend(struct inet_connection_sock, icsk_ca_priv);
    ...
}

snd_cwnd = 1 passes the check and gives delta = (1 * 15) >> 3 = 1 with
the default beta_scale. Suppose a struct_ops program stores
snd_cwnd = 1, snd_ssthresh <= 1 (so tcp_in_slow_start() is false and
acked is not consumed), ca->epoch_start = 0 and ca->last_cwnd != 1, and
then calls:

    cubictcp_cong_avoid(sk, 0, 0xffffffff);

The call chain is
cubictcp_cong_avoid()->__cubictcp_cong_avoid()->bictcp_update(ca, 1, 0xffffffff).
The epoch_start == 0 branch sets ca->ack_cnt = 0xffffffff, and then
this loop runs about 2^32 times during ACK processing with the socket
lock held:

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

beta_scale is a runtime global, so the compiler is not expected to turn
this loop into a division.

The program can set up that state again and call the kfunc repeatedly,
either unrolled or through bpf_loop(). Would that keep the CPU stuck
past the soft lockup and RCU stall thresholds, which is the same hang
in ACK processing the commit message describes?

Could the kfunc wrapper also bound acked (and/or ca->ack_cnt), or could
the BPF path compute the friendliness update with a division instead of
the loop?

> +}
> +

[ ... ]

> @@ -475,7 +489,7 @@ __bpf_kfunc static void cubictcp_acked(struct sock *sk, const struct ack_sample
>  static struct tcp_congestion_ops cubictcp __read_mostly = {
>  	.init		= cubictcp_init,
>  	.ssthresh	= cubictcp_recalc_ssthresh,
> -	.cong_avoid	= cubictcp_cong_avoid,
> +	.cong_avoid	= __cubictcp_cong_avoid,
>  	.set_state	= cubictcp_state,
>  	.undo_cwnd	= tcp_reno_undo_cwnd,
>  	.cwnd_event_tx_start = cubictcp_cwnd_event_tx_start,

[Severity: Medium]
Can a BPF congestion control still hand native CUBIC an invalid
snd_cwnd after the clamp in tcp_reinit_congestion_control() has run?

bpf_tcp_ca_get_func_proto() allows bpf_setsockopt() from every
tcp_congestion_ops callback except .release. So a BPF .cong_avoid,
.in_ack_event, .pkts_acked, .set_state or .undo_cwnd can call
bpf_setsockopt(sk, SOL_TCP, TCP_CONGESTION, "cubic", ...).

That call goes through sol_tcp_sockopt_congestion(),
do_tcp_setsockopt() and tcp_set_congestion_control() into
tcp_reinit_congestion_control(). That function clamps snd_cwnd and
switches icsk_ca_ops to cubictcp synchronously. Control then returns
to the BPF callback, which is still running.

The callback still has write access to tp->snd_cwnd and
tp->snd_ssthresh through bpf_tcp_ca_btf_struct_access(). It can
store snd_cwnd = 0 (or 0x88888889) and snd_ssthresh = 0 right after
bpf_setsockopt() returns. sol_tcp_sockopt_congestion() already
carries a comment about a similar after-the-switch hazard for cdg.

The .undo_cwnd case does not even need a direct write:

net/ipv4/tcp_input.c:tcp_undo_cwnd_reduction() {
    ...
		tcp_snd_cwnd_set(tp, icsk->icsk_ca_ops->undo_cwnd(sk));
    ...
}

The value returned by the BPF .undo_cwnd() is stored after that same
call has already switched icsk_ca_ops to cubictcp.

The hang then follows on the next ACK, or later in the same tcp_ack()
when the switch is done from .in_ack_event or .pkts_acked.
__cubictcp_cong_avoid() runs bictcp_update() with cwnd == 0 or
0x88888889, so delta is 0. The TCP friendliness loop then spins
forever in ACK processing, usually from softirq with the socket
spinlock held.

__cubictcp_cong_avoid() now relies on snd_cwnd being sane after
tcp_reinit_congestion_control(). Given this ordering, is that the
right place for the check? Would it be better to reject
TCP_CONGESTION from bpf_setsockopt() inside tcp_congestion_ops
callbacks, the way cdg is already rejected? Or to validate snd_cwnd
at a point the BPF program cannot run after?

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

Re: [PATCH net] tcp: cubic: do not trust snd_cwnd set by BPF congestion controls

[Eric Dumazet]

Le lun. 9 nov. 2026 à 17:40, <netdev-bot+sashiko@kernel.org> a écrit :
>
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 (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] The fix is incomplete: a BPF congestion control can still stall
>   the CPU in the TCP-friendliness loop of bictcp_update() through the…
> - [High] tcp: unsanitized snd_ssthresh in tcp_reinit_congestion_control()
>   hangs CUBIC
> - [Medium] tcp: BPF CC can overwrite snd_cwnd after
>   bpf_setsockopt(TCP_CONGESTION)
> - [Low] The new comment above the cubictcp_cong_avoid() wrapper
>   (net/ipv4/tcp_cubic.c) says bictcp_update() 'would loop forever ...…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but it is in the
>   same 'BPF can store any value' threat model and is High severity.
>
> --
>
> tcp: cubic: do not trust snd_cwnd set by BPF congestion controls
>
> This patch turns the cubictcp_cong_avoid() kfunc into a wrapper. The
> wrapper ignores calls when snd_cwnd is outside [1, TCP_MAX_SND_CWND],
> while native CUBIC calls __cubictcp_cong_avoid() directly. It also
> clamps snd_cwnd to [1, snd_cwnd_clamp] in
> tcp_reinit_congestion_control(), to stop the TCP friendliness loop in
> bictcp_update() from hanging the CPU.
>
> 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..9595bdc9589c2 100644
> > --- a/net/ipv4/tcp_cubic.c
> > +++ b/net/ipv4/tcp_cubic.c
> > @@ -318,7 +318,7 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked)
> >       ca->cnt = max(ca->cnt, 2U);
> >  }
> >
> > -__bpf_kfunc static void cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
> > +static noinline void __cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
> >  {
> >       struct tcp_sock *tp = tcp_sk(sk);
> >       struct bictcp *ca = inet_csk_ca(sk);
> > @@ -335,6 +335,20 @@ __bpf_kfunc static void cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
> >       tcp_cong_avoid_ai(tp, ca->cnt, acked);
>
> [Severity: High]
> Does clamping only snd_cwnd here still leave the same hang reachable
> through tp->snd_ssthresh?

Yes, and the Medium issue below (writes done after
bpf_setsockopt(TCP_CONGESTION) from a BPF callback) shows that
sanitizing snd_cwnd at entry points can not be complete.

v2 will instead make the friendliness code in bictcp_update() safe
for any cwnd and ack_cnt values: delta is at least 1, and the loop
is replaced by a division, only done when ack_cnt > delta.

>
> A BPF congestion control can write any u32 to tp->snd_ssthresh via
> bpf_tcp_ca_btf_struct_access(). That includes its .release()
> callback, which tcp_cleanup_congestion_control() runs just before
> this clamp. It can also return any value from .ssthresh(), which
> tcp_init_cwnd_reduction() and tcp_enter_loss() store into
> tp->snd_ssthresh. A BPF .ssthresh() can simply return 0.
>
> tcp_reinit_congestion_control() does not touch snd_ssthresh,
> icsk_ca_state or tp->undo_marker. With the default hystart and
> initial_ssthresh parameters, cubictcp_init() leaves snd_ssthresh
> alone as well.
>
> Suppose the socket is in TCP_CA_CWR or TCP_CA_Recovery when it is
> moved to CUBIC. The next ACK that advances snd_una past high_seq then
> goes through tcp_fastretrans_alert()->tcp_end_cwnd_reduction():
>
> net/ipv4/tcp_input.c:tcp_end_cwnd_reduction() {
>     ...
>         if (tp->snd_ssthresh < TCP_INFINITE_SSTHRESH &&
>             (inet_csk(sk)->icsk_ca_state == TCP_CA_CWR || tp->undo_marker)) {
>                 tcp_snd_cwnd_set(tp, tp->snd_ssthresh);
>                 tp->snd_cwnd_stamp = tcp_jiffies32;
>         }
>     ...
> }
>
> This copies snd_ssthresh into snd_cwnd without any bound.
> tcp_snd_cwnd_set() warns once on 0 but still stores it.
>
> Later in the same tcp_ack(), tcp_cong_control() calls
> __cubictcp_cong_avoid() with snd_cwnd == 0 and snd_ssthresh == 0.
> tcp_in_slow_start() is false, so bictcp_update(ca, 0, acked)
> computes delta = 0 and the TCP friendliness loop never terminates.
>
> A snd_ssthresh between TCP_MAX_SND_CWND and TCP_INFINITE_SSTHRESH
> would instead leave snd_cwnd above TCP_MAX_SND_CWND. There
> cwnd * beta_scale wraps and the u32 multiplications in native
> ->ssthresh() handlers overflow, which is the state the commit message
> says native congestion controls must not see.
>
> Should tcp_reinit_congestion_control() also bound snd_ssthresh, for
> example to snd_cwnd_clamp or by resetting it to
> TCP_INFINITE_SSTHRESH? Or should it reset the CWR/Recovery state
> when switching away from a BPF congestion control? Or is there
> something that keeps tcp_end_cwnd_reduction() from running after
> such a switch that I am missing?
>
> [Severity: High]
>
> This isn't a bug introduced by this patch, but it falls under the same
> "BPF can store any value" threat model. tcp_cong_avoid_ai() is exposed as
> a kfunc through bpf_tcp_ca_check_kfunc_ids. It divides by w without
> checking for zero:

Right, this is a separate issue, I will send a separate patch.

>
> 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;
>     ...
> }
>
> When w == 0 the guard is always true.
>
> Can a BPF congestion control reach this division by zero in any of these
> ways?
>
> - Call tcp_cong_avoid_ai(tp, 0, acked) directly. The verifier does not
>   range check scalar kfunc arguments.
>
> - Store snd_cwnd = 0 and snd_ssthresh = 0, then call the
>   tcp_reno_cong_avoid() kfunc. It passes tcp_snd_cwnd(tp) as w and does
>   not check cwnd.
>
> - Use the new cubictcp_cong_avoid() wrapper with a valid snd_cwnd, after
>   writing ca->cnt = 0, ca->last_cwnd = snd_cwnd and ca->last_time = a
>   recent tcp_jiffies32 value (readable from fields such as
>   tp->lsndtime). bictcp_update() then returns early, before
>   ca->cnt = max(ca->cnt, 2U), and __cubictcp_cong_avoid() calls
>   tcp_cong_avoid_ai(tp, 0, acked).
>
> On x86 this would raise a divide error during ACK processing, usually in
> softirq context. Native callers never pass w == 0.
>
> >  }
> >
> > +/* BPF congestion controls can store any value in tp->snd_cwnd.
> > + * bictcp_update() would loop forever with cwnd == 0, or with a cwnd
> > + * large enough for cwnd * beta_scale to wrap.
>
> [Severity: Low]
>

Comment gone in v2.

> This isn't a bug, but is this comment accurate? It suggests that the
> TCP_MAX_SND_CWND check rules out every cwnd for which cwnd * beta_scale
> wraps.
>
> cubictcp_register() accepts beta up to 1023, which gives
> beta_scale = 8 * 2047 / 3 / 1 = 5458. With that value the u32 product
> wraps for any cwnd above 786912. So every cwnd in [786913, 1048576]
> passes the wrapper and still wraps.
>
> In that range the product wraps only once and delta stays at least 482,
> so the infinite loop is still prevented. The comment, though, treats
> "wraps" as if it meant "loops forever", and the wrap still produces a
> much smaller delta than intended.
>
> The TCP_MAX_SND_CWND comment in include/net/tcp.h (from 475ff14ba558)
> says multipliers go "up to 2047", which does not account for beta_scale
> in bictcp_update().
>
> > + * Native callers do not need this check.
> > + */
> > +__bpf_kfunc static void cubictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
> > +{
> > +     u32 cwnd = tcp_snd_cwnd(tcp_sk(sk));
> > +
> > +     if (unlikely(!cwnd || cwnd > TCP_MAX_SND_CWND))
> > +             return;
> > +     __cubictcp_cong_avoid(sk, ack, acked);
>
> [Severity: High]
>
> Does this wrapper fully close the hang in the bictcp_update() TCP
> friendliness loop? It checks only snd_cwnd.

Same answer: there is no loop anymore in v2.

The loop runs about
> ack_cnt / delta times, and ack_cnt comes either from the acked argument,
> which the BPF program chooses, or from struct bictcp state in
> icsk_ca_priv. BPF is allowed to write that state:
>
> net/ipv4/bpf_tcp_ca.c:bpf_tcp_ca_btf_struct_access() {
>     ...
>         case bpf_ctx_range(struct inet_connection_sock, icsk_ca_priv):
>                 end = offsetofend(struct inet_connection_sock, icsk_ca_priv);
>     ...
> }
>
> snd_cwnd = 1 passes the check and gives delta = (1 * 15) >> 3 = 1 with
> the default beta_scale. Suppose a struct_ops program stores
> snd_cwnd = 1, snd_ssthresh <= 1 (so tcp_in_slow_start() is false and
> acked is not consumed), ca->epoch_start = 0 and ca->last_cwnd != 1, and
> then calls:
>
>     cubictcp_cong_avoid(sk, 0, 0xffffffff);
>
> The call chain is
> cubictcp_cong_avoid()->__cubictcp_cong_avoid()->bictcp_update(ca, 1, 0xffffffff).
> The epoch_start == 0 branch sets ca->ack_cnt = 0xffffffff, and then
> this loop runs about 2^32 times during ACK processing with the socket
> lock held:
>
>                 delta = (cwnd * scale) >> 3;
>                 while (ca->ack_cnt > delta) {
>                         ca->ack_cnt -= delta;
>                         ca->tcp_cwnd++;
>                 }
>
> beta_scale is a runtime global, so the compiler is not expected to turn
> this loop into a division.
>
> The program can set up that state again and call the kfunc repeatedly,
> either unrolled or through bpf_loop(). Would that keep the CPU stuck
> past the soft lockup and RCU stall thresholds, which is the same hang
> in ACK processing the commit message describes?
>
> Could the kfunc wrapper also bound acked (and/or ca->ack_cnt), or could
> the BPF path compute the friendliness update with a division instead of
> the loop?
>
> > +}
> > +
>
> [ ... ]
>
> > @@ -475,7 +489,7 @@ __bpf_kfunc static void cubictcp_acked(struct sock *sk, const struct ack_sample
> >  static struct tcp_congestion_ops cubictcp __read_mostly = {
> >       .init           = cubictcp_init,
> >       .ssthresh       = cubictcp_recalc_ssthresh,
> > -     .cong_avoid     = cubictcp_cong_avoid,
> > +     .cong_avoid     = __cubictcp_cong_avoid,
> >       .set_state      = cubictcp_state,
> >       .undo_cwnd      = tcp_reno_undo_cwnd,
> >       .cwnd_event_tx_start = cubictcp_cwnd_event_tx_start,
>
> [Severity: Medium]
> Can a BPF congestion control still hand native CUBIC an invalid
> snd_cwnd after the clamp in tcp_reinit_congestion_control() has run?
>
> bpf_tcp_ca_get_func_proto() allows bpf_setsockopt() from every
> tcp_congestion_ops callback except .release. So a BPF .cong_avoid,
> .in_ack_event, .pkts_acked, .set_state or .undo_cwnd can call
> bpf_setsockopt(sk, SOL_TCP, TCP_CONGESTION, "cubic", ...).
>
> That call goes through sol_tcp_sockopt_congestion(),
> do_tcp_setsockopt() and tcp_set_congestion_control() into
> tcp_reinit_congestion_control(). That function clamps snd_cwnd and
> switches icsk_ca_ops to cubictcp synchronously. Control then returns
> to the BPF callback, which is still running.
>
> The callback still has write access to tp->snd_cwnd and
> tp->snd_ssthresh through bpf_tcp_ca_btf_struct_access(). It can
> store snd_cwnd = 0 (or 0x88888889) and snd_ssthresh = 0 right after
> bpf_setsockopt() returns. sol_tcp_sockopt_congestion() already
> carries a comment about a similar after-the-switch hazard for cdg.
>
> The .undo_cwnd case does not even need a direct write:
>
> net/ipv4/tcp_input.c:tcp_undo_cwnd_reduction() {
>     ...
>                 tcp_snd_cwnd_set(tp, icsk->icsk_ca_ops->undo_cwnd(sk));
>     ...
> }
>
> The value returned by the BPF .undo_cwnd() is stored after that same
> call has already switched icsk_ca_ops to cubictcp.
>
> The hang then follows on the next ACK, or later in the same tcp_ack()
> when the switch is done from .in_ack_event or .pkts_acked.
> __cubictcp_cong_avoid() runs bictcp_update() with cwnd == 0 or
> 0x88888889, so delta is 0. The TCP friendliness loop then spins
> forever in ACK processing, usually from softirq with the socket
> spinlock held.
>
> __cubictcp_cong_avoid() now relies on snd_cwnd being sane after
> tcp_reinit_congestion_control(). Given this ordering, is that the
> right place for the check? Would it be better to reject
> TCP_CONGESTION from bpf_setsockopt() inside tcp_congestion_ops
> callbacks, the way cdg is already rejected? Or to validate snd_cwnd
> at a point the BPF program cannot run after?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106133748.2181133-1-edumazet%40kernel.org


pw-bot: cr