[PATCH v2 net] tcp: bound snd_cwnd_clamp to TCP_MAX_SND_CWND

[PATCH v2 net] tcp: bound snd_cwnd_clamp to TCP_MAX_SND_CWND

[Eric Dumazet]

snd_cwnd has no practical upper bound: snd_cwnd_clamp defaults to ~0,
and RTAX_INITCWND, a locked RTAX_CWND metric, TCP_BPF_IW and
TCP_BPF_SNDCWND_CLAMP accept any positive value.

Congestion control modules do u32 arithmetic on cwnd that assumes
much smaller values. For instance, ssthresh() handlers compute:

  CUBIC, BIC, ROCCET:  cwnd * (BICTCP_BETA_SCALE + beta), up to 2047
  DCTCP:               cwnd * dctcp_alpha, up to 1024
  CDG, NV, CUBIC, ...: cwnd * beta, below 1024

With a large initcwnd route or TCP_BPF_IW, cwnd is set directly to
such values, and the next RTO or recovery calls ->ssthresh() with it.
The product then wraps: DCTCP with cwnd = 2^24 and alpha = 1024 gets
ssthresh == cwnd, and CUBIC computes a wrong ssthresh and Wmax.
CUBIC bictcp_update() also expects cwnd below about one million packets.

The paths that can grow cwnd by large amounts (tcp_slow_start(),
tcp_cong_avoid_ai(), bbr_set_cwnd() and tcp_init_cwnd()) already
bound it by snd_cwnd_clamp, so give snd_cwnd_clamp a finite default
of 2^20 packets, and cap the metric and BPF paths that set it or set
cwnd directly.

2^20 packets do not limit real flows: with the maximum window scale
of 14, a peer can not advertise more than 65535 << 14 bytes (~1GB),
which is less than 2^20 packets for any MSS of 1024 bytes or more.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Chris Mason <mason@kernel.org>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
v2: lower TCP_MAX_SND_CWND to 2^20 to cover CC ssthresh() products (Sashiko)
v1: https://lore.kernel.org/netdev/20261027130617.1550253-1-edumazet@kernel.org/

Chris found the bug thanks to an LLM. A repro can confirm it.
---
 include/net/tcp.h      | 9 +++++++++
 net/core/filter.c      | 3 ++-
 net/ipv4/tcp.c         | 2 +-
 net/ipv4/tcp_metrics.c | 3 ++-
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3560adddf821544d260bcf07cdd8a4d280c0ff36..5c9203aae83417d75bfe520cfdbecff36a15a057 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1554,6 +1554,15 @@ static inline unsigned int tcp_packets_in_flight(const struct tcp_sock *tp)
 
 #define TCP_INFINITE_SSTHRESH	0x7fffffff
 
+/* Default and maximum snd_cwnd_clamp, in packets.
+ * A peer can not advertise more than 65535 << 14 bytes, so a larger
+ * cwnd is useless with any MSS of 1024 bytes or more.
+ * Congestion control modules multiply cwnd by u32 factors up to 2047
+ * (cwnd * (BICTCP_BETA_SCALE + beta) in CUBIC), and CUBIC's
+ * bictcp_update() expects cwnd to stay below about one million packets.
+ */
+#define TCP_MAX_SND_CWND	(1U << 20)
+
 static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp)
 {
 	return tp->snd_cwnd;
diff --git a/net/core/filter.c b/net/core/filter.c
index 09c60a58c82abe8fc188c8f68059691cdcb52fb4..96f9ba67095a286b8973cbc198adf8da18af1b16 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5590,11 +5590,12 @@ static int bpf_sol_tcp_setsockopt(struct sock *sk, int optname,
 	case TCP_BPF_IW:
 		if (val <= 0 || tp->data_segs_out > tp->syn_data)
 			return -EINVAL;
-		tcp_snd_cwnd_set(tp, val);
+		tcp_snd_cwnd_set(tp, min_t(u32, val, tp->snd_cwnd_clamp));
 		break;
 	case TCP_BPF_SNDCWND_CLAMP:
 		if (val <= 0)
 			return -EINVAL;
+		val = min_t(u32, val, TCP_MAX_SND_CWND);
 		tp->snd_cwnd_clamp = val;
 		WRITE_ONCE(tp->snd_ssthresh, val);
 		break;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index b9de7c302ba83acfcb9cc6a2a2f67362277386b6..876f3e5f34c07f3af592d49e79d48eed3fc5d61c 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -456,7 +456,7 @@ void tcp_init_sock(struct sock *sk)
 	 * initialization of these values.
 	 */
 	tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;
-	tp->snd_cwnd_clamp = ~0;
+	tp->snd_cwnd_clamp = TCP_MAX_SND_CWND;
 	tp->mss_cache = TCP_MSS_DEFAULT;
 
 	tp->reordering = READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_reordering);
diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index d58f99c67d4dc0877d0e853c9e9faddd80343c22..ae551546422fe7c38b2ac3224f9ea9adbd3679d0 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -495,7 +495,8 @@ void tcp_init_metrics(struct sock *sk)
 	}
 
 	if (tcp_metric_locked(tm, TCP_METRIC_CWND))
