[PATCH v4 8/8] 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 Signed-off-by: Stefano Brivio --- 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