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] [day] [month] [year] [list]
Message-ID: <adf56f38-55ba-a878-f48d-31e2b8a59b7b@cloudflare.com>
Date: Sat, 10 Jun 2023 23:10:20 -0500
From: Mike Freemon <mfreemon@...udflare.com>
To: Eric Dumazet <edumazet@...gle.com>, Neal Cardwell <ncardwell@...gle.com>
Cc: netdev@...r.kernel.org, kernel-team@...udflare.com
Subject: Re: [PATCH net-next v3] tcp: enforce receive buffer memory limits by
 allowing the tcp window to shrink



On 6/9/23 20:43, Eric Dumazet wrote:
> On Fri, Jun 9, 2023 at 10:47 PM Mike Freemon <mfreemon@...udflare.com> wrote:
>>
>> From: "mfreemon@...udflare.com" <mfreemon@...udflare.com>
>>
>> Under certain circumstances, the tcp receive buffer memory limit
>> set by autotuning (sk_rcvbuf) is increased due to incoming data
>> packets as a result of the window not closing when it should be.
>> This can result in the receive buffer growing all the way up to
>> tcp_rmem[2], even for tcp sessions with a low BDP.
>>
>> To reproduce:  Connect a TCP session with the receiver doing
>> nothing and the sender sending small packets (an infinite loop
>> of socket send() with 4 bytes of payload with a sleep of 1 ms
>> in between each send()).  This will cause the tcp receive buffer
>> to grow all the way up to tcp_rmem[2].
>>
>> As a result, a host can have individual tcp sessions with receive
>> buffers of size tcp_rmem[2], and the host itself can reach tcp_mem
>> limits, causing the host to go into tcp memory pressure mode.
>>
>> The fundamental issue is the relationship between the granularity
>> of the window scaling factor and the number of byte ACKed back
>> to the sender.  This problem has previously been identified in
>> RFC 7323, appendix F [1].
>>
>> The Linux kernel currently adheres to never shrinking the window.
>>
>> In addition to the overallocation of memory mentioned above, the
>> current behavior is functionally incorrect, because once tcp_rmem[2]
>> is reached when no remediations remain (i.e. tcp collapse fails to
>> free up any more memory and there are no packets to prune from the
>> out-of-order queue), the receiver will drop in-window packets
>> resulting in retransmissions and an eventual timeout of the tcp
>> session.  A receive buffer full condition should instead result
>> in a zero window and an indefinite wait.
>>
>> In practice, this problem is largely hidden for most flows.  It
>> is not applicable to mice flows.  Elephant flows can send data
>> fast enough to "overrun" the sk_rcvbuf limit (in a single ACK),
>> triggering a zero window.
>>
>> But this problem does show up for other types of flows.  Examples
>> are websockets and other type of flows that send small amounts of
>> data spaced apart slightly in time.  In these cases, we directly
>> encounter the problem described in [1].
>>
>> RFC 7323, section 2.4 [2], says there are instances when a retracted
>> window can be offered, and that TCP implementations MUST ensure
>> that they handle a shrinking window, as specified in RFC 1122,
>> section 4.2.2.16 [3].  All prior RFCs on the topic of tcp window
>> management have made clear that sender must accept a shrunk window
>> from the receiver, including RFC 793 [4] and RFC 1323 [5].
>>
>> This patch implements the functionality to shrink the tcp window
>> when necessary to keep the right edge within the memory limit by
>> autotuning (sk_rcvbuf).  This new functionality is enabled with
>> the new sysctl: net.ipv4.tcp_shrink_window
>>
>> Additional information can be found at:
>> https://blog.cloudflare.com/unbounded-memory-usage-by-tcp-for-receive-buffers-and-how-we-fixed-it/
>>
>> [1] https://www.rfc-editor.org/rfc/rfc7323#appendix-F
>> [2] https://www.rfc-editor.org/rfc/rfc7323#section-2.4
>> [3] https://www.rfc-editor.org/rfc/rfc1122#page-91
>> [4] https://www.rfc-editor.org/rfc/rfc793
>> [5] https://www.rfc-editor.org/rfc/rfc1323
>>
>> Signed-off-by: Mike Freemon <mfreemon@...udflare.com>
>> ---
>>  Documentation/networking/ip-sysctl.rst | 13 +++++
>>  include/net/netns/ipv4.h               |  1 +
>>  net/ipv4/sysctl_net_ipv4.c             |  9 ++++
>>  net/ipv4/tcp_ipv4.c                    |  2 +
>>  net/ipv4/tcp_output.c                  | 73 ++++++++++++++++++++++++--
>>  5 files changed, 93 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
>> index 366e2a5097d9..ddb895e8af56 100644
>> --- a/Documentation/networking/ip-sysctl.rst
>> +++ b/Documentation/networking/ip-sysctl.rst
>> @@ -981,6 +981,19 @@ tcp_tw_reuse - INTEGER
>>  tcp_window_scaling - BOOLEAN
>>         Enable window scaling as defined in RFC1323.
>>
>> +tcp_shrink_window - BOOLEAN
>> +       This changes how the TCP receive window is calculated.
>> +
>> +       RFC 7323, section 2.4, says there are instances when a retracted
>> +       window can be offered, and that TCP implementations MUST ensure
>> +       that they handle a shrinking window, as specified in RFC 1122.
>> +
>> +       - 0 - Disabled. The window is never shrunk.
>> +       - 1 - Enabled.  The window is shrunk when necessary to remain within
>> +                       the memory limit set by autotuning (sk_rcvbuf).
>> +
>> +       Default: 0
>> +
>>  tcp_wmem - vector of 3 INTEGERs: min, default, max
>>         min: Amount of memory reserved for send buffers for TCP sockets.
>>         Each TCP socket has rights to use it due to fact of its birth.
>> diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
>> index a4efb7a2796c..f00374718159 100644
>> --- a/include/net/netns/ipv4.h
>> +++ b/include/net/netns/ipv4.h
>> @@ -65,6 +65,7 @@ struct netns_ipv4 {
>>  #endif
>>         bool                    fib_has_custom_local_routes;
>>         bool                    fib_offload_disabled;
>> +       u8                      sysctl_tcp_shrink_window;
>>  #ifdef CONFIG_IP_ROUTE_CLASSID
>>         atomic_t                fib_num_tclassid_users;
>>  #endif
>> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
>> index 356afe54951c..2afb0870648b 100644
>> --- a/net/ipv4/sysctl_net_ipv4.c
>> +++ b/net/ipv4/sysctl_net_ipv4.c
>> @@ -1480,6 +1480,15 @@ static struct ctl_table ipv4_net_table[] = {
>>                 .extra1         = SYSCTL_ZERO,
>>                 .extra2         = &tcp_syn_linear_timeouts_max,
>>         },
>> +       {
>> +               .procname       = "tcp_shrink_window",
>> +               .data           = &init_net.ipv4.sysctl_tcp_shrink_window,
>> +               .maxlen         = sizeof(u8),
>> +               .mode           = 0644,
>> +               .proc_handler   = proc_dou8vec_minmax,
>> +               .extra1         = SYSCTL_ZERO,
>> +               .extra2         = SYSCTL_ONE,
>> +       },
>>         { }
>>  };
>>
>> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
>> index 84a5d557dc1a..9213804b034f 100644
>> --- a/net/ipv4/tcp_ipv4.c
>> +++ b/net/ipv4/tcp_ipv4.c
>> @@ -3281,6 +3281,8 @@ static int __net_init tcp_sk_init(struct net *net)
>>                 net->ipv4.tcp_congestion_control = &tcp_reno;
>>
>>         net->ipv4.sysctl_tcp_syn_linear_timeouts = 4;
>> +       net->ipv4.sysctl_tcp_shrink_window = 0;
>> +
>>         return 0;
>>  }
>>
>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>> index f8ce77ce7c3e..5c86873e2193 100644
>> --- a/net/ipv4/tcp_output.c
>> +++ b/net/ipv4/tcp_output.c
>> @@ -260,8 +260,8 @@ static u16 tcp_select_window(struct sock *sk)
>>         u32 old_win = tp->rcv_wnd;
>>         u32 cur_win = tcp_receive_window(tp);
>>         u32 new_win = __tcp_select_window(sk);
>> +       struct net *net = sock_net(sk);
> 
> Here you cache sock_net() in @net variable.
> 
>>
>> -       /* Never shrink the offered window */
>>         if (new_win < cur_win) {
>>                 /* Danger Will Robinson!
>>                  * Don't update rcv_wup/rcv_wnd here or else
>> @@ -270,11 +270,15 @@ static u16 tcp_select_window(struct sock *sk)
>>                  *
>>                  * Relax Will Robinson.
>>                  */
>> -               if (new_win == 0)
>> -                       NET_INC_STATS(sock_net(sk),
>> -                                     LINUX_MIB_TCPWANTZEROWINDOWADV);
>> -               new_win = ALIGN(cur_win, 1 << tp->rx_opt.rcv_wscale);
>> +               if (!READ_ONCE(net->ipv4.sysctl_tcp_shrink_window)) {
>> +                       /* Never shrink the offered window */
>> +                       if (new_win == 0)
>> +                               NET_INC_STATS(sock_net(sk),
> 
> You then can replace sock_net(sk) by @net here.
> 
>> +                                             LINUX_MIB_TCPWANTZEROWINDOWADV);
>> +                       new_win = ALIGN(cur_win, 1 << tp->rx_opt.rcv_wscale);
>> +               }
>>         }
>> +
>>         tp->rcv_wnd = new_win;
>>         tp->rcv_wup = tp->rcv_nxt;
>>
>> @@ -3003,6 +3007,7 @@ u32 __tcp_select_window(struct sock *sk)
>>  {
>>         struct inet_connection_sock *icsk = inet_csk(sk);
>>         struct tcp_sock *tp = tcp_sk(sk);
>> +       struct net *net = sock_net(sk);
>>         /* MSS for the peer's data.  Previous versions used mss_clamp
>>          * here.  I don't know if the value based on our guesses
>>          * of peer's MSS is better for the performance.  It's more correct
>> @@ -3024,6 +3029,12 @@ u32 __tcp_select_window(struct sock *sk)
>>                 if (mss <= 0)
>>                         return 0;
>>         }
>> +
>> +       if (READ_ONCE(net->ipv4.sysctl_tcp_shrink_window))
>> +               goto shrink_window_allowed;
>> +
>> +       /* do not allow window to shrink */
>> +
>>         if (free_space < (full_space >> 1)) {
>>                 icsk->icsk_ack.quick = 0;
>>
>> @@ -3077,6 +3088,58 @@ u32 __tcp_select_window(struct sock *sk)
>>                         window = free_space;
>>         }
>>
>> +       return window;
>> +
>> +shrink_window_allowed:
>> +       /* new window should always be an exact multiple of scaling factor */
>> +       free_space = round_down(free_space, 1 << tp->rx_opt.rcv_wscale);
>> +
>> +       if (free_space < (full_space >> 1)) {
>> +               icsk->icsk_ack.quick = 0;
>> +
>> +               if (tcp_under_memory_pressure(sk))
>> +                       tcp_adjust_rcv_ssthresh(sk);
>> +
>> +               /* if free space is too low, return a zero window */
>> +               if (free_space < (allowed_space >> 4) || free_space < mss ||
>> +                       free_space < (1 << tp->rx_opt.rcv_wscale))
>> +                       return 0;
> 
> Are you sure this block can not be shared with the existing one ?
> 
> Existing one has this added part:
> 
> free_space = round_down(free_space, 1 << tp->rx_opt.rcv_wscale);
> 
> Not sure why this would break the tcp_shrink_window == 1 case.

The main issue is the additional conditional on the if that returns 0:
    
    free_space < (1 << tp->rx_opt.rcv_wscale))

This is an important difference that I think needs to continue to be
different in the two cases.

I'll post a "v4" of the patch with the cleaned up code from the other 
comments so we can have a clean look at it.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