[PATCH] tcp: Keep updating window and checking for socket data after FIN from guest

[PATCH] tcp: Keep updating window and checking for socket data after FIN from guest

[Stefano Brivio]

Once we get a FIN segment from the container/guest, we enter something
resembling CLOSE_WAIT (from the perspective of the peer), but that
doesn't mean that we should stop processing window updates from the
guest and checking for socket data if the guest acknowledges
something.

If we don't do that, we can very easily run into a situation where we
send a burst of data to the tap, get a zero window update, along with
a FIN segment, because the flow is meant to be unidirectional, and now
the connection will be stuck forever, because we'll ignore updates.

Reproducer, server:

  $ pasta --config-net -t 9999 -- sh -c 'echo DONE | socat TCP-LISTEN:9997,shut-down STDIO'

and client:

  $ ./test/rampstream send 50000 | socat -u STDIN TCP:$LOCAL_ADDR:9997
  2025/02/13 09:14:45 socat[2997126] E write(5, 0x55f5dbf47000, 8192): Broken pipe

while at it, update the message string for the third passive close
state (which we see in this case): it's CLOSE_WAIT, not LAST_ACK.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tcp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tcp.c b/tcp.c
index a1d6c53..16d01f6 100644
--- a/tcp.c
+++ b/tcp.c
@@ -338,7 +338,7 @@ static const char *tcp_state_str[] __attribute((__unused__)) = {
 	"SYN_RCVD",	/* approximately maps to TAP_SYN_ACK_SENT */
 
 	/* Passive close: */
-	"CLOSE_WAIT", "CLOSE_WAIT", "LAST_ACK", "LAST_ACK", "LAST_ACK",
+	"CLOSE_WAIT", "CLOSE_WAIT", "CLOSE_WAIT", "LAST_ACK", "LAST_ACK",
 	/* Active close (+5): */
 	"CLOSING", "FIN_WAIT_1", "FIN_WAIT_1", "FIN_WAIT_2", "TIME_WAIT",
 };
@@ -1968,6 +1968,8 @@ 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_update_seqack_from_tap(c, conn, ntohl(th->ack_seq));
+		tcp_tap_window_update(conn, ntohs(th->window));
+		tcp_data_from_sock(c, conn);
 
 		if (conn->events & SOCK_FIN_RCVD &&
 		    conn->seq_ack_from_tap == conn->seq_to_tap)
-- 
@@ -338,7 +338,7 @@ static const char *tcp_state_str[] __attribute((__unused__)) = {
 	"SYN_RCVD",	/* approximately maps to TAP_SYN_ACK_SENT */
 
 	/* Passive close: */
-	"CLOSE_WAIT", "CLOSE_WAIT", "LAST_ACK", "LAST_ACK", "LAST_ACK",
+	"CLOSE_WAIT", "CLOSE_WAIT", "CLOSE_WAIT", "LAST_ACK", "LAST_ACK",
 	/* Active close (+5): */
 	"CLOSING", "FIN_WAIT_1", "FIN_WAIT_1", "FIN_WAIT_2", "TIME_WAIT",
 };
@@ -1968,6 +1968,8 @@ 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_update_seqack_from_tap(c, conn, ntohl(th->ack_seq));
+		tcp_tap_window_update(conn, ntohs(th->window));
+		tcp_data_from_sock(c, conn);
 
 		if (conn->events & SOCK_FIN_RCVD &&
 		    conn->seq_ack_from_tap == conn->seq_to_tap)
-- 
2.43.0

Re: [PATCH] tcp: Keep updating window and checking for socket data after FIN from guest

[David Gibson]

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

On Thu, Feb 13, 2025 at 11:16:44PM +0100, Stefano Brivio wrote:
> Once we get a FIN segment from the container/guest, we enter something
> resembling CLOSE_WAIT (from the perspective of the peer), but that
> doesn't mean that we should stop processing window updates from the
> guest and checking for socket data if the guest acknowledges
> something.
> 
> If we don't do that, we can very easily run into a situation where we
> send a burst of data to the tap, get a zero window update, along with
> a FIN segment, because the flow is meant to be unidirectional, and now
> the connection will be stuck forever, because we'll ignore updates.
> 
> Reproducer, server:
> 
>   $ pasta --config-net -t 9999 -- sh -c 'echo DONE | socat TCP-LISTEN:9997,shut-down STDIO'
> 
> and client:
> 
>   $ ./test/rampstream send 50000 | socat -u STDIN TCP:$LOCAL_ADDR:9997
>   2025/02/13 09:14:45 socat[2997126] E write(5, 0x55f5dbf47000, 8192): Broken pipe
> 
> while at it, update the message string for the third passive close
> state (which we see in this case): it's CLOSE_WAIT, not LAST_ACK.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  tcp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tcp.c b/tcp.c
> index a1d6c53..16d01f6 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -338,7 +338,7 @@ static const char *tcp_state_str[] __attribute((__unused__)) = {
>  	"SYN_RCVD",	/* approximately maps to TAP_SYN_ACK_SENT */
>  
>  	/* Passive close: */
> -	"CLOSE_WAIT", "CLOSE_WAIT", "LAST_ACK", "LAST_ACK", "LAST_ACK",
> +	"CLOSE_WAIT", "CLOSE_WAIT", "CLOSE_WAIT", "LAST_ACK", "LAST_ACK",
>  	/* Active close (+5): */
>  	"CLOSING", "FIN_WAIT_1", "FIN_WAIT_1", "FIN_WAIT_2", "TIME_WAIT",
>  };
> @@ -1968,6 +1968,8 @@ 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_update_seqack_from_tap(c, conn, ntohl(th->ack_seq));
> +		tcp_tap_window_update(conn, ntohs(th->window));
> +		tcp_data_from_sock(c, conn);
>  
>  		if (conn->events & SOCK_FIN_RCVD &&
>  		    conn->seq_ack_from_tap == conn->seq_to_tap)

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