netdev - Re: [PATCH net-next v2] tcp: enable mid stream window clamp

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]

Message-ID: <BYAPR15MB251818EA80E5569A768F0EC5BAC69@BYAPR15MB2518.namprd15.prod.outlook.com>
Date:   Wed, 25 Aug 2021 16:57:30 +0000
From:   Neil Spring <ntspring@...com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>
CC:     "yoshfuji@...ux-ipv6.org" <yoshfuji@...ux-ipv6.org>,
        "dsahern@...nel.org" <dsahern@...nel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ncardwell@...gle.com" <ncardwell@...gle.com>,
        "ycheng@...gle.com" <ycheng@...gle.com>
Subject: Re: [PATCH net-next v2] tcp: enable mid stream window clamp


Eric Dumazet wrote:
> On 8/19/21 12:54 PM, Neil Spring wrote:
> > The TCP_WINDOW_CLAMP socket option is defined in tcp(7) to "Bound the size of
> > the advertised window to this value."  Window clamping is distributed across two
> > variables, window_clamp ("Maximal window to advertise" in tcp.h) and rcv_ssthresh
> > ("Current window clamp").
> >
> > This patch updates the function where the window clamp is set to also reduce the current
> > window clamp, rcv_sshthresh, if needed.  With this, setting the TCP_WINDOW_CLAMP option
> > has the documented effect of limiting the window.
> >
> > Signed-off-by: Neil Spring <ntspring@...com>
> > ---
> > v2: - fix email formatting
> >
> >
> >  net/ipv4/tcp.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index f931def6302e..2dc6212d5888 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -3338,6 +3338,8 @@ int tcp_set_window_clamp(struct sock *sk, int val)
> >        } else {
> >                tp->window_clamp = val < SOCK_MIN_RCVBUF / 2 ?
> >                        SOCK_MIN_RCVBUF / 2 : val;
> > +             tp->rcv_ssthresh = min(tp->rcv_ssthresh,
> > +                                    tp->window_clamp);

> This fits in a single line I think.
>                 tp->rcv_ssthresh = min(tp->rcv_ssthresh, tp->window_clamp);

I'll fix in v3 in a moment, thanks!

> >        }
> >        return 0;
> >  }
> >

> Hi Neil
> 
> Can you provide a packetdrill test showing the what the new expected behavior is ?

Sure.  I submitted a pull request on packetdrill -
https://github.com/google/packetdrill/pull/56 - to document the intended behavior.

> It is not really clear why you need this.

The observation is that this option is currently ineffective at limiting once the
connection is exchanging data. I interpret this as a result of only looking at 
the window clamp when growing rcv_ssthresh, not as a key to reduce this 
limit.  

The packetdrill example will fail at the point where an ack should have a reduced
window due to the clamp.

> Also if we are unable to increase tp->rcv_ssthresh, this means the following sequence
> will not work as we would expect :

> +0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [10000], 4) = 0
> +0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [100000], 4) = 0

The packetdrill shows that raising the window clamp works:
tcp_grow_window takes over and raises the window quickly, but I'll add
a specific test for this sequence (with no intervening data) to confirm.

> Thanks.

Thanks Eric!

-neil