Received-SPF: pass (google.com: domain of linux-kernel+bounces-25327-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sat, 13 Jan 2024 15:46:32 +0000
From: Simon Horman <horms@kernel.org>
To: Menglong Dong <menglong8.dong@gmail.com>
Cc: edumazet@google.com, davem@davemloft.net, dsahern@kernel.org,
	kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH net-next v2] net: tcp: accept old ack during closing
Message-ID: <20240113154632.GI392144@kernel.org>
References: <20240112094603.23706-1-menglong8.dong@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240112094603.23706-1-menglong8.dong@gmail.com>

On Fri, Jan 12, 2024 at 05:46:03PM +0800, Menglong Dong wrote:
> For now, the packet with an old ack is not accepted if we are in
> FIN_WAIT1 state, which can cause retransmission. Taking the following
> case as an example:
> 
>     Client                               Server
>       |                                    |
>   FIN_WAIT1(Send FIN, seq=10)          FIN_WAIT1(Send FIN, seq=20, ack=10)
>       |                                    |
>       |                                Send ACK(seq=21, ack=11)
>    Recv ACK(seq=21, ack=11)
>       |
>    Recv FIN(seq=20, ack=10)
> 
> In the case above, simultaneous close is happening, and the FIN and ACK
> packet that send from the server is out of order. Then, the FIN will be
> dropped by the client, as it has an old ack. Then, the server has to
> retransmit the FIN, which can cause delay if the server has set the
> SO_LINGER on the socket.
> 
> Old ack is accepted in the ESTABLISHED and TIME_WAIT state, and I think
> it should be better to keep the same logic.
> 
> In this commit, we accept old ack in FIN_WAIT1/FIN_WAIT2/CLOSING/LAST_ACK
> states. Maybe we should limit it to FIN_WAIT1 for now?
> 
> Signed-off-by: Menglong Dong <menglong8.dong@gmail.com>
> ---
> v2:
> - fix the compiling error
> ---
>  net/ipv4/tcp_input.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index df7b13f0e5e0..70642bb08f3a 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6699,17 +6699,21 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
>  		return 0;
>  
>  	/* step 5: check the ACK field */
> -	acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH |
> -				      FLAG_UPDATE_TS_RECENT |
> -				      FLAG_NO_CHALLENGE_ACK) > 0;
> +	reason = tcp_ack(sk, skb, FLAG_SLOWPATH |
> +				  FLAG_UPDATE_TS_RECENT |
> +				  FLAG_NO_CHALLENGE_ACK);

Hi Menglong Dong,

Probably I am missing something terribly obvious,
but I am confused about the types used here.

The type of reason is enum skb_drop_reason.
For which, which on my system, the compiler uses an unsigned entity.
i.e. it is an unsigned integer.

But tcp_ack returns a (signed) int. And below reason is checked
for values less than zero, and negated. This doesn't seem right.

>  
> -	if (!acceptable) {
> +	if (reason <= 0) {
>  		if (sk->sk_state == TCP_SYN_RECV)
>  			return 1;	/* send one RST */
> -		tcp_send_challenge_ack(sk);
> -		SKB_DR_SET(reason, TCP_OLD_ACK);
> -		goto discard;
> +		/* accept old ack during closing */
> +		if (reason < 0) {
> +			tcp_send_challenge_ack(sk);
> +			reason = -reason;
> +			goto discard;
> +		}
>  	}
> +	SKB_DR_SET(reason, NOT_SPECIFIED);
>  	switch (sk->sk_state) {
>  	case TCP_SYN_RECV:
>  		tp->delivered++; /* SYN-ACK delivery isn't tracked in tcp_ack */
> -- 
> 2.39.2
>