[PATCH] tcp: Flush socket before checking for more data in active close state

[PATCH] tcp: Flush socket before checking for more data in active close state

[Stefano Brivio]

Otherwise, if all the pending data is acknowledged:

- tcp_update_seqack_from_tap() updates the current tap-side ACK
  sequence (conn->seq_ack_from_tap)

- next, we compare the sequence we sent (conn->seq_to_tap) to the
  ACK sequence (conn->seq_ack_from_tap) in tcp_data_from_sock() to
  understand if there's more data we can send.

  If they match, we conclude that we haven't sent any of that data,
  and keep re-sending it.

We need, instead, to flush the socket (drop acknowledged data) before
calling tcp_update_seqack_from_tap(), so that once we update
conn->seq_ack_from_tap, we can be sure that all data until there is
gone from the socket.

Link: https://bugs.passt.top/show_bug.cgi?id=114
Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Fixes: 30f1e082c3c0 ("tcp: Keep updating window and checking for socket data after FIN from guest")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tcp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tcp.c b/tcp.c
index 68af43d..fa1d885 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2049,6 +2049,7 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 
 	/* Established connections not accepting data from tap */
 	if (conn->events & TAP_FIN_RCVD) {
+		tcp_sock_consume(conn, ntohl(th->ack_seq));
 		tcp_update_seqack_from_tap(c, conn, ntohl(th->ack_seq));
 		tcp_tap_window_update(conn, ntohs(th->window));
 		tcp_data_from_sock(c, conn);
-- 
@@ -2049,6 +2049,7 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 
 	/* Established connections not accepting data from tap */
 	if (conn->events & TAP_FIN_RCVD) {
+		tcp_sock_consume(conn, ntohl(th->ack_seq));
 		tcp_update_seqack_from_tap(c, conn, ntohl(th->ack_seq));
 		tcp_tap_window_update(conn, ntohs(th->window));
 		tcp_data_from_sock(c, conn);
-- 
2.43.0

Re: [PATCH] tcp: Flush socket before checking for more data in active close state

[David Gibson]

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

On Wed, Mar 19, 2025 at 08:05:19PM +0100, Stefano Brivio wrote:
> Otherwise, if all the pending data is acknowledged:
> 
> - tcp_update_seqack_from_tap() updates the current tap-side ACK
>   sequence (conn->seq_ack_from_tap)
> 
> - next, we compare the sequence we sent (conn->seq_to_tap) to the
>   ACK sequence (conn->seq_ack_from_tap) in tcp_data_from_sock() to
>   understand if there's more data we can send.
> 
>   If they match, we conclude that we haven't sent any of that data,
>   and keep re-sending it.
> 
> We need, instead, to flush the socket (drop acknowledged data) before
> calling tcp_update_seqack_from_tap(), so that once we update
> conn->seq_ack_from_tap, we can be sure that all data until there is
> gone from the socket.

IIUC, this is, roughly speaking, a manifestation of the difference
between "seq ack from tap" and "seq ack to sock".  We don't track the
latter, because it's kind of implied by the former.  However, with the
incorrect ordering here the brief window in which they're not the same
is biting us.

> Link: https://bugs.passt.top/show_bug.cgi?id=114
> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Fixes: 30f1e082c3c0 ("tcp: Keep updating window and checking for socket data after FIN from guest")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  tcp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tcp.c b/tcp.c
> index 68af43d..fa1d885 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2049,6 +2049,7 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  
>  	/* Established connections not accepting data from tap */
>  	if (conn->events & TAP_FIN_RCVD) {
> +		tcp_sock_consume(conn, ntohl(th->ack_seq));
>  		tcp_update_seqack_from_tap(c, conn, ntohl(th->ack_seq));
>  		tcp_tap_window_update(conn, ntohs(th->window));
>  		tcp_data_from_sock(c, conn);

-- 
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 --]