2024-01-12 10:02:45

by Menglong Dong

Subject: [RFC PATCH net-next v2] net: tcp: accept old ack during closing

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 <[email protected]>
---
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);

- 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

2024-01-13 15:46:50

by Simon Horman

Subject: Re: [RFC PATCH net-next v2] net: tcp: accept old ack during closing

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

2024-01-15 02:41:22

by Menglong Dong

Subject: Re: [RFC PATCH net-next v2] net: tcp: accept old ack during closing

On Sat, Jan 13, 2024 at 11:46 PM Simon Horman <[email protected]> wrote:
>
> 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 <[email protected]>
> > ---
> > 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.
>

Hello! You are right, and it seems that I make the same
mistake with Eric in this commit:

843f77407eeb ("tcp: fix signed/unsigned comparison")

I should convert it to signed int before comparing it
like this:

if ((int)reason <= 0) {
......
if ((int)reason < 0) {
....
}
}

Thanks!
Menglong Dong

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

2024-01-15 11:58:55

by Simon Horman

Subject: Re: [RFC PATCH net-next v2] net: tcp: accept old ack during closing

On Mon, Jan 15, 2024 at 10:40:56AM +0800, Menglong Dong wrote:
> On Sat, Jan 13, 2024 at 11:46 PM Simon Horman <[email protected]> wrote:
> >
> > 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 <[email protected]>
> > > ---
> > > 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.
> >
>
> Hello! You are right, and it seems that I make the same
> mistake with Eric in this commit:
>
> 843f77407eeb ("tcp: fix signed/unsigned comparison")
>
> I should convert it to signed int before comparing it
> like this:
>
> if ((int)reason <= 0) {
> ......
> if ((int)reason < 0) {
> ....
> }
> }

Thanks. FWIIW, I would probably assign the unsigned value to an unsigned
local variable.

..