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]
Date: Mon, 27 Nov 2023 22:59:05 +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.

> On Fri, 2023-11-24 at 05:27 +0000, Neil Spring wrote:
> > >
> > > ________________________________________
> > > 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);
> 
> FTR I considered something similar to the above, but I opted for the
> present patch, as the above does not pass the pktdrill suggested by
> Eric here:
> 
> https://lore.kernel.org/netdev/6070816e-f7d2-725a-ec10-9d85f15455a2@gmail.com/
> 
> Cheers,
> 
> Paolo

Ahh, thanks for pointing that out!  I see that the packetdrill also needs some fixes to address changes to the initial window.  :(

Would the following address the concern?  

tp->rcv_ssthresh = min(max(tp->rcv_ssthresh, tp->rcv_wnd), tp->window_clamp);

(that is, rcv_sshthresh must be no greater than window_clamp, but otherwise it can keep the larger of its current value or the last advertised window.)

I believe this addresses both problem cases (transient tiny clamp; closed window when clamping) and passes (slightly less picky) packetdrill tests.

-neil

Powered by blists - more mailing lists