Received-SPF: pass (google.com: domain of linux-kernel+bounces-25596-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240112094603.23706-1-menglong8.dong@gmail.com> <20240113154632.GI392144@kernel.org>
In-Reply-To: <20240113154632.GI392144@kernel.org>
From: Menglong Dong <menglong8.dong@gmail.com>
Date: Mon, 15 Jan 2024 10:40:56 +0800
Message-ID: <CADxym3a6qNcb47R_DfXMsac9Ou_zkz5hR3bGY9tr7Jhsdw3y-Q@mail.gmail.com>
Subject: Re: [RFC PATCH net-next v2] net: tcp: accept old ack during closing
To: Simon Horman <horms@kernel.org>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Jan 13, 2024 at 11:46=E2=80=AFPM Simon Horman <horms@kernel.org> wr=
ote:
>
> 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=3D10)          FIN_WAIT1(Send FIN, seq=3D20, =
ack=3D10)
> >       |                                    |
> >       |                                Send ACK(seq=3D21, ack=3D11)
> >    Recv ACK(seq=3D21, ack=3D11)
> >       |
> >    Recv FIN(seq=3D20, ack=3D10)
> >
> > 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_A=
CK
> > 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, stru=
ct sk_buff *skb)
> >               return 0;
> >
> >       /* step 5: check the ACK field */
> > -     acceptable =3D tcp_ack(sk, skb, FLAG_SLOWPATH |
> > -                                   FLAG_UPDATE_TS_RECENT |
> > -                                   FLAG_NO_CHALLENGE_ACK) > 0;
> > +     reason =3D 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 <=3D 0) {
      ......
      if ((int)reason < 0) {
          ....
      }
  }

Thanks!
Menglong Dong

> >
> > -     if (!acceptable) {
> > +     if (reason <=3D 0) {
> >               if (sk->sk_state =3D=3D 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 =3D -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
> >