-		tp->snd_cwnd_clamp = tcp_metric_get(tm, TCP_METRIC_CWND);
+		tp->snd_cwnd_clamp = min(tcp_metric_get(tm, TCP_METRIC_CWND),
+					 TCP_MAX_SND_CWND);
 
 	val = READ_ONCE(net->ipv4.sysctl_tcp_no_ssthresh_metrics_save) ?
 	      0 : tcp_metric_get(tm, TCP_METRIC_SSTHRESH);
-- 
2.53.0

Re: [PATCH v2 net] tcp: bound snd_cwnd_clamp to TCP_MAX_SND_CWND

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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).

- [Medium] BPF struct_ops congestion control programs can still store any
  u32 into snd_cwnd and snd_ssthresh, skipping both snd_cwnd_clamp and…
- [Low] Some CC u32 arithmetic can still overflow with cwnd at or below
  TCP_MAX_SND_CWND, which contradicts the new tcp.h comment saying CC…

--

tcp: bound snd_cwnd_clamp to TCP_MAX_SND_CWND

This patch gives snd_cwnd_clamp a finite default of TCP_MAX_SND_CWND
(2^20 packets) instead of ~0. It also caps the locked RTAX_CWND metric
and the TCP_BPF_IW and TCP_BPF_SNDCWND_CLAMP setsockopt paths. The goal
is to keep the u32 cwnd arithmetic in congestion control modules from
wrapping.

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 3560adddf8215..5c9203aae8341 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1554,6 +1554,15 @@ static inline unsigned int tcp_packets_in_flight(const struct tcp_sock *tp)
>  
>  #define TCP_INFINITE_SSTHRESH	0x7fffffff
>  
> +/* Default and maximum snd_cwnd_clamp, in packets.
> + * A peer can not advertise more than 65535 << 14 bytes, so a larger
> + * cwnd is useless with any MSS of 1024 bytes or more.
> + * Congestion control modules multiply cwnd by u32 factors up to 2047
> + * (cwnd * (BICTCP_BETA_SCALE + beta) in CUBIC), and CUBIC's
> + * bictcp_update() expects cwnd to stay below about one million packets.
> + */
> +#define TCP_MAX_SND_CWND	(1U << 20)

[Severity: Low]
Does the "factors up to 2047" sizing hold for CUBIC's bictcp_update()
with valid beta values?

cubictcp_register() in net/ipv4/tcp_cubic.c accepts beta in [0, 1023],
then computes:

	beta_scale = 8 * (BICTCP_BETA_SCALE + b) / 3
		/ (BICTCP_BETA_SCALE - b);

For beta=1023 this gives beta_scale = 5458. bictcp_update() then does
this in u32:

		delta = (cwnd * scale) >> 3;

With that scale, the product wraps for cwnd above about 786,906, which
is below TCP_MAX_SND_CWND. For cwnd = 800000, delta comes out as
8,929,088 instead of 545,800,000. The ack_cnt loop then inflates
ca->tcp_cwnd and pushes ca->cnt down toward 2, so cwnd grows too fast
near the clamp.

beta is also module_param(beta, int, 0644) in both tcp_cubic.c and
tcp_bic.c, and a write at runtime is not validated. If beta > 1023 is
written later, cwnd * (BICTCP_BETA_SCALE + beta) in
cubictcp_recalc_ssthresh() can wrap at cwnd = 2^20.

In BIC, beta=4096 makes cwnd * beta in bictcp_recalc_ssthresh() wrap to
0 at cwnd = 2^20, which returns an ssthresh of 2. That beta value makes
no sense anyway, though.

All of these need privileged module tuning, and the effect stays within
CC behaviour.

