Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 23 Oct 2020 16:51:45 +0100
From:   Ricardo Dias <rdias@memsql.com>
To:     Eric Dumazet <edumazet@google.com>
Cc:     David Miller <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] tcp: fix race condition when creating child sockets from
 syncookies
Message-ID: <20201023155145.GA316015@rdias-suse-pc.lan>
References: <20201023111352.GA289522@rdias-suse-pc.lan>
 <CANn89iJDt=XpUZA_uYK98cK8tctW6M=f4RFtGQpTxRaqwnnqSQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANn89iJDt=XpUZA_uYK98cK8tctW6M=f4RFtGQpTxRaqwnnqSQ@mail.gmail.com>
Precedence: bulk

On Fri, Oct 23, 2020 at 04:03:27PM +0200, Eric Dumazet wrote:
> On Fri, Oct 23, 2020 at 1:14 PM Ricardo Dias <rdias@memsql.com> wrote:
> >
> > When the TCP stack is in SYN flood mode, the server child socket is
> > created from the SYN cookie received in a TCP packet with the ACK flag
> > set.
> >
> ...
> 
> This patch only handles IPv4, unless I am missing something ?

Yes, currently the patch only handles IPv4. I'll improve it to also
handle the IPv6 case. 

> 
> It looks like the fix should be done in inet_ehash_insert(), not
> adding yet another helper in TCP.
> This would be family generic.

Ok, sounds good as long as there is not problem in changing the
signature and semantics of the inet_ehash_insert() function, as well as
changing the inet_ehash_nolisten() function.

> 
> Note that normally, all packets for the same 4-tuple should be handled
> by the same cpu,
> so this race is quite unlikely to happen in standard setups.

I was able to write a small client/server program that used the
loopback interface to create connections, which could hit the race
condition in 1/200 runs.

The server when accepts a connection sends an 8 byte identifier to
the client, and then waits for the client to echo the same identifier.
The client creates hundreds of simultaneous connections to the server,
and in each connection it sends one byte as soon as the connection is
established, then reads the 8 byte identifier from the server and sends
it back to the server.

When we hit the race condition, one of the server connections gets an 8
byte identifier different from its own identifier.