netdev - Re: [PATCH net] tcp: tcp_acceptable_seq select SND.UNA when SND.WND is 0

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: <CAHx7jf_kZhR__FU--grcZgVd3bC4SZXScn3CJJBZBpspv9GSnQ@mail.gmail.com>
Date: Wed, 16 Apr 2025 17:59:05 -0300
From: Luiz Carlos Mourão Paes de Carvalho <luizcmpc@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>, 
	Neal Cardwell <ncardwell@...gle.com>
Subject: Re: [PATCH net] tcp: tcp_acceptable_seq select SND.UNA when SND.WND
 is 0

Hi Paolo,

The dropped ack is a response to data sent by the peer.

Peer sends a chunk of data, we ACK with an incorrect SEQ (SND.NXT)
that gets dropped
by the peer's tcp_sequence function. Connection only advances when we
send a RTO.

Let me know if the following describes the scenario you expected. I'll
add a packetdrill with
the expected interaction to the patch if it makes sense.

// Tests the invalid SEQs sent by the listener
// which are then dropped by the client.

`./common/defaults.sh
./common/set_sysctls.py /proc/sys/net/ipv4/tcp_shrink_window=0`

    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

   +0 < S 0:0(0) win 8 <mss 1000,sackOK,nop,nop,nop,wscale 7>
   +0 > S. 0:0(0) ack 1 <...>
  +.1 < . 1:1(0) ack 1 win 8
   +0 accept(3, ..., ...) = 4

   +0 write(4, ..., 990) = 990
   +0 > P. 1:991(990) ack 1
   +0 < .  1:1(0) ack 991 win 8           // win=8 despite buffer
being almost full, shrink_window=0

   +0 write(4, ..., 100) = 100
   +0 > P. 991:1091(100) ack 1            // SND.NXT=1091
   +0 < .  1:1(0) ack 991 win 0           // failed to queue rx data,
RCV.NXT=991, RCV.WND=0

 +0.1 < P. 1:1001(1000) ack 901 win 0
   +0 > .  1091:1091(0) ack 1001          // dropped on tcp_sequence,
note that SEQ=1091, while (RCV.NXT + RCV.WND)=991:
                                          // if (after(seq,
tp->rcv_nxt + tcp_receive_window(tp)))
                                          //     return
SKB_DROP_REASON_TCP_INVALID_SEQUENCE;

 +0.2 > P. 991:1091(100) ack 1001         // this is a RTO, ack accepted
   +0 < P. 1001:2001(1000) ack 991 win 0  // peer responds, still no
space available, but has more data to send
   +0 > .  1091:1091(0) ack 2001          // ack dropped

 +0.3 > P. 991:1091(100) ack 2001         // RTO, ack accepted
   +0 < .  2001:3001(1000) ack 991 win 0  // still no space available,
but another chunk of data
   +0 > .  1091:1091(0) ack 3001          // ack dropped

 +0.6 > P. 991:1091(100) ack 3001         // RTO, ack accepted
   +0 < .  3001:4001(1000) ack 991 win 0  // no space available, but
peer has data to send at all times
   +0 > .  1091:1091(0) ack 4001          // ack dropped

 +1.2 > P. 991:1091(100) ack 4001         // another probe, accepted

  // this goes on and on. note that the peer always has data just
waiting there to be sent,
  // server acks it, but the ack is dropped because SEQ is incorrect.
  // only the probes are advancing the connection, but are back-offed
every time.

// Reset sysctls
`/tmp/sysctl_restore_${PPID}.sh`

Luiz Carvalho

On Tue, Apr 15, 2025 at 8:30 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
>
>
> On 4/10/25 7:50 PM, Luiz Carvalho wrote:
> > The current tcp_acceptable_seq() returns SND.NXT when the available
> > window shrinks to less then one scaling factor. This works fine for most
> > cases, and seemed to not be a problem until a slight behavior change to
> > how tcp_select_window() handles ZeroWindow cases.
> >
> > Before commit 8c670bdfa58e ("tcp: correct handling of extreme memory squeeze"),
> > a zero window would only be announced when data failed to be consumed,
> > and following packets would have non-zero windows despite the receiver
> > still not having any available space. After the commit, however, the
> > zero window is stored in the socket and the advertised window will be
> > zero until the receiver frees up space.
> >
> > For tcp_acceptable_seq(), a zero window case will result in SND.NXT
> > being sent, but the problem now arises when the receptor validates the
> > sequence number in tcp_sequence():
> >
> > static enum skb_drop_reason tcp_sequence(const struct tcp_sock *tp,
> >                                        u32 seq, u32 end_seq)
> > {
> >       // ...
> >       if (after(seq, tp->rcv_nxt + tcp_receive_window(tp)))
> >               return SKB_DROP_REASON_TCP_INVALID_SEQUENCE;
> >       // ...
> > }
> >
> > Because RCV.WND is now stored in the socket as zero, using SND.NXT will fail
> > the INVALID_SEQUENCE check: SEG.SEQ <= RCV.NXT + RCV.WND. A valid ACK is
> > dropped by the receiver, correctly, as RFC793 mentions:
> >
> >       There are four cases for the acceptability test for an incoming
> >         segment:
> >
> >       Segment Receive  Test
> >         Length  Window
> >         ------- -------  -------------------------------------------
> >
> >            0       0     SEG.SEQ = RCV.NXT
> >
> > The ACK will be ignored until tcp_write_wakeup() sends SND.UNA again,
> > and the connection continues. If the receptor announces ZeroWindow
> > again, the stall could be very long, as was in my case. Found this out
> > while giving a shot at bug #213827.
>
> The dropped ack causing the stall is a zero window probe from the sender
> right?
> Could you please describe the relevant scenario with a pktdrill test?
>
> Thanks!
>
> Paolo
>