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

[Menglong Dong]

On Sat, Jan 13, 2024 at 11:46 PM Simon Horman <horms@xxxxxxxxxx> 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 <menglong8.dong@xxxxxxxxx>
> > ---
> > 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
> >