netdev - Re: [PATCH net-next] tcp: update window_clamp together with scaling

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: <CANn89iJOSUa2EvgENS=zc+TKtD6gOgfVn-6me1SNhwFrA2+CXw@mail.gmail.com>
Date: Wed, 3 Apr 2024 16:22:57 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Hechao Li <hli@...flix.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Soheil Hassas Yeganeh <soheil@...gle.com>, netdev@...r.kernel.org, 
	kernel-developers@...flix.com, Tycho Andersen <tycho@...ho.pizza>
Subject: Re: [PATCH net-next] tcp: update window_clamp together with scaling_ratio

On Tue, Apr 2, 2024 at 11:56 PM Hechao Li <hli@...flix.com> wrote:
>
> After commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale"),
> we noticed an application-level timeout due to reduced throughput. This
> can be reproduced by the following minimal client and server program.
>
> server:
>
...
>
> Before the commit, it takes around 22 seconds to transfer 10M data.
> After the commit, it takes 40 seconds. Because our application has a
> 30-second timeout, this regression broke the application.
>
> The reason that it takes longer to transfer data is that
> tp->scaling_ratio is initialized to a value that results in ~0.25 of
> rcvbuf. In our case, SO_RCVBUF is set to 65536 by the application, which
> translates to 2 * 65536 = 131,072 bytes in rcvbuf and hence a ~28k
> initial receive window.

What driver are you using, what MTU is set ?

If you get a 0.25 ratio, that is because a driver is oversizing rx skbs.

SO_RCVBUF 65536 would map indeed to 32768 bytes of payload.

>
> Later, even though the scaling_ratio is updated to a more accurate
> skb->len/skb->truesize, which is ~0.66 in our environment, the window
> stays at ~0.25 * rcvbuf. This is because tp->window_clamp does not
> change together with the tp->scaling_ratio update. As a result, the
> window size is capped at the initial window_clamp, which is also ~0.25 *
> rcvbuf, and never grows bigger.
>
> This patch updates window_clamp along with scaling_ratio. It changes the
> calculation of the initial rcv_wscale as well to make sure the scale
> factor is also not capped by the initial window_clamp.

This is very suspicious.

>
> A comment from Tycho Andersen <tycho@...ho.pizza> is "What happens if
> someone has done setsockopt(sk, TCP_WINDOW_CLAMP) explicitly; will this
> and the above not violate userspace's desire to clamp the window size?".
> This comment is not addressed in this patch because the existing code
> also updates window_clamp at several places without checking if
> TCP_WINDOW_CLAMP is set by user space. Adding this check now may break
> certain user space assumption (similar to how the original patch broke
> the assumption of buffer overhead being 50%). For example, if a user
> space program sets TCP_WINDOW_CLAMP but the applicaiton behavior relies
> on window_clamp adjusted by the kernel as of today.

Quite frankly I would prefer we increase tcp_rmem[] sysctls, instead
of trying to accomodate
with too small SO_RCVBUF values.

This would benefit old applications that were written 20 years ago.