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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240115115841.GS392144@kernel.org>
Date: Mon, 15 Jan 2024 11:58:41 +0000
From: Simon Horman <horms@...nel.org>
To: Menglong Dong <menglong8.dong@...il.com>
Cc: edumazet@...gle.com, davem@...emloft.net, dsahern@...nel.org,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
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 <horms@...nel.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@...il.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.

..