tcp: Don't send FIN segment to guest yet if we have pending unacknowledged data

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 <pholzing@redhat.com>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Tested-by: Paul Holzinger <pholzing@redhat.com>
Reviewed-by: Jon Maloy <jmaloy@redhat.com>

1 files changed, 4 insertions, 1 deletions

diff --git a/tcp_buf.c b/tcp_buf.c
index bc898de..b02d986 100644
--- a/tcp_buf.c
+++ b/tcp_buf.c
@@ -369,7 +369,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);