| 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
| ||
|
Message-ID:
<SA1PR15MB5187F56AEFC6B6A581E056C5A3B9A@SA1PR15MB5187.namprd15.prod.outlook.com>
Date: Fri, 24 Nov 2023 05:27:28 +0000
From: Neil Spring <ntspring@...a.com>
To: Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
Neal
Cardwell <ncardwell@...gle.com>, Wei Wang <weiwan@...gle.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"David S. Miller"
<davem@...emloft.net>,
David Ahern <dsahern@...nel.org>, Jakub Kicinski
<kuba@...nel.org>,
David Gibson <david@...son.dropbear.id.au>
Subject: Re: [PATCH net] tcp: fix mid stream window clamp.
>
> ________________________________________
> From: Paolo Abeni <pabeni@...hat.com>
> Sent: Thursday, November 23, 2023 10:16 AM
> To: Eric Dumazet; Neal Cardwell; Wei Wang
> Cc: netdev@...r.kernel.org; David S. Miller; David Ahern; Jakub Kicinski; Neil Spring; David Gibson
> Subject: Re: [PATCH net] tcp: fix mid stream window clamp.
>
> !-------------------------------------------------------------------|
> This Message Is From an External Sender
>
> |-------------------------------------------------------------------!
>
> On Thu, 2023-11-23 at 18:10 +0100, Eric Dumazet wrote:
> > CC Neal and Wei
> >
> > On Thu, Nov 23, 2023 at 4:25 PM Paolo Abeni <pabeni@...hat.com> wrote:
> > >
> > > After the blamed commit below, if the user-space application performs
> > > window clamping when tp->rcv_wnd is 0, the TCP socket will never be
> > > able to announce a non 0 receive window, even after completely emptying
> > > the receive buffer and re-setting the window clamp to higher values.
> > >
> > > Refactor tcp_set_window_clamp() to address the issue: when the user
> > > decreases the current clamp value, set rcv_ssthresh according to the
> > > same logic used at buffer initialization time.
> > > When increasing the clamp value, give the rcv_ssthresh a chance to grow
> > > according to previously implemented heuristic.
> > >
> > > Fixes: 3aa7857fe1d7 ("tcp: enable mid stream window clamp")
> > > Reported-by: David Gibson <david@...son.dropbear.id.au>
> > > Reported-by: Stefano Brivio <sbrivio@...hat.com>
> > > Reviewed-by: Stefano Brivio <sbrivio@...hat.com>
> > > Tested-by: Stefano Brivio <sbrivio@...hat.com>
> > > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > > ---
> > > net/ipv4/tcp.c | 19 ++++++++++++++++---
> > > 1 file changed, 16 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > index 53bcc17c91e4..1a9b9064e080 100644
> > > --- a/net/ipv4/tcp.c
> > > +++ b/net/ipv4/tcp.c
> > > @@ -3368,9 +3368,22 @@ int tcp_set_window_clamp(struct sock *sk, int val)
> > > return -EINVAL;
> > > tp->window_clamp = 0;
> > > } else {
> > > - tp->window_clamp = val < SOCK_MIN_RCVBUF / 2 ?
> > > - SOCK_MIN_RCVBUF / 2 : val;
> > > - tp->rcv_ssthresh = min(tp->rcv_wnd, tp->window_clamp);
> > > + u32 new_rcv_ssthresh, old_window_clamp = tp->window_clamp;
> > > + u32 new_window_clamp = val < SOCK_MIN_RCVBUF / 2 ?
> > > + SOCK_MIN_RCVBUF / 2 : val;
> > > +
> > > + if (new_window_clamp == old_window_clamp)
> > > + return 0;
> > > +
> > > + tp->window_clamp = new_window_clamp;
> > > + if (new_window_clamp < old_window_clamp) {
> > > + tp->rcv_ssthresh = min(tp->rcv_ssthresh,
> > > + new_window_clamp);
> > > + } else {
> > > + new_rcv_ssthresh = min(tp->rcv_wnd, tp->window_clamp);
> > > + tp->rcv_ssthresh = max(new_rcv_ssthresh,
> > > + tp->rcv_ssthresh);
> > > + }
> > > }
> > > return 0;
> > > }
> >
> > It seems there is no provision for SO_RESERVE_MEM
>
> Indeed I did take that in account.
>
> > I wonder if tcp_adjust_rcv_ssthresh() could help here ?
>
> I don't know how to fit it into the above. tcp_adjust_rcv_ssthresh()
> tends to shrink rcv_ssthresh to low values when no memory is reserved.
>
> Dealing directly with SO_RESERVE_MEM when shrinking the threshold feels
> easier to me, something alike:
>
> if (new_window_clamp == old_window_clamp)
> return 0;
>
> tp->window_clamp = new_window_clamp;
> if (new_window_clamp < old_window_clamp) {
> int unused_mem = sk_unused_reserved_mem(sk);
>
> tp->rcv_ssthresh = min(tp->rcv_ssthresh,
> new_window_clamp);
>
> if (unused_mem)
> tp->rcv_ssthresh = max_t(u32, tp->rcv_ssthresh,
> tcp_win_from_space(sk, unused_mem));
> } else {
> new_rcv_ssthresh = min(tp->rcv_wnd, tp->window_clamp);
> tp->rcv_ssthresh = max(new_rcv_ssthresh,
> tp->rcv_ssthresh);
> }
>
> Possibly the bits shared with tcp_adjust_rcv_ssthresh() could be
> factored out in a common helper.
>
> > Have you considered reverting 3aa7857fe1d7 ("tcp: enable mid stream
> > window clamp") ?
>
>
> That would work, too and will be simpler.
>
> The issue at hand was noted with an application that really wants to
> limit the announced window:
>
> https://gitlab.com/dgibson/passt
>
> I guess touching rcv_ssthresh would be a bit more effective.
>
> Not much more in the end, as both window_clamp and rcv_ssthresh can
> later grow due to rcv buf auto-tune. Ideally we would like to prevent
> tcp_rcv_space_adjust() from touching window_clamp after
> TCP_WINDOW_CLAMP - but that is another matter/patch.
>
> Thanks!
>
> Paolo
>
The patch to fix the bug where rcv_sshthresh is reduced to zero on a full receive window and cannot recover is:
-tp->rcv_ssthresh = min(tp->rcv_wnd, tp->window_clamp);
+tp->rcv_ssthresh = min(tp->rcv_ssthresh, tp->window_clamp);
This change and the patch in this thread behave the same in the following packetdrill that reproduces the problem.
-neil
---
`../common/defaults.sh`
// Establish a connection.
0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, ..., ...) = 0
+0 listen(3, 1) = 0
+.1 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 8>
+0 > S. 0:0(0) ack 1 win 65535 <mss 1460,nop,nop,sackOK,nop,wscale 8>
+.1 < . 1:1(0) ack 1 win 320
+0 accept(3, ..., ...) = 4
+0 setsockopt(4, SOL_SOCKET, SO_RCVBUF, [65536], 4) = 0
// Close rcv_wnd to zero
+.01 < . 1:64000(63999) ack 1 win 320
+.04 > . 1:1(0) ack 64000 win 248
+.01 < . 64000:128000(64000) ack 1 win 320
// delayed ack waiting for user to read.
+0.02~+0.14 > . 1:1(0) ack 128000 win 0
+0 setsockopt(4, IPPROTO_TCP, TCP_WINDOW_CLAMP, [12800], 4) = 0
+.01 read(4, ..., 129000) = 127999
// read opens window ; this is the key part of the test.
+0 > . 1:1(0) ack 128000 win 50
// raise clamp further, above buffer size to observe growth
+0 setsockopt(4, IPPROTO_TCP, TCP_WINDOW_CLAMP, [128000], 4) = 0
+.01 < . 128000:140800(12800) ack 1 win 320
+0 > . 1:1(0) ack 140800 win 150
// Read to clear the receive buffer.
+.01 read(4, ..., 129000) = 12800
+.01 < . 140800:172000(31200) ack 1 win 320
+0 > . 1:1(0) ack 172000 win 361
Powered by blists - more mailing lists