linux-kernel - Re: [PATCH net 1/2] tcp_metrics: set maximum cwnd from the dst entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250618190124.04b7a2c3@mordecai.tesarici.cz>
Date: Wed, 18 Jun 2025 19:01:24 +0200
From: Petr Tesarik <ptesarik@...e.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Neal Cardwell <ncardwell@...gle.com>, Kuniyuki
 Iwashima <kuniyu@...gle.com>, "open list:NETWORKING [TCP]"
 <netdev@...r.kernel.org>, David Ahern <dsahern@...nel.org>, Jakub Kicinski
 <kuba@...nel.org>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net 1/2] tcp_metrics: set maximum cwnd from the dst
 entry

On Tue, 17 Jun 2025 13:39:35 +0200
Petr Tesarik <ptesarik@...e.com> wrote:

> On Tue, 17 Jun 2025 13:00:53 +0200
> Paolo Abeni <pabeni@...hat.com> wrote:
> 
> > On 6/13/25 12:20 PM, Petr Tesarik wrote:  
> > > diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> > > index 4251670e328c8..dd8f3457bd72e 100644
> > > --- a/net/ipv4/tcp_metrics.c
> > > +++ b/net/ipv4/tcp_metrics.c
> > > @@ -477,6 +477,9 @@ void tcp_init_metrics(struct sock *sk)
> > >  	if (!dst)
> > >  		goto reset;
> > >  
> > > +	if (dst_metric_locked(dst, RTAX_CWND))
> > > +		tp->snd_cwnd_clamp = dst_metric(dst, RTAX_CWND);
> > > +
> > >  	rcu_read_lock();
> > >  	tm = tcp_get_metrics(sk, dst, false);
> > >  	if (!tm) {
> > > @@ -484,9 +487,6 @@ void tcp_init_metrics(struct sock *sk)
> > >  		goto reset;
> > >  	}
> > >  
> > > -	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) {    
> > 
> > It's unclear to me why you drop the tcp_metric_get() here. It looks like
> > the above will cause a functional regression, with unlocked cached
> > metrics no longer taking effects?  
> 
> Unlocked cached TCP_METRIC_CWND has never taken effects. As you can
> see, tcp_metric_get() was executed only if the metric was locked. 
> 
> In fact, the cwnd parameter in the route does not have any effect
> either. It's even documented in the manual page of ip-route(8):
> 
>               cwnd NUMBER (Linux 2.3.15+ only)
>                      the clamp for congestion window. It is ignored if
>                      the lock flag is not used.
> 
> Note that here is also an initcwnd parameter, and I'm not changing
> anything about the handling of that one.
> 
> Now, if you think that this TCP_METRIC_CWND is quite useless, then I
> wholeheartedly agree with you, but we cannot simply remove it, as it
> has become part of uapi, defined in include/uapi/linux/tcp_metrics.h.

As an afterthought, I'm not quite sure about the semantics of this
metric. The value calculated in tcp_update_metrics() has never been
used for anything since it was introduced in 2.3.15. So there is:

- either a locked cwnd value, which is used to clamp cwnd on a
  route and never updated,
- or an unlocked cwnd value, which is updated upon connection
  termination but never used for anything by the kernel.

OK, the unlocked value can be read by userspace, but what is it
supposed to mean? The manual page for route-tcp_metrics(8) says: “CWND
metric value”, which sounds like the author did not have a clue either.

Unless someone here _has_ a clue, I'll just leave it as is, except the
clamp value will be taken from the routing table, as it makes no sense
to wait until the very same value propagates to a tcp_metrics_block
(where it is then never updated).

Petr T