Re: [PATCH v4 8/8] tcp: Don't send FIN segment to guest yet if we have pending unacknowledged data

[Jon Maloy <jmaloy@redhat.com>]

On 2025-09-09 14:16, 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 <pholzing@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: Jon Maloy <jmaloy@redhat.com>

> ---
>   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);