netdev - Re: [PATCH net-next 4/4] tcp: remove limit on initial receive window

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:   Tue, 12 Jan 2021 18:05:36 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Heath Caldwell <hcaldwel@...mai.com>
Cc:     netdev <netdev@...r.kernel.org>, Yuchung Cheng <ycheng@...gle.com>,
        Josh Hunt <johunt@...mai.com>, Ji Li <jli@...mai.com>
Subject: Re: [PATCH net-next 4/4] tcp: remove limit on initial receive window

On Tue, Jan 12, 2021 at 5:02 PM Heath Caldwell <hcaldwel@...mai.com> wrote:
>
> On 2021-01-12 09:30 (+0100), Eric Dumazet <edumazet@...gle.com> wrote:
> > I think the whole patch series is an attempt to badly break TCP stack.
>
> Can you explain the concern that you have about how these changes might
> break the TCP stack?
>
> Patches 1 and 3 fix clear bugs.

Not clear to me at least.

If they were bug fixes, a FIxes: tag would be provided.

You are a first time contributor to linux TCP stack, so better make
sure your claims are solid.

>
> Patches 2 and 4 might be arguable, though.

So we have to pick up whatever pleases us ?

>
> Is you objection primarily about the limit removed by patch 4?
>
> > Hint : 64K is really the max allowed by TCP standards. Yes, this is
> > sad, but this is it.
>
> Do you mean the limit imposed by the size of the "Window Size" header
> field?  This limitation is directly addressed by the check in
> __tcp_transmit_skb():
>
>         if (likely(!(tcb->tcp_flags & TCPHDR_SYN))) {
>                 th->window      = htons(tcp_select_window(sk));
>                 tcp_ecn_send(sk, skb, th, tcp_header_size);
>         } else {
>                 /* RFC1323: The window in SYN & SYN/ACK segments
>                  * is never scaled.
>                  */
>                 th->window      = htons(min(tp->rcv_wnd, 65535U));
>         }
>
> and checking (and capping it there) allows for the field to not overflow
> while also not artificially restricting the size of the window which
> will later be advertised (once window scaling is negotiated).
>
> > I will not spend hours of work running  packetdrill tests over your
> > changes, but I am sure they are now quite broken.
> >
> > If you believe auto tuning is broken, fix it properly, without trying
> > to change all the code so that you can understand it.
>
> The removal of the limit specifically addresses the situation where auto
> tuning cannot work: on the initial burst.

Which standard allows for a very big initial burst ?
AFAIK, IW10 got a lot of pushback, and that was only for a burst of ~14600 bytes
Allowing arbitrarily large windows needs IETF approval.


>  There is no way to know
> whether an installation desires to receive a larger first burst unless
> it is specifically configured - and this limit prevents such
> configuration.
>
> > I strongly advise you read RFC 7323 before doing any changes in TCP
> > stack, and asking us to spend time reviewing your patches.
>
> Can you point out the part of the RFC which would be violated by
> initially (that is, the first packet after the SYN) advertising a window
> larger than 64KB?

We handle gradual increase of rwin based on the behavior of
applications and senders (skb len/truesize ratio)

Again if you believe autotuning is broken please fix it, instead of
throwing it away.

Changing initial RWIN can have subtle differences on how fast DRS algo
can kick in.

This kind of change would need to be heavily tested, with millions of
TCP flows in the wild.
(ie not in a lab environment)

Have you done this ? What happens if flows are malicious and sent 100
bytes per segment ?