Re: [net,v2] tcp: correct handling of extreme memory squeeze

On Mon, Jan 27, 2025 at 6:32 PM Stefano Brivio wrote:

On Mon, 27 Jan 2025 18:17:28 +0800 Jason Xing wrote:

...

I'm not that sure if it's a bug belonging to the Linux kernel.

It is, because for at least 20-25 years (before that it's a bit hard to understand from history) a non-zero window would be announced, as obviously expected, once there's again space in the receive window.

Sorry for the late reply. I think the key of this problem is what should we do when we receive a tcp packet and we are out of memory. The RFC doesn't define such a thing, so in the commit e2142825c120 ("net: tcp: send zero-window ACK when no memory"), I reply with a zero-window ACK to the peer. And the peer will keep probing the window by retransmitting the packet that we dropped if the peer is a LINUX SYSTEM. As I said, the RFC doesn't define such a case, so the behavior of the peer is undefined if it is not a LINUX SYSTEM. If the peer doesn't keep retransmitting the packet, it will hang the connection, just like the problem that described in this commit log. However, we can make some optimization to make it more adaptable. We can send a ACK with the right window to the peer when the memory is available, and __tcp_cleanup_rbuf() is a good choice. Generally speaking, I think this patch makes sense. However, I'm not sure if there is any other influence if we make "tp->rcv_wnd=0", but it can trigger a ACK in __tcp_cleanup_rbuf(). Following is the code that I thought before to optimize this case (the code is totally not tested): diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h index 3c82fad904d4..bedd78946762 100644 --- a/include/net/inet_connection_sock.h +++ b/include/net/inet_connection_sock.h @@ -116,7 +116,8 @@ struct inet_connection_sock { #define ATO_BITS 8 __u32 ato:ATO_BITS, /* Predicted tick of soft clock */ lrcv_flowlabel:20, /* last received ipv6 flowlabel */ - unused:4; + is_oom:1, + unused:3; unsigned long timeout; /* Currently scheduled timeout */ __u32 lrcvtime; /* timestamp of last received data packet */ __u16 last_seg_size; /* Size of last incoming segment */ diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 0d704bda6c41..6f3c85a1f4da 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -1458,11 +1458,11 @@ static int tcp_peek_sndq(struct sock *sk, struct msghdr *msg, int len) */ void __tcp_cleanup_rbuf(struct sock *sk, int copied) { + struct inet_connection_sock *icsk = inet_csk(sk); struct tcp_sock *tp = tcp_sk(sk); bool time_to_ack = false; if (inet_csk_ack_scheduled(sk)) { - const struct inet_connection_sock *icsk = inet_csk(sk); if (/* Once-per-two-segments ACK was not sent by tcp_input.c */ tp->rcv_nxt - tp->rcv_wup > icsk->icsk_ack.rcv_mss || @@ -1502,6 +1502,11 @@ void __tcp_cleanup_rbuf(struct sock *sk, int copied) time_to_ack = true; } } + if (unlikely(icsk->icsk_ack.is_oom)) { + icsk->icsk_ack.is_oom = false; + time_to_ack = true; + } + if (time_to_ack) tcp_send_ack(sk); } diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 0e5b9a654254..e2d65213b3b7 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -268,9 +268,12 @@ static u16 tcp_select_window(struct sock *sk) * are out of memory. The window is temporary, so we don't store * it on the socket. */ - if (unlikely(inet_csk(sk)->icsk_ack.pending & ICSK_ACK_NOMEM)) + if (unlikely(inet_csk(sk)->icsk_ack.pending & ICSK_ACK_NOMEM)) { + inet_csk(sk)->icsk_ack.is_oom = true; return 0; + } + inet_csk(sk)->icsk_ack.is_oom = false; cur_win = tcp_receive_window(tp); new_win = __tcp_select_window(sk); if (new_win < cur_win) {

...

The other side not sending a window probe causes this issue...?

It doesn't cause this issue, but it triggers it.

...

The other part of me says we cannot break the user's behaviour.

This sounds quite relevant, yes.

-- Stefano