On Tue, Sep 09, 2025 at 08:16:55PM +0200, Stefano Brivio wrote: > For some reason, tcp_vu_data_from_sock() already takes care of this, > but the non-vhost-user version ignores this possibility and just sends > out a FIN segment whenever we infer we received one host-side, > regardless of the fact that we might have unacknowledged data still to > send. > > Somewhat surprisingly, this didn't cause any issue to be reported yet, > until 6.17-rc1 and 1d2fbaad7cd8 ("tcp: stronger sk_rcvbuf checks") > came around, leading to the following report from Paul, who hit this > running Podman tests: > > 439 0.033032 169.254.1.2 → 192.168.122.100 65540 TCP 56602 → 5789 [PSH, ACK] Seq=10336361 Ack=1 Win=65536 Len=65480 > 440 0.033055 169.254.1.2 → 192.168.122.100 30324 TCP [TCP Window Full] 56602 → 5789 [PSH, ACK] Seq=10401841 Ack=1 Win=65536 Len=30264 > > we're sending data to the container, up to the edge of the window > > 441 0.033059 192.168.122.100 → 169.254.1.2 60 TCP 5789 → 56602 [ACK] Seq=1 Ack=10401841 Win=83968 Len=0 > > and the container acknowledges it > > 442 0.033091 169.254.1.2 → 192.168.122.100 53716 TCP 56602 → 5789 [PSH, ACK] Seq=10432105 Ack=1 Win=65536 Len=53656 > > we send more data, all we possibly can, in window > > 443 0.033126 192.168.122.100 → 169.254.1.2 60 TCP [TCP ZeroWindow] 5789 → 56602 [ACK] Seq=1 Ack=10432105 Win=0 Len=0 > > and the container shrinks the window due to the issue introduced > by kernel commit e2142825c120 ("net: tcp: send zero-window ACK when no > memory"). With a previous patch from this series, we rewind the > sequence, meaning that we assign conn->seq_to_tap from > conn->seq_ack_from_tap, so that we'll retransmit this segment, by > reading again from the socket, and increasing conn->seq_to_tap > once more. > > However: > > 444 0.033144 169.254.1.2 → 192.168.122.100 60 TCP 56602 → 5789 [FIN, PSH, ACK] Seq=10485761 Ack=1 Win=65536 Len=0 > > we eventually get a zero-length read from the socket and we miss the > fact that conn->seq_to_tap != conn->seq_ack_from_tap, so we send a > FIN flag with the most recent sequence. The kernel insists: > > 445 0.033147 192.168.122.100 → 169.254.1.2 60 TCP [TCP ZeroWindow] 5789 → 56602 [ACK] Seq=1 Ack=10432105 Win=0 Len=0 > > with its buggy zero-window update, but: > > 446 0.033152 192.168.122.100 → 169.254.1.2 60 TCP [TCP Window Update] 5789 → 56602 [ACK] Seq=1 Ack=10432105 Win=69120 Len=0 > 447 0.033202 192.168.122.100 → 169.254.1.2 60 TCP [TCP Window Update] 5789 → 56602 [ACK] Seq=1 Ack=10432105 Win=142848 Len=0 > > we don't reset the TAP_FIN_SENT flag anymore, and don't resend > the FIN segment (nor data), as we already rewound the sequence > earlier. > > To solve this, hold off the FIN segment until we get a zero-length > read from the socket *and* we know that there's no unacknowledged > pending data, also without vhost-user, in tcp_buf_data_from_sock(). > > Reported-by: Paul Holzinger > Signed-off-by: Stefano Brivio Reviewed-by: David Gibson At least, I think this makes sense. As always, I find the semantics of the STALLED flag difficult to wrap my head around. > --- > tcp_buf.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/tcp_buf.c b/tcp_buf.c > index 4ebb013..49bddbe 100644 > --- a/tcp_buf.c > +++ b/tcp_buf.c > @@ -363,7 +363,10 @@ int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn) > } > > if (!len) { > - if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) == SOCK_FIN_RCVD) { > + if (already_sent) { > + conn_flag(c, conn, STALLED); > + } else if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) == > + SOCK_FIN_RCVD) { > int ret = tcp_buf_send_flag(c, conn, FIN | ACK); > if (ret) { > tcp_rst(c, conn); > -- > 2.43.0 > -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson