[PATCH] tcp: Set ACK flag on *all* RST segments, even for client in SYN-SENT state

[PATCH] tcp: Set ACK flag on *all* RST segments, even for client in SYN-SENT state

[Stefano Brivio]

Somewhat curiously, RFC 9293, section 3.10.7.3, states:

   If the state is SYN-SENT, then
   [...]

      Second, check the RST bit:
      -  If the RST bit is set,
      [...]

         o  If the ACK was acceptable, then signal to the user "error:
            connection reset", drop the segment, enter CLOSED state,
            delete TCB, and return.  Otherwise (no ACK), drop the
            segment and return.

which matches verbatim RFC 793, pages 66-67, and is implemented as-is
by tcp_rcv_synsent_state_process() in the Linux kernel, that is:

	/* No ACK in the segment */

	if (th->rst) {
		/* rfc793:
		 * "If the RST bit is set
		 *
		 *      Otherwise (no ACK) drop the segment and return."
		 */

		goto discard_and_undo;
	}

meaning that if a client is in SYN-SENT state, and we send a RST
segment once we realise that we can't establish the outbound
connection, the client will ignore our segment and will need to
pointlessly wait until the connection times out instead of aborting
it right away.

The ACK flag on a RST, in this case, doesn't really seem to have any
function, but we must set it nevertheless. The ACK sequence number is
already correct because we always set it before calling
tcp_prepare_flags(), whenever relevant.

This leaves us with no cases where we should *not* set the ACK flag
on non-SYN segments, so always set the ACK flag for RST segments.

Note that non-SYN, non-RST segments were already covered by commit
4988e2b40631 ("tcp: Unconditionally force ACK for all !SYN, !RST
packets").

Reported-by: Dirk Janssen <Dirk.Janssen@schiphol.nl>
Reported-by: Roeland van de Pol <Roeland.van.de.Pol@schiphol.nl>
Reported-by: Robert Floor <Robert.Floor@schiphol.nl>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcp.c b/tcp.c
index 4d6a6b3..c89f323 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1147,7 +1147,7 @@ int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn,
 
 		*opts = TCP_SYN_OPTS(mss, conn->ws_to_tap);
 		*optlen = sizeof(*opts);
-	} else if (!(flags & RST)) {
+	} else {
 		flags |= ACK;
 	}
 
-- 
@@ -1147,7 +1147,7 @@ int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn,
 
 		*opts = TCP_SYN_OPTS(mss, conn->ws_to_tap);
 		*optlen = sizeof(*opts);
-	} else if (!(flags & RST)) {
+	} else {
 		flags |= ACK;
 	}
 
-- 
2.43.0

Re: [PATCH] tcp: Set ACK flag on *all* RST segments, even for client in SYN-SENT state

[David Gibson]

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

On Mon, Jan 20, 2025 at 07:15:20PM +0100, Stefano Brivio wrote:
> Somewhat curiously, RFC 9293, section 3.10.7.3, states:
> 
>    If the state is SYN-SENT, then
>    [...]
> 
>       Second, check the RST bit:
>       -  If the RST bit is set,
>       [...]
> 
>          o  If the ACK was acceptable, then signal to the user "error:
>             connection reset", drop the segment, enter CLOSED state,
>             delete TCB, and return.  Otherwise (no ACK), drop the
>             segment and return.
> 
> which matches verbatim RFC 793, pages 66-67, and is implemented as-is
> by tcp_rcv_synsent_state_process() in the Linux kernel, that is:
> 
> 	/* No ACK in the segment */
> 
> 	if (th->rst) {
> 		/* rfc793:
> 		 * "If the RST bit is set
> 		 *
> 		 *      Otherwise (no ACK) drop the segment and return."
> 		 */
> 
> 		goto discard_and_undo;
> 	}
> 
> meaning that if a client is in SYN-SENT state, and we send a RST
> segment once we realise that we can't establish the outbound
> connection, the client will ignore our segment and will need to
> pointlessly wait until the connection times out instead of aborting
> it right away.
> 
> The ACK flag on a RST, in this case, doesn't really seem to have any
> function, but we must set it nevertheless. The ACK sequence number is
> already correct because we always set it before calling
> tcp_prepare_flags(), whenever relevant.
> 
> This leaves us with no cases where we should *not* set the ACK flag
> on non-SYN segments, so always set the ACK flag for RST segments.
> 
> Note that non-SYN, non-RST segments were already covered by commit
> 4988e2b40631 ("tcp: Unconditionally force ACK for all !SYN, !RST
> packets").
> 
> Reported-by: Dirk Janssen <Dirk.Janssen@schiphol.nl>
> Reported-by: Roeland van de Pol <Roeland.van.de.Pol@schiphol.nl>
> Reported-by: Robert Floor <Robert.Floor@schiphol.nl>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 4d6a6b3..c89f323 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1147,7 +1147,7 @@ int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn,
>  
>  		*opts = TCP_SYN_OPTS(mss, conn->ws_to_tap);
>  		*optlen = sizeof(*opts);
> -	} else if (!(flags & RST)) {
> +	} else {
>  		flags |= ACK;
>  	}
>  

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