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

[Jason Xing]

On Thu, Mar 6, 2025 at 5:45 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
>
> On Thu, Mar 6, 2025 at 6:22 AM Jason Xing <kerneljasonxing@xxxxxxxxx> wrote:
> >
> > On Wed, Mar 5, 2025 at 10:49 PM Matthieu Baerts (NGI0)
> > <matttbe@xxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > Tested-by: Jason Xing <kerneljasonxing@xxxxxxxxx>
> >
> > 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.
>
> Can you share what precise test you did ?
>
> Thanks !

I did the test[1] on the virtual machine running the kernel [2]. And
after seeing your reply, I checked out a clean branch and compiled the
kernel with the patch reverted again and rebooted. The case can still
be reliably reproduced in my machine. Here are some outputs from BPF
program[3]:
 sudo bpftrace tcp_cap.bt.2
Attaching 1 probe...
4327813, 4326775, 4310912
netperf
        tcp_set_window_clamp+1
        tcp_data_queue+1744
        tcp_rcv_established+501
        tcp_v4_do_rcv+369
        tcp_v4_rcv+4800
        ip_protocol_deliver_rcu+65

4327813, 4326827, 4310912
netperf
        tcp_set_window_clamp+1
        tcp_data_queue+1744
        tcp_rcv_established+501
        tcp_v4_do_rcv+369
        tcp_v4_rcv+4800
        ip_protocol_deliver_rcu+65

418081, 417052, 417024
swapper/11
        tcp_set_window_clamp+1
        tcp_data_queue+1744
        tcp_rcv_established+501
        tcp_v4_do_rcv+369
        tcp_v4_rcv+4800
        ip_protocol_deliver_rcu+65

I can help if you want to see something else.

Thanks,
Jason

[1]: netperf -H 127.0.0.1 and netperf -H 127.0.0.1 -t TCP_CRR
[2]: based on commit f130a0cc1b4ff1 with reverted patch
commit 196150bf8e912eb27d4f083fc223ad8787709c6f (HEAD -> main)
Author: Jason Xing <kerneljasonxing@xxxxxxxxx>
Date:   Thu Mar 6 19:00:47 2025 +0800

    Revert "tcp: tcp_set_window_clamp() cleanup"

    This reverts commit 863a952eb79a6acf2b1f654f4e75ed104ff4cc81.

commit f130a0cc1b4ff1ef28a307428d40436032e2b66e (origin/main, origin/HEAD)
Author: Eric Dumazet <edumazet@xxxxxxxxxx>
Date:   Tue Mar 4 12:59:18 2025 +0000

    inet: fix lwtunnel_valid_encap_type() lock imbalance

    After blamed commit rtm_to_fib_config() now calls
    lwtunnel_valid_encap_type{_attr}() without RTNL held,
    triggering an unlock balance in __rtnl_unlock,
    as reported by syzbot [1]
[3]:
kprobe: tcp_set_window_clamp
{
        $sk = (struct sock *) arg0;
        $val = arg1;
        $tp = (struct tcp_sock *) $sk;
        $dport = $sk->__sk_common.skc_dport;
        $dport = bswap($dport);
        $lport = $sk->__sk_common.skc_num;

        if ($tp->rcv_ssthresh > $tp->rcv_wnd) {
                printf("%u, %u, %u\n", $tp->window_clamp,
$tp->rcv_ssthresh, $tp->rcv_wnd);
                printf("%s %s\n", comm, kstack(6));
        }
}