[PATCH] tcp: Don't reset outbound connection on SYN retries

[PATCH] tcp: Don't reset outbound connection on SYN retries

[Stefano Brivio]

Reported by somebody on IRC: if the server has considerable latency,
it might happen that the client retries sending SYN segments for the
same flow while we're still in a TAP_SYN_RCVD, non-ESTABLISHED state.

In that case, we should go with the blanket assumption that we need
to reset the connection on any unexpected segment: RFC 9293 explicitly
mentions this case in Figure 8: Recovery from Old Duplicate SYN,
section 3.5. It doesn't make sense for us to set a specific sequence
number, socket-side, but we should definitely wait and see.

Ignoring the duplicate SYN segment should also be compatible with
section 3.10.7.3. SYN-SENT STATE, which mentions updating sequences
socket-side (which we can't do anyway), but certainly not reset the
connection.

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

diff --git a/tcp.c b/tcp.c
index 7787381..51ad692 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1920,6 +1920,9 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 
 	/* Establishing connection from tap */
 	if (conn->events & TAP_SYN_RCVD) {
+		if (th->syn && !th->ack && !th->fin)
+			return 1;	/* SYN retry: ignore and keep waiting */
+
 		if (!(conn->events & TAP_SYN_ACK_SENT))
 			goto reset;
 
-- 
@@ -1920,6 +1920,9 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 
 	/* Establishing connection from tap */
 	if (conn->events & TAP_SYN_RCVD) {
+		if (th->syn && !th->ack && !th->fin)
+			return 1;	/* SYN retry: ignore and keep waiting */
+
 		if (!(conn->events & TAP_SYN_ACK_SENT))
 			goto reset;
 
-- 
2.43.0

Re: [PATCH] tcp: Don't reset outbound connection on SYN retries

[David Gibson]

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

On Mon, Feb 03, 2025 at 08:25:58AM +0100, Stefano Brivio wrote:
> Reported by somebody on IRC: if the server has considerable latency,
> it might happen that the client retries sending SYN segments for the
> same flow while we're still in a TAP_SYN_RCVD, non-ESTABLISHED state.
> 
> In that case, we should go with the blanket assumption that we need
> to reset the connection on any unexpected segment: RFC 9293 explicitly
> mentions this case in Figure 8: Recovery from Old Duplicate SYN,
> section 3.5. It doesn't make sense for us to set a specific sequence
> number, socket-side, but we should definitely wait and see.
> 
> Ignoring the duplicate SYN segment should also be compatible with
> section 3.10.7.3. SYN-SENT STATE, which mentions updating sequences
> socket-side (which we can't do anyway), but certainly not reset the
> connection.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

I haven't dug into the RFC to check this thoroughly, but it makes
intuitive sense to me.

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

> ---
>  tcp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tcp.c b/tcp.c
> index 7787381..51ad692 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1920,6 +1920,9 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  
>  	/* Establishing connection from tap */
>  	if (conn->events & TAP_SYN_RCVD) {
> +		if (th->syn && !th->ack && !th->fin)
> +			return 1;	/* SYN retry: ignore and keep waiting */
> +
>  		if (!(conn->events & TAP_SYN_ACK_SENT))
>  			goto reset;
>  

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