| Message ID | 20240126040519.1846345-1-menglong8.dong@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 795a7dfbc3d95e4c7c09569f319f026f8c7f5a9c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3] net: tcp: accept old ack during closing | expand |
On Fri, Jan 26, 2024 at 12:05:19PM +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> > --- > v3: > - fix unsigned/signed compare error Thanks for the update. I wish this could be done without a cast. But I do see that what you have done is consistent with at least one other use of tcp_ack(). So from that point of view: Reviewed-by: Simon Horman <horms@kernel.org> ...
On Fri, Jan 26, 2024 at 5:04 AM Menglong Dong <menglong8.dong@gmail.com> 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> > Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 26 Jan 2024 12:05:19 +0800 you 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) > > [...] Here is the summary with links: - [net-next,v3] net: tcp: accept old ack during closing https://git.kernel.org/netdev/net-next/c/795a7dfbc3d9 You are awesome, thank you!
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index df7b13f0e5e0..2d20edf652e6 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); - if (!acceptable) { + if ((int)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 ((int)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 */
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> --- v3: - fix unsigned/signed compare error v2: - fix the compiling error --- net/ipv4/tcp_input.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)