linux-kernel - Re: [PATCH net-next] tcp: clamp window like before the cleanup

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAL+tcoAqZmeV0-4rjH-EPmhBBaS=ZSwgcXhU8ZsBCr_aXS3Lqw@mail.gmail.com>
Date: Thu, 6 Mar 2025 13:21:56 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: "Matthieu Baerts (NGI0)" <matttbe@...nel.org>
Cc: mptcp@...ts.linux.dev, Eric Dumazet <edumazet@...gle.com>, 
	Neal Cardwell <ncardwell@...gle.com>, Kuniyuki Iwashima <kuniyu@...zon.com>, 
	"David S. Miller" <davem@...emloft.net>, David Ahern <dsahern@...nel.org>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] tcp: clamp window like before the cleanup

On Wed, Mar 5, 2025 at 10:49 PM Matthieu Baerts (NGI0)
<matttbe@...nel.org> wrote:
>
> A recent cleanup changed the behaviour of tcp_set_window_clamp(). This
> looks unintentional, and affects MPTCP selftests, e.g. some tests
> re-establishing a connection after a disconnect are now unstable.
>
> Before the cleanup, this operation was done:
>
>   new_rcv_ssthresh = min(tp->rcv_wnd, new_window_clamp);
>   tp->rcv_ssthresh = max(new_rcv_ssthresh, tp->rcv_ssthresh);
>
> The cleanup used the 'clamp' macro which takes 3 arguments -- value,
> lowest, and highest -- and returns a value between the lowest and the
> highest allowable values. This then assumes ...
>
>   lowest (rcv_ssthresh) <= highest (rcv_wnd)
>
> ... which doesn't seem to be always the case here according to the MPTCP
> selftests, even when running them without MPTCP, but only TCP.
>
> For example, when we have ...
>
>   rcv_wnd < rcv_ssthresh < new_rcv_ssthresh
>
> ... before the cleanup, the rcv_ssthresh was not changed, while after
> the cleanup, it is lowered down to rcv_wnd (highest).
>
> During a simple test with TCP, here are the values I observed:
>
>   new_window_clamp (val)  rcv_ssthresh (lo)  rcv_wnd (hi)
>       117760   (out)         65495         <  65536
>       128512   (out)         109595        >  80256  => lo > hi
>       1184975  (out)         328987        <  329088
>
>       113664   (out)         65483         <  65536
>       117760   (out)         110968        <  110976
>       129024   (out)         116527        >  109696 => lo > hi
>
> Here, we can see that it is not that rare to have rcv_ssthresh (lo)
> higher than rcv_wnd (hi), so having a different behaviour when the
> clamp() macro is used, even without MPTCP.
>
> Note: new_window_clamp is always out of range (rcv_ssthresh < rcv_wnd)
> here, which seems to be generally the case in my tests with small
> connections.
>
> I then suggests reverting this part, not to change the behaviour.
>
> Fixes: 863a952eb79a ("tcp: tcp_set_window_clamp() cleanup")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/551
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@...nel.org>

Tested-by: Jason Xing <kerneljasonxing@...il.com>

Thanks for catching this. I should have done more tests :(

Now I use netperf with TCP_CRR to test loopback and easily see the
case where tp->rcv_ssthresh is larger than tp->rcv_wnd, which means
tp->rcv_wnd is not the upper bound as you said.

Thanks,
Jason

> ---
> Notes: the 'Fixes' commit is only in net-next
> ---
>  net/ipv4/tcp.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index eb5a60c7a9ccdd23fb78a74d614c18c4f7e281c9..46951e74930844af952dfbc57a107b504d4e296b 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3693,7 +3693,7 @@ EXPORT_SYMBOL(tcp_sock_set_keepcnt);
>
>  int tcp_set_window_clamp(struct sock *sk, int val)
>  {
> -       u32 old_window_clamp, new_window_clamp;
> +       u32 old_window_clamp, new_window_clamp, new_rcv_ssthresh;
>         struct tcp_sock *tp = tcp_sk(sk);
>
>         if (!val) {
> @@ -3714,12 +3714,12 @@ int tcp_set_window_clamp(struct sock *sk, int val)
>         /* Need to apply the reserved mem provisioning only
>          * when shrinking the window clamp.
>          */
> -       if (new_window_clamp < old_window_clamp)
> +       if (new_window_clamp < old_window_clamp) {
>                 __tcp_adjust_rcv_ssthresh(sk, new_window_clamp);
> -       else
> -               tp->rcv_ssthresh = clamp(new_window_clamp,
> -                                        tp->rcv_ssthresh,
> -                                        tp->rcv_wnd);
> +       } else {
> +               new_rcv_ssthresh = min(tp->rcv_wnd, new_window_clamp);
> +               tp->rcv_ssthresh = max(new_rcv_ssthresh, tp->rcv_ssthresh);
> +       }
>         return 0;
>  }
>
>
> ---
> base-commit: c62e6f056ea308d6382450c1cb32e41727375885
> change-id: 20250305-net-next-fix-tcp-win-clamp-9f4c417ff44d
>
> Best regards,
> --
> Matthieu Baerts (NGI0) <matttbe@...nel.org>
>