Re: [PATCH 1/4] tcp: Fix ACK sequence on FIN to tap

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3871 bytes --]

On Thu, Oct 02, 2025 at 02:06:43AM +0200, Stefano Brivio wrote:
> If we reach end-of-file on a socket (or get EPOLLRDHUP / EPOLLHUP) and
> send a FIN segment to the guest / container acknowledging a sequence
> number that's behind what we received so far, we won't have any
> further trigger to send an updated ACK segment, as we are now
> switching the epoll socket monitoring to edge-triggered mode.
> 
> To avoid this situation, in tcp_update_seqack_wnd(), we set the next
> acknowledgement sequence to the current observed sequence, regardless
> of what was acknowledged socket-side.

To double check my understanding: things should work if we always
acknowledged everything we've received.  Acknowledging only what the
peer has acked is a refinement to give the guest a view that's closer
to what it would be end-to-end with the peer (which might improve the
operation of flow control).

We can't use that refined mechanism when the socket is closing
(amongst other cases), because while we can get the peer acked bytes
from TCP_INFO, we can't get events when that changes, so we have no
mechanism to provide updates to the guest at the right time.  So we
fall back to the simpler method.

Is that correct?

> However, we don't necessarily call tcp_update_seqack_wnd() before
> sending the FIN segment, which might potentially lead to a situation,
> not observed in practice, where we unnecessarily cause a
> retransmission at some point after our FIN segment.
> 
> Avoid that by setting the ACK sequence to whatever we received from
> the container / guest, before sending a FIN segment and switching to
> EPOLLET.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Based on my understanding above, this looks correct to me, so,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

My only concern is whether we could instead insert an extra call to
tcp_update_seqack_wnd() to reduce duplicated logic.

> ---
>  tcp_buf.c | 14 +++++++++++++-
>  tcp_vu.c  |  7 ++++++-
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/tcp_buf.c b/tcp_buf.c
> index a493b5a..cc106bc 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -368,7 +368,19 @@ int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>  			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);
> +			int ret;
> +
> +			/* On TAP_FIN_SENT, we won't get further data events
> +			 * from the socket, and this might be the last ACK
> +			 * segment we send to the tap, so update its sequence to
> +			 * include everything we received until now.
> +			 *
> +			 * See also the special handling on CONN_IS_CLOSING() in
> +			 * tcp_update_seqack_wnd().
> +			 */
> +			conn->seq_ack_to_tap = conn->seq_from_tap;
> +
> +			ret = tcp_buf_send_flag(c, conn, FIN | ACK);
>  			if (ret) {
>  				tcp_rst(c, conn);
>  				return ret;
> diff --git a/tcp_vu.c b/tcp_vu.c
> index ebd3a1e..3ec3538 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -410,7 +410,12 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>  			conn_flag(c, conn, STALLED);
>  		} else if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) ==
>  			   SOCK_FIN_RCVD) {
> -			int ret = tcp_vu_send_flag(c, conn, FIN | ACK);
> +			int ret;
> +
> +			/* See tcp_buf_data_from_sock() */
> +			conn->seq_ack_to_tap = conn->seq_from_tap;
> +
> +			ret = tcp_vu_send_flag(c, conn, FIN | ACK);
>  			if (ret) {
>  				tcp_rst(c, conn);
>  				return ret;
> -- 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]