[PATCH net v2] tcp: reject zero CWND route metrics

[PATCH net v2] tcp: reject zero CWND route metrics

[Weiming Shi]

A process with CAP_NET_ADMIN in a user-created network namespace can send
an RTM_NEWROUTE request with RTAX_CWND set to zero. It can also set the
RTAX_CWND lock bit while omitting RTAX_CWND, leaving the zero-initialized
metric value locked. tcpm_suck_dst() copies the zero into the TCP metrics
cache, and tcp_init_metrics() later installs it as snd_cwnd_clamp. This
makes the initial cwnd zero and can reach a divide by zero in
tcp_cong_avoid_ai().

Reject zero RTAX_CWND and RTAX_INITCWND metrics while parsing route
metrics. Also reject a locked CWND without a value, so a zero CWND cannot
enter the TCP metrics cache.

Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
Reported-by: co+71b6509cb791e531@bugs.sh
Suggested-by: Eric Dumazet <edumazet@kernel.org>
Assisted-by: LLM
Cc: stable@vger.kernel.org
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 net/ipv4/metrics.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/ipv4/metrics.c b/net/ipv4/metrics.c
index ad40762a8b38..c26f95126ae7 100644
--- a/net/ipv4/metrics.c
+++ b/net/ipv4/metrics.c
@@ -54,9 +54,19 @@ 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 || type == RTAX_INITCWND) && !val) {
+			NL_SET_ERR_MSG(extack, "CWND metric must be greater than zero");
+			return -EINVAL;
+		}
 		metrics[type - 1] = val;
 	}
 
+	if ((metrics[RTAX_LOCK - 1] & (1U << RTAX_CWND)) &&
+	    !metrics[RTAX_CWND - 1]) {
+		NL_SET_ERR_MSG(extack, "Locked CWND metric requires a value");
+		return -EINVAL;
+	}
+
 	if (ecn_ca)
 		metrics[RTAX_FEATURES - 1] |= DST_FEATURE_ECN_CA;
 
-- 
2.55.0

Re: [PATCH net v2] tcp: reject zero CWND route metrics

[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 v2] tcp: reject zero CWND route metrics

[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] In net/ipv4/metrics.c:ip_metrics_convert(), the new per-
  attribute check `if ((type == RTAX_CWND || type == RTAX_INITCWND) &&…
- [Low] The commit message says the locked-CWND check exists 'so a zero
  CWND cannot enter the TCP metrics cache'.

--

tcp: reject zero CWND route metrics

This patch makes ip_metrics_convert() reject RTAX_CWND and RTAX_INITCWND
metrics that have a value of zero. It also rejects routes that set the
RTAX_CWND lock bit but do not supply a CWND value. The aim is to stop a
zero snd_cwnd_clamp from reaching tcp_cong_avoid_ai().

> Reject zero RTAX_CWND and RTAX_INITCWND metrics while parsing route
> metrics. Also reject a locked CWND without a value, so a zero CWND cannot
> enter the TCP metrics cache.

[Severity: Low]
Is this description accurate? tcpm_suck_dst() always copies the raw route
metric into the cache:

net/ipv4/tcp_metrics.c:tcpm_suck_dst() {
    ...
	tcp_metric_set(tm, TCP_METRIC_CWND,
		       dst_metric_raw(dst, RTAX_CWND));
    ...
}

dst_metric_raw() returns 0 for any route that has no RTAX_CWND metric. So
even with this patch, a zero CWND still enters the TCP metrics cache for
most routes.

That zero does no harm, because tcp_init_metrics() only uses it when the
CWND lock bit is set. What the new check really prevents is a locked zero
CWND.

Could the commit message say that a locked zero CWND can no longer reach
snd_cwnd_clamp, rather than saying no zero CWND can enter the cache?

> diff --git a/net/ipv4/metrics.c b/net/ipv4/metrics.c
> index ad40762a8b383..c26f95126ae72 100644
> --- a/net/ipv4/metrics.c
> +++ b/net/ipv4/metrics.c
> @@ -54,9 +54,19 @@ 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 || type == RTAX_INITCWND) && !val) {
> +			NL_SET_ERR_MSG(extack, "CWND metric must be greater than zero");
> +			return -EINVAL;
> +		}

[Severity: Medium]
Does this per-attribute check reject more than the divide by zero fix
needs?

The kernel used to accept route installs such as "ip route add ...
initcwnd 0" and "ip route add ... cwnd 0" (without lock). Both now fail
with -EINVAL in ip_fib_metrics_init()->ip_metrics_convert(), for IPv4 and
IPv6 alike.

The only reader of RTAX_INITCWND treats zero as "use the default":

net/ipv4/tcp_input.c:tcp_init_cwnd() {
	__u32 cwnd = (dst ? dst_metric(dst, RTAX_INITCWND) : 0);

	if (!cwnd)
		cwnd = TCP_INIT_CWND;
    ...
}

A cached CWND only reaches snd_cwnd_clamp when the lock bit is set:

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, tcp_update_metrics() only compares or averages it.
It is never used as a divisor.

The lock-gated check after the loop already rejects a locked zero CWND on
its own. That covers both an explicit zero and a missing attribute, since
ip_fib_metrics_init() zero-initializes the metrics array.

This patch is tagged for stable. Would it be better to keep only the
lock-gated check, and drop the INITCWND and unlocked CWND rejections?

Also, Documentation/netlink/specs/rt-route.yaml still describes cwnd and
initcwnd as plain u32 values and does not exclude zero. If these
rejections stay, should the spec be updated to match?

>  		metrics[type - 1] = val;
>  	}
>  
> +	if ((metrics[RTAX_LOCK - 1] & (1U << RTAX_CWND)) &&
> +	    !metrics[RTAX_CWND - 1]) {
> +		NL_SET_ERR_MSG(extack, "Locked CWND metric requires a value");
> +		return -EINVAL;
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004164944.2006741-1-bestswngs%40gmail.com