Received-SPF: pass (google.com: domain of linux-kernel+bounces-25944-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 15 Jan 2024 11:58:41 +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: <20240115115841.GS392144@kernel.org>
References: <20240112094603.23706-1-menglong8.dong@gmail.com>
 <20240113154632.GI392144@kernel.org>
 <CADxym3a6qNcb47R_DfXMsac9Ou_zkz5hR3bGY9tr7Jhsdw3y-Q@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CADxym3a6qNcb47R_DfXMsac9Ou_zkz5hR3bGY9tr7Jhsdw3y-Q@mail.gmail.com>

On Mon, Jan 15, 2024 at 10:40:56AM +0800, Menglong Dong wrote:
> On Sat, Jan 13, 2024 at 11:46 PM Simon Horman <horms@kernel.org> 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@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.
> >
> 
> 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.

..