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: <CANn89iJ4DyC8OSEA2Qn3WhWHAUr9Bpo_ZmJdcx3ofM-qKvEU=g@mail.gmail.com>
Date: Thu, 6 Mar 2025 11:16:13 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Matthieu Baerts <matttbe@...nel.org>
Cc: Jason Xing <kerneljasonxing@...il.com>, mptcp@...ts.linux.dev, 
	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 Thu, Mar 6, 2025 at 11:12 AM Matthieu Baerts <matttbe@...nel.org> wrote:
>
> On 06/03/2025 11:02, Eric Dumazet wrote:
> > On Thu, Mar 6, 2025 at 10:55 AM Matthieu Baerts <matttbe@...nel.org> wrote:
> >>
> >> Hi Eric,
> >>
> >> On 06/03/2025 10:45, Eric Dumazet wrote:
> >>> On Thu, Mar 6, 2025 at 6:22 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> >>>>
> >>>> 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
> >>>>
> >>>
> >>> Patch looks fine to me but all our tests are passing with the current kernel,
> >>> and I was not able to trigger the condition.
> >>
> >> Thank you for having looked at this patch!
> >>
> >>
> >>> Can you share what precise test you did ?
> >>
> >> To be able to get a situation where "rcv_ssthresh > rcv_wnd", I simply
> >> executed MPTCP Connect selftest. You can also force creating TCP only
> >> connections with '-tt', e.g.
> >>
> >>   ./mptcp_connect.sh -tt
> >
> > I was asking Jason about TCP tests. He mentioned TCP_CRR
>
> Oops, I'm sorry, I didn't look at the "To:" field.
>
> > I made several of them, with temporary debug in the kernel that did
> > not show the issue.
> >
> >
> > I am wondering if this could hide an issue in MPTCP ?
> Indeed, I was wondering the same thing. I didn't see anything obvious
> when looking at this issue. The behaviours around the window clamping,
> with MPTCP single flow, and "plain" TCP were quite similar I think.

OK, let me run mptcp tests just in case I see something dubious.