> +
>  static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp)
>  {
>  	return tp->snd_cwnd;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 09c60a58c82ab..96f9ba67095a2 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5590,11 +5590,12 @@ static int bpf_sol_tcp_setsockopt(struct sock *sk, int optname,
>  	case TCP_BPF_IW:
>  		if (val <= 0 || tp->data_segs_out > tp->syn_data)
>  			return -EINVAL;
> -		tcp_snd_cwnd_set(tp, val);
> +		tcp_snd_cwnd_set(tp, min_t(u32, val, tp->snd_cwnd_clamp));
>  		break;
>  	case TCP_BPF_SNDCWND_CLAMP:
>  		if (val <= 0)
>  			return -EINVAL;
> +		val = min_t(u32, val, TCP_MAX_SND_CWND);
>  		tp->snd_cwnd_clamp = val;
>  		WRITE_ONCE(tp->snd_ssthresh, val);
>  		break;

[Severity: Medium]
The commit message says the patch will "cap the metric and BPF paths
that set it or set cwnd directly". Does that also cover struct_ops
congestion control programs?

bpf_tcp_ca_btf_struct_access() in net/ipv4/bpf_tcp_ca.c only checks
that the store stays inside the field:

	case offsetof(struct tcp_sock, snd_cwnd):
		end = offsetofend(struct tcp_sock, snd_cwnd);
		break;
	...
	case offsetof(struct tcp_sock, snd_ssthresh):
		end = offsetofend(struct tcp_sock, snd_ssthresh);
		break;

So a BPF CC can still store any u32 into snd_cwnd and snd_ssthresh.
Those stores skip both snd_cwnd_clamp and TCP_MAX_SND_CWND.

The native functions named in the commit message are exported as kfuncs
to BPF_PROG_TYPE_STRUCT_OPS: cubictcp_recalc_ssthresh(),
cubictcp_cong_avoid() and dctcp_ssthresh(). Take snd_cwnd = 1 << 24 and
the default dctcp_alpha of 1024. In dctcp_ssthresh(), cwnd * dctcp_alpha
wraps to 0 and ssthresh == cwnd, which is the failure the commit message
describes.

tcp_reinit_congestion_control() doesn't reset snd_cwnd either. Switching
from a BPF CC to native CUBIC, DCTCP or BIC would carry an oversized
cwnd into their ->ssthresh() handlers.

There is a worse case in bictcp_update(). With the default beta_scale of
15 and snd_cwnd = 0x88888889, cwnd * 15 wraps to 7, so delta is 0:

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

Can this loop spin forever? A struct_ops CC could reach it by writing
snd_cwnd and setting snd_ssthresh = 1. It would then call the
cubictcp_cong_avoid() kfunc with acked >= 1 while the flow is cwnd
limited. The CPU would then spin in native code during ACK processing,
where the verifier's loop limits do not apply.

Loading a struct_ops program needs CAP_BPF plus CAP_PERFMON, or a
delegated BPF token. A cwnd of 0 can also reach the kfunc hang, and an
upper cap would not fix that.

Should the bound also be enforced on the struct_ops write path, or in
these kfuncs?

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

Re: [PATCH v2 net] tcp: bound snd_cwnd_clamp to TCP_MAX_SND_CWND

[Paolo Abeni]

On 11/2/26 18:17, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · 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).
> 
> - [Medium] BPF struct_ops congestion control programs can still store any
>    u32 into snd_cwnd and snd_ssthresh, skipping both snd_cwnd_clamp and…
> - [Low] Some CC u32 arithmetic can still overflow with cwnd at or below
>    TCP_MAX_SND_CWND, which contradicts the new tcp.h comment saying CC…
> 
> --
> 
> tcp: bound snd_cwnd_clamp to TCP_MAX_SND_CWND
> 
> This patch gives snd_cwnd_clamp a finite default of TCP_MAX_SND_CWND
> (2^20 packets) instead of ~0. It also caps the locked RTAX_CWND metric
> and the TCP_BPF_IW and TCP_BPF_SNDCWND_CLAMP setsockopt paths. The goal
> is to keep the u32 cwnd arithmetic in congestion control modules from
> wrapping.
> 
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index 3560adddf8215..5c9203aae8341 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -1554,6 +1554,15 @@ static inline unsigned int tcp_packets_in_flight(const struct tcp_sock *tp)
>>   
>>   #define TCP_INFINITE_SSTHRESH	0x7fffffff
>>   
>> +/* Default and maximum snd_cwnd_clamp, in packets.
>> + * A peer can not advertise more than 65535 << 14 bytes, so a larger
>> + * cwnd is useless with any MSS of 1024 bytes or more.
>> + * Congestion control modules multiply cwnd by u32 factors up to 2047
>> + * (cwnd * (BICTCP_BETA_SCALE + beta) in CUBIC), and CUBIC's
>> + * bictcp_update() expects cwnd to stay below about one million packets.
>> + */
>> +#define TCP_MAX_SND_CWND	(1U << 20)
> 
> [Severity: Low]
> Does the "factors up to 2047" sizing hold for CUBIC's bictcp_update()
> with valid beta values?
> 
> cubictcp_register() in net/ipv4/tcp_cubic.c accepts beta in [0, 1023],
> then computes:
> 
> 	beta_scale = 8 * (BICTCP_BETA_SCALE + b) / 3
> 		/ (BICTCP_BETA_SCALE - b);
> 
> For beta=1023 this gives beta_scale = 5458. bictcp_update() then does
> this in u32:
> 
> 		delta = (cwnd * scale) >> 3;
> 
> With that scale, the product wraps for cwnd above about 786,906, which
> is below TCP_MAX_SND_CWND. For cwnd = 800000, delta comes out as
> 8,929,088 instead of 545,800,000. The ack_cnt loop then inflates
> ca->tcp_cwnd and pushes ca->cnt down toward 2, so cwnd grows too fast
> near the clamp.
> 
> beta is also module_param(beta, int, 0644) in both tcp_cubic.c and
> tcp_bic.c, and a write at runtime is not validated. If beta > 1023 is
> written later, cwnd * (BICTCP_BETA_SCALE + beta) in
> cubictcp_recalc_ssthresh() can wrap at cwnd = 2^20.
> 
> In BIC, beta=4096 makes cwnd * beta in bictcp_recalc_ssthresh() wrap to
> 0 at cwnd = 2^20, which returns an ssthresh of 2. That beta value makes
> no sense anyway, though.
> 
> All of these need privileged module tuning, and the effect stays within
> CC behaviour.

My understanding is that the last sentence makes this comment mostly
irrelevant.

>>   static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp)
>>   {
>>   	return tp->snd_cwnd;
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 09c60a58c82ab..96f9ba67095a2 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -5590,11 +5590,12 @@ static int bpf_sol_tcp_setsockopt(struct sock *sk, int optname,
>>   	case TCP_BPF_IW:
>>   		if (val <= 0 || tp->data_segs_out > tp->syn_data)
>>   			return -EINVAL;
>> -		tcp_snd_cwnd_set(tp, val);
>> +		tcp_snd_cwnd_set(tp, min_t(u32, val, tp->snd_cwnd_clamp));
>>   		break;
>>   	case TCP_BPF_SNDCWND_CLAMP:
>>   		if (val <= 0)
>>   			return -EINVAL;
>> +		val = min_t(u32, val, TCP_MAX_SND_CWND);
>>   		tp->snd_cwnd_clamp = val;
>>   		WRITE_ONCE(tp->snd_ssthresh, val);
>>   		break;
> 
> [Severity: Medium]
> The commit message says the patch will "cap the metric and BPF paths
> that set it or set cwnd directly". Does that also cover struct_ops
> congestion control programs?
> 
> bpf_tcp_ca_btf_struct_access() in net/ipv4/bpf_tcp_ca.c only checks
> that the store stays inside the field:
> 
> 	case offsetof(struct tcp_sock, snd_cwnd):
> 		end = offsetofend(struct tcp_sock, snd_cwnd);
> 		break;
> 	...
> 	case offsetof(struct tcp_sock, snd_ssthresh):
> 		end = offsetofend(struct tcp_sock, snd_ssthresh);
> 		break;
> 
> So a BPF CC can still store any u32 into snd_cwnd and snd_ssthresh.
> Those stores skip both snd_cwnd_clamp and TCP_MAX_SND_CWND.
> 
> The native functions named in the commit message are exported as kfuncs
> to BPF_PROG_TYPE_STRUCT_OPS: cubictcp_recalc_ssthresh(),
> cubictcp_cong_avoid() and dctcp_ssthresh(). Take snd_cwnd = 1 << 24 and
> the default dctcp_alpha of 1024. In dctcp_ssthresh(), cwnd * dctcp_alpha
> wraps to 0 and ssthresh == cwnd, which is the failure the commit message
> describes.
> 
> tcp_reinit_congestion_control() doesn't reset snd_cwnd either. Switching
> from a BPF CC to native CUBIC, DCTCP or BIC would carry an oversized
> cwnd into their ->ssthresh() handlers.
> 
> There is a worse case in bictcp_update(). With the default beta_scale of
> 15 and snd_cwnd = 0x88888889, cwnd * 15 wraps to 7, so delta is 0:
> 
> 		delta = (cwnd * scale) >> 3;
> 		while (ca->ack_cnt > delta) {
> 			ca->ack_cnt -= delta;
> 			ca->tcp_cwnd++;
> 		}
> 
> Can this loop spin forever? A struct_ops CC could reach it by writing
> snd_cwnd and setting snd_ssthresh = 1. It would then call the
> cubictcp_cong_avoid() kfunc with acked >= 1 while the flow is cwnd
> limited. The CPU would then spin in native code during ACK processing,
> where the verifier's loop limits do not apply.
> 
> Loading a struct_ops program needs CAP_BPF plus CAP_PERFMON, or a
> delegated BPF token. A cwnd of 0 can also reach the kfunc hang, and an
> upper cap would not fix that.
> 
> Should the bound also be enforced on the struct_ops write path, or in
> these kfuncs?
This items looks like a separate thing deserving a separate fix.

/P

Re: [PATCH v2 net] tcp: bound snd_cwnd_clamp to TCP_MAX_SND_CWND

[Eric Dumazet]

Le mar. 3 nov. 2026 à 10:51, Paolo Abeni <pabeni@redhat.com> a écrit :
>
> On 11/2/26 18:17, netdev-bot+sashiko@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 2 potential
> > issue(s) to consider.
> >
> > Critical: 0 · High: 0 · 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).
> >
> > - [Medium] BPF struct_ops congestion control programs can still store any
> >    u32 into snd_cwnd and snd_ssthresh, skipping both snd_cwnd_clamp and…
> > - [Low] Some CC u32 arithmetic can still overflow with cwnd at or below
> >    TCP_MAX_SND_CWND, which contradicts the new tcp.h comment saying CC…
> >
> > --
> >
> > tcp: bound snd_cwnd_clamp to TCP_MAX_SND_CWND
> >
> > This patch gives snd_cwnd_clamp a finite default of TCP_MAX_SND_CWND
> > (2^20 packets) instead of ~0. It also caps the locked RTAX_CWND metric
> > and the TCP_BPF_IW and TCP_BPF_SNDCWND_CLAMP setsockopt paths. The goal
> > is to keep the u32 cwnd arithmetic in congestion control modules from
> > wrapping.
> >
> >> diff --git a/include/net/tcp.h b/include/net/tcp.h
> >> index 3560adddf8215..5c9203aae8341 100644
> >> --- a/include/net/tcp.h
> >> +++ b/include/net/tcp.h
> >> @@ -1554,6 +1554,15 @@ static inline unsigned int tcp_packets_in_flight(const struct tcp_sock *tp)
> >>
> >>   #define TCP_INFINITE_SSTHRESH      0x7fffffff
> >>
> >> +/* Default and maximum snd_cwnd_clamp, in packets.
> >> + * A peer can not advertise more than 65535 << 14 bytes, so a larger
> >> + * cwnd is useless with any MSS of 1024 bytes or more.
> >> + * Congestion control modules multiply cwnd by u32 factors up to 2047
> >> + * (cwnd * (BICTCP_BETA_SCALE + beta) in CUBIC), and CUBIC's
> >> + * bictcp_update() expects cwnd to stay below about one million packets.
> >> + */
> >> +#define TCP_MAX_SND_CWND    (1U << 20)
> >
> > [Severity: Low]
> > Does the "factors up to 2047" sizing hold for CUBIC's bictcp_update()
> > with valid beta values?
> >
> > cubictcp_register() in net/ipv4/tcp_cubic.c accepts beta in [0, 1023],
> > then computes:
> >
> >       beta_scale = 8 * (BICTCP_BETA_SCALE + b) / 3
> >               / (BICTCP_BETA_SCALE - b);
> >
> > For beta=1023 this gives beta_scale = 5458. bictcp_update() then does
> > this in u32:
> >
> >               delta = (cwnd * scale) >> 3;
> >
> > With that scale, the product wraps for cwnd above about 786,906, which
> > is below TCP_MAX_SND_CWND. For cwnd = 800000, delta comes out as
> > 8,929,088 instead of 545,800,000. The ack_cnt loop then inflates
> > ca->tcp_cwnd and pushes ca->cnt down toward 2, so cwnd grows too fast
> > near the clamp.
> >
> > beta is also module_param(beta, int, 0644) in both tcp_cubic.c and
> > tcp_bic.c, and a write at runtime is not validated. If beta > 1023 is
> > written later, cwnd * (BICTCP_BETA_SCALE + beta) in
> > cubictcp_recalc_ssthresh() can wrap at cwnd = 2^20.
> >
> > In BIC, beta=4096 makes cwnd * beta in bictcp_recalc_ssthresh() wrap to
> > 0 at cwnd = 2^20, which returns an ssthresh of 2. That beta value makes
> > no sense anyway, though.
> >
> > All of these need privileged module tuning, and the effect stays within
> > CC behaviour.
>
> My understanding is that the last sentence makes this comment mostly
> irrelevant.
>
> >>   static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp)
> >>   {
> >>      return tp->snd_cwnd;
> >> diff --git a/net/core/filter.c b/net/core/filter.c
> >> index 09c60a58c82ab..96f9ba67095a2 100644
> >> --- a/net/core/filter.c
> >> +++ b/net/core/filter.c
> >> @@ -5590,11 +5590,12 @@ static int bpf_sol_tcp_setsockopt(struct sock *sk, int optname,
> >>      case TCP_BPF_IW:
> >>              if (val <= 0 || tp->data_segs_out > tp->syn_data)
> >>                      return -EINVAL;
> >> -            tcp_snd_cwnd_set(tp, val);
> >> +            tcp_snd_cwnd_set(tp, min_t(u32, val, tp->snd_cwnd_clamp));
> >>              break;
> >>      case TCP_BPF_SNDCWND_CLAMP:
> >>              if (val <= 0)
> >>                      return -EINVAL;
> >> +            val = min_t(u32, val, TCP_MAX_SND_CWND);
> >>              tp->snd_cwnd_clamp = val;
> >>              WRITE_ONCE(tp->snd_ssthresh, val);
> >>              break;
> >
> > [Severity: Medium]
> > The commit message says the patch will "cap the metric and BPF paths
> > that set it or set cwnd directly". Does that also cover struct_ops
> > congestion control programs?
> >
> > bpf_tcp_ca_btf_struct_access() in net/ipv4/bpf_tcp_ca.c only checks
> > that the store stays inside the field:
> >
> >       case offsetof(struct tcp_sock, snd_cwnd):
> >               end = offsetofend(struct tcp_sock, snd_cwnd);
> >               break;
> >       ...
> >       case offsetof(struct tcp_sock, snd_ssthresh):
> >               end = offsetofend(struct tcp_sock, snd_ssthresh);
> >               break;
> >
> > So a BPF CC can still store any u32 into snd_cwnd and snd_ssthresh.
> > Those stores skip both snd_cwnd_clamp and TCP_MAX_SND_CWND.
> >
> > The native functions named in the commit message are exported as kfuncs
> > to BPF_PROG_TYPE_STRUCT_OPS: cubictcp_recalc_ssthresh(),
> > cubictcp_cong_avoid() and dctcp_ssthresh(). Take snd_cwnd = 1 << 24 and
> > the default dctcp_alpha of 1024. In dctcp_ssthresh(), cwnd * dctcp_alpha
> > wraps to 0 and ssthresh == cwnd, which is the failure the commit message
> > describes.
> >
> > tcp_reinit_congestion_control() doesn't reset snd_cwnd either. Switching
> > from a BPF CC to native CUBIC, DCTCP or BIC would carry an oversized
> > cwnd into their ->ssthresh() handlers.
> >
> > There is a worse case in bictcp_update(). With the default beta_scale of
> > 15 and snd_cwnd = 0x88888889, cwnd * 15 wraps to 7, so delta is 0:
> >
> >               delta = (cwnd * scale) >> 3;
> >               while (ca->ack_cnt > delta) {
> >                       ca->ack_cnt -= delta;
> >                       ca->tcp_cwnd++;
> >               }
> >
> > Can this loop spin forever? A struct_ops CC could reach it by writing
> > snd_cwnd and setting snd_ssthresh = 1. It would then call the
> > cubictcp_cong_avoid() kfunc with acked >= 1 while the flow is cwnd
> > limited. The CPU would then spin in native code during ACK processing,
> > where the verifier's loop limits do not apply.
> >
> > Loading a struct_ops program needs CAP_BPF plus CAP_PERFMON, or a
> > delegated BPF token. A cwnd of 0 can also reach the kfunc hang, and an
> > upper cap would not fix that.
> >
> > Should the bound also be enforced on the struct_ops write path, or in
> > these kfuncs?
> This items looks like a separate thing deserving a separate fix.

Agreed, I am working on a follow up.