Re: [PATCH net v3] net: ipv4: add zero check for snd_cwnd_clamp

[netdev-bot+sashiko@kernel.org]

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

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

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: the divide-by-zero it targets can still
  be triggered.
- [Medium] The new check rejects every RTM_NEWROUTE with RTAX_CWND == 0
  with -EINVAL, whether or not RTAX_CWND is locked in RTAX_LOCK.
- [Low] The new error path in ip_metrics_convert() uses plain
  NL_SET_ERR_MSG(extack, "RTAX_CWND must be greater than zero") and does…
- [Low] In the commit message, 'Reported-by: co+71b6509cb791e531@bugs.sh'
  is followed directly by 'Assisted-by: LLM' instead of a Closes: or…

--

net: ipv4: add zero check for snd_cwnd_clamp

This commit adds a check in ip_metrics_convert() that rejects an RTAX_CWND
route metric with a value of zero. The goal is to stop snd_cwnd_clamp from
being set to zero, which causes a divide error in tcp_cong_avoid_ai().

> Reported-by: co+71b6509cb791e531@bugs.sh
> Assisted-by: LLM

[Severity: Low]
This isn't a bug, but checkpatch.pl warns that Reported-by: should be
immediately followed by a Closes: or Link: tag with a URL to the report.
Could the link to the original report be added?

> diff --git a/net/ipv4/metrics.c b/net/ipv4/metrics.c
> index ad40762a8b383..e83a129a25b68 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 == 0) {

[Severity: High]
Does this fully close the divide-by-zero? The check only runs when
RTA_METRICS actually contains an RTAX_CWND attribute.

Suppose RTA_METRICS contains only RTAX_LOCK = (1 << RTAX_CWND) and has no
RTAX_CWND attribute. Then this loop never sees type == RTAX_CWND.

ip_fib_metrics_init() allocates the array with kzalloc_obj(), so
metrics[RTAX_CWND - 1] stays 0, and the lock bit is stored unchanged. That
leaves the same state as the locked "cwnd 0" configuration this patch is
meant to reject.

>From there, the path in the oops looks the same as before:

tcpm_suck_dst()
    if (dst_metric_locked(dst, RTAX_CWND))
        val |= 1 << TCP_METRIC_CWND;
    ...
    tcp_metric_set(tm, TCP_METRIC_CWND,
                   dst_metric_raw(dst, RTAX_CWND));

tcp_init_metrics()
    if (tcp_metric_locked(tm, TCP_METRIC_CWND))
        tp->snd_cwnd_clamp = tcp_metric_get(tm, TCP_METRIC_CWND);

tcp_slow_start()
    tcp_snd_cwnd_set(tp, min(cwnd, tp->snd_cwnd_clamp));

tcp_reno_cong_avoid()->tcp_cong_avoid_ai(tp, 0, acked)
    u32 delta = tp->snd_cwnd_cnt / w;

Both fib_create_info() and ip6_route_info_create() go through
ip_fib_metrics_init(), so IPv4 and IPv6 routes would both still be exposed.

Could this per-attribute check be replaced? One option is to check after
the loop that a locked RTAX_CWND has a non-zero value:

    if ((metrics[RTAX_LOCK - 1] & (1 << RTAX_CWND)) &&
        !metrics[RTAX_CWND - 1])

Another is a guard in tcp_init_metrics() that only applies the clamp when
the cached value is non-zero.

[Severity: Medium]
This check rejects every route with RTAX_CWND == 0, whether or not
RTAX_CWND is locked in RTAX_LOCK. Is that intended?

As far as I can tell, the value only reaches socket state through
tcp_init_metrics(), and only when it is locked:

    if (tcp_metric_locked(tm, TCP_METRIC_CWND))
        tp->snd_cwnd_clamp = tcp_metric_get(tm, TCP_METRIC_CWND);

The other uses in tcp_metrics.c only update the cache for unlocked entries.

So a route such as "ip route add ... cwnd 0" without lock used to be
accepted and was harmless. It now fails with -EINVAL. This applies to IPv6
routes too, because ip6_route_info_create() uses the same
ip_fib_metrics_init() path. With Cc: stable, the same change would reach
stable kernels.

The commit message also says:

    User can pass zero to snd_cwnd_clamp via RTAX_CWND type.

However, the value only reaches snd_cwnd_clamp when it is locked. The
"net: ipv4:" subject also doesn't mention that IPv6 route installation is
affected.

Would a lock-aware check after the attribute loop, or a guard in
tcp_init_metrics(), avoid rejecting the harmless unlocked case?

> +			NL_SET_ERR_MSG(extack, "RTAX_CWND must be greater than zero");

[Severity: Low]
This isn't a bug, but could this use NL_SET_ERR_MSG_ATTR(extack, nla, ...)
so the offending attribute is reported? The nla_len check earlier in
ip_metrics_convert() already does this, and nla is in scope here.

> +			return -EINVAL;
> +		}
>  		metrics[type - 1] = val;
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015201549.4023-2-kj2864%40nyu.edu