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

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

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