* Re: [PATCH net v4] net: ipv4: add zero check for snd_cwnd_clamp
2026-10-17 21:48 [PATCH net v4] net: ipv4: add zero check for snd_cwnd_clamp Kaho Jiang
@ 2026-10-18 22:31 ` netdev-bot+sashiko
2026-10-19 8:33 ` Antoine Tenart
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-18 22:31 UTC (permalink / raw)
To: kj2864
Cc: ncardwell, kuniyu, davem, kuba, pabeni, horms, edumazet, netdev,
linux-kernel, co+71b6509cb791e531, stable
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 3 · Low: 0
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] The patch silently changes tcp_snd_cwnd_set() in
include/net/tcp.h, a hot-path helper used by every TCP congestion…
- [Medium] The new check in ip_metrics_convert() rejects the wrong set of
inputs.
- [Medium] net: ipv4: snd_cwnd_clamp still 0 in tcp_init_metrics() via
RTAX_LOCK
--
net: ipv4: add zero check for snd_cwnd_clamp
This patch makes ip_metrics_convert() reject a zero RTAX_CWND metric.
The goal is to avoid a divide error in tcp_cong_avoid_ai() when
snd_cwnd_clamp is zero. The patch also changes tcp_snd_cwnd_set() so it
no longer has the WARN_ON_ONCE() and instead clamps the written value to
at least 1.
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 4416cdf9bf30e..ad80b8fb8a520 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1521,8 +1521,7 @@ static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp)
>
> static inline void tcp_snd_cwnd_set(struct tcp_sock *tp, u32 val)
> {
> - WARN_ON_ONCE((int)val <= 0);
> - WRITE_ONCE(tp->snd_cwnd, val);
> + WRITE_ONCE(tp->snd_cwnd, max(val, 1U));
> }
[Severity: Medium]
Is this change meant to be part of this fix? The commit message only
talks about rejecting zero in ip_metrics_convert(). This hunk changes
tcp_snd_cwnd_set(), which every congestion control module uses.
The WARN_ON_ONCE() came from commit 40570375356c ("tcp: add accessors to
read/set tp->snd_cwnd"). Its purpose was to catch where and when
tp->snd_cwnd gets set to an illegal value. After this change, a zero
cwnd from any caller is silently turned into 1, with no diagnostic.
Underflowed values such as 0x80000000 used to trigger the warning
because (int)val <= 0. max(val, 1U) leaves them unchanged, so they now
pass without any warning.
It also looks like this hunk, not the metrics.c check, is what actually
prevents the reported divide error. With snd_cwnd_clamp == 0,
tcp_reno_cong_avoid()->tcp_slow_start() now stores 1 instead of 0. That
means tcp_cong_avoid_ai() no longer divides by zero.
Could the WARN_ON_ONCE() be kept? If the setter change is really needed,
could it go in a separate patch, or at least be described in the commit
message?
> diff --git a/net/ipv4/metrics.c b/net/ipv4/metrics.c
> index ad40762a8b383..cb36686973e57 100644
> --- a/net/ipv4/metrics.c
> +++ b/net/ipv4/metrics.c
> @@ -54,6 +54,10 @@ static int ip_metrics_convert(struct nlattr *fc_mx,
> NL_SET_ERR_MSG(extack, "Unknown flag set in feature mask in metrics attribute");
> return -EINVAL;
> }
> + if (type == RTAX_CWND && !val) {
> + NL_SET_ERR_MSG(extack, "RTAX_CWND must be greater than zero");
> + return -EINVAL;
> + }
[Severity: Medium]
Does this fully close the zero snd_cwnd_clamp case? The new check only
runs when an RTAX_CWND attribute is actually present in fc_mx.
ip_fib_metrics_init() allocates fib_metrics with kzalloc_obj(), so
metrics[RTAX_CWND - 1] starts out as zero. Consider a route request
whose RTA_METRICS has RTAX_LOCK with (1 << RTAX_CWND) set but no
RTAX_CWND attribute at all. It passes ip_metrics_convert() with CWND
locked and the CWND value still zero.
tcpm_suck_dst() then sees dst_metric_locked(dst, RTAX_CWND) and copies
both the lock bit and dst_metric_raw(dst, RTAX_CWND), which is 0, into
the tcp_metrics_block. tcp_init_metrics() applies a locked CWND
without checking for zero. TCP_METRIC_SSTHRESH and
TCP_METRIC_REORDERING are only applied when non-zero:
net/ipv4/tcp_metrics.c:tcp_init_metrics() {
...
if (tcp_metric_locked(tm, TCP_METRIC_CWND))
tp->snd_cwnd_clamp = tcp_metric_get(tm, TCP_METRIC_CWND);
val = READ_ONCE(net->ipv4.sysctl_tcp_no_ssthresh_metrics_save) ?
0 : tcp_metric_get(tm, TCP_METRIC_SSTHRESH);
if (val) {
WRITE_ONCE(tp->snd_ssthresh, val);
if (tp->snd_ssthresh > tp->snd_cwnd_clamp)
WRITE_ONCE(tp->snd_ssthresh, tp->snd_cwnd_clamp);
}
...
}
So tp->snd_cwnd_clamp is still set to 0. If a saved ssthresh exists,
tp->snd_ssthresh is clamped down to 0 as well.
tcp_init_cwnd() then returns min(cwnd, tp->snd_cwnd_clamp), which is
0. The congestion control paths that clamp against snd_cwnd_clamp keep
computing a cwnd of 0. These include tcp_slow_start(),
tcp_cong_avoid_ai(), hstcp_cong_avoid(), htcp_cong_avoid(),
tcp_veno_cong_avoid() and tcp_vegas_cong_avoid().
With the tcp.h hunk, tcp_snd_cwnd_set() silently turns that 0 into 1.
The divide error goes away, but the connection is pinned at a cwnd of
1.
BPF congestion control programs (net/ipv4/bpf_tcp_ca.c) are also
allowed to write tp->snd_cwnd directly, bypassing tcp_snd_cwnd_set().
They can still end up with snd_cwnd == 0.
This needs CAP_NET_ADMIN, but so does the RTAX_CWND == 0 case the
patch is trying to close.
Would it be better to validate the final result after the loop? For
example, reject the route when metrics[RTAX_LOCK - 1] has
(1 << RTAX_CWND) set and metrics[RTAX_CWND - 1] is zero. Or should
tcp_init_metrics() ignore a locked CWND of zero, the same way it
already ignores a zero ssthresh?
[Severity: Medium]
Should this check only apply when RTAX_CWND is locked?
TCP uses the CWND metric as snd_cwnd_clamp only when it is locked:
net/ipv4/tcp_metrics.c:tcp_init_metrics() {
...
if (tcp_metric_locked(tm, TCP_METRIC_CWND))
tp->snd_cwnd_clamp = tcp_metric_get(tm, TCP_METRIC_CWND);
...
}
When it is not locked, RTAX_CWND only seeds TCP_METRIC_CWND through
tcpm_suck_dst(). After that, tcp_update_metrics() treats it as a running
estimate and overwrites it:
net/ipv4/tcp_metrics.c:tcp_update_metrics() {
...
if (!tcp_metric_locked(tm, TCP_METRIC_CWND)) {
val = tcp_metric_get(tm, TCP_METRIC_CWND);
if (tcp_snd_cwnd(tp) > val)
tcp_metric_set(tm, TCP_METRIC_CWND,
tcp_snd_cwnd(tp));
}
...
}
So an unlocked zero means the same thing as unset. With this patch,
route requests that used to be accepted, such as "ip route add ...
cwnd 0" without "lock", now fail with -EINVAL.
ip_metrics_convert() is shared by IPv4 through
fib_create_info()->ip_fib_metrics_init() and by IPv6 through
ip6_route_info_create()->ip_fib_metrics_init(). The stricter input
check therefore applies to IPv6 routes too, even though the subject
says "net: ipv4". The commit message does not mention this change in
accepted input.
Also, a locked CWND with no RTAX_CWND attribute at all still seems to be
accepted. That leaves metrics[RTAX_CWND - 1] at zero, which
tcp_init_metrics() then copies into snd_cwnd_clamp.
Would it be better to check for "CWND locked and final value zero" after
the loop, rather than rejecting a zero attribute value?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017214845.28769-2-kj2864%40nyu.edu
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net v4] net: ipv4: add zero check for snd_cwnd_clamp
2026-10-17 21:48 [PATCH net v4] net: ipv4: add zero check for snd_cwnd_clamp Kaho Jiang
2026-10-18 22:31 ` netdev-bot+sashiko
@ 2026-10-19 8:33 ` Antoine Tenart
1 sibling, 0 replies; 3+ messages in thread
From: Antoine Tenart @ 2026-10-19 8:33 UTC (permalink / raw)
To: Kaho Jiang
Cc: ncardwell, kuniyu, davem, kuba, pabeni, horms, edumazet, netdev,
linux-kernel, co+71b6509cb791e531, stable
On Sat, Oct 17, 2026 at 05:48:46PM -0400, Kaho Jiang wrote:
> User can pass zero to snd_cwnd_clamp via RTAX_CWND type.
> This could lead to division of zero error if not checked properly.
>
> Oops: divide error: 0000 [#1] SMP KASAN NOPTI
> Call Trace:
> <IRQ>
> tcp_reno_cong_avoid (net/ipv4/tcp_cong.c:510 (discriminator 1))
> tcp_ack (net/ipv4/tcp_input.c:3517 net/ipv4/tcp_input.c:3873 net/ipv4/tcp_input.c:4440)
> tcp_rcv_established (net/ipv4/tcp_input.c:6685)
> tcp_v4_do_rcv (net/ipv4/tcp_ipv4.c:1854)
> tcp_v4_rcv (net/ipv4/tcp_ipv4.c:2248)
> ip_protocol_deliver_rcu (net/ipv4/ip_input.c:207 (discriminator 1))
> ip_local_deliver_finish (net/ipv4/ip_input.c:241 (discriminator 1))
> ip_local_deliver (./include/linux/netfilter.h:325 ./include/linux/netfilter.h:319 net/ipv4/ip_input.c:262)
> ip_rcv (./include/net/dst.h:481 (discriminator 6) net/ipv4/ip_input.c:492 (discriminator 6) ./include/linux/netfilter.h:325 (discriminator 6) ./include/li)
> __netif_receive_skb_one_core (net/core/dev.c:6297 (discriminator 4))
> __netif_receive_skb (net/core/dev.c:6410)
> process_backlog (net/core/dev.c:6761)
> __napi_poll.constprop.0 (net/core/dev.c:7830)
> net_rx_action (net/core/dev.c:7893 net/core/dev.c:8050)
> handle_softirqs (kernel/softirq.c:645)
> __irq_exit_rcu (kernel/softirq.c:679 kernel/softirq.c:519 kernel/softirq.c:758)
> irq_exit_rcu (kernel/softirq.c:775)
> sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1062 (discriminator 35) arch/x86/kernel/apic/apic.c:1062 (discriminator 35))
> RIP: 0010:tcp_cong_avoid_ai (net/ipv4/tcp_cong.c:482)
>
> Fix by checking if user pass in zero as parameter,
> if so reject the value zero.
>
> Fixes: 814d488c6126 ("tcp: fix the timid additive increase on stretch ACKs")
> Cc: stable@vger.kernel.org
> Reported-by: co+71b6509cb791e531@bugs.sh
> Assisted-by: LLM
> Signed-off-by: Kaho Jiang <kj2864@nyu.edu>
> ---
> include/net/tcp.h | 3 +--
> net/ipv4/metrics.c | 4 ++++
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 4416cdf9bf30..ad80b8fb8a52 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1521,8 +1521,7 @@ static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp)
>
> static inline void tcp_snd_cwnd_set(struct tcp_sock *tp, u32 val)
> {
> - WARN_ON_ONCE((int)val <= 0);
> - WRITE_ONCE(tp->snd_cwnd, val);
> + WRITE_ONCE(tp->snd_cwnd, max(val, 1U));
Please keep the warning (it's not only about 0).
> }
>
> static inline bool tcp_in_slow_start(const struct tcp_sock *tp)
> diff --git a/net/ipv4/metrics.c b/net/ipv4/metrics.c
> index ad40762a8b38..cb36686973e5 100644
> --- a/net/ipv4/metrics.c
> +++ b/net/ipv4/metrics.c
> @@ -54,6 +54,10 @@ static int ip_metrics_convert(struct nlattr *fc_mx,
> NL_SET_ERR_MSG(extack, "Unknown flag set in feature mask in metrics attribute");
> return -EINVAL;
> }
> + if (type == RTAX_CWND && !val) {
> + NL_SET_ERR_MSG(extack, "RTAX_CWND must be greater than zero");
> + return -EINVAL;
> + }
> metrics[type - 1] = val;
> }
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread