Received-SPF: pass (google.com: domain of linux-kernel+bounces-215522-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Kuniyuki Iwashima <kuniyu@amazon.com>
To: <luoxuanqiang@kylinos.cn>
CC: <davem@davemloft.net>, <dccp@vger.kernel.org>, <dsahern@kernel.org>,
	<edumazet@google.com>, <fw@strlen.de>, <kuba@kernel.org>,
	<kuniyu@amazon.com>, <linux-kernel@vger.kernel.org>,
	<netdev@vger.kernel.org>, <pabeni@redhat.com>
Subject: Re: [PATCH net v2] Fix race for duplicate reqsk on identical SYN
Date: Fri, 14 Jun 2024 15:24:33 -0700
Message-ID: <20240614222433.19580-1-kuniyu@amazon.com>
In-Reply-To: <7075bb26-ede9-0dc7-fe93-e18703e5ddaa@kylinos.cn>
References: <7075bb26-ede9-0dc7-fe93-e18703e5ddaa@kylinos.cn>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

From: luoxuanqiang <luoxuanqiang@kylinos.cn>
Date: Fri, 14 Jun 2024 20:42:07 +0800
> 在 2024/6/14 18:54, Florian Westphal 写道:
> > luoxuanqiang <luoxuanqiang@kylinos.cn> wrote:
> >>   include/net/inet_connection_sock.h |  2 +-
> >>   net/dccp/ipv4.c                    |  2 +-
> >>   net/dccp/ipv6.c                    |  2 +-
> >>   net/ipv4/inet_connection_sock.c    | 15 +++++++++++----
> >>   net/ipv4/tcp_input.c               | 11 ++++++++++-
> >>   5 files changed, 24 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
> >> index 7d6b1254c92d..8773d161d184 100644
> >> --- a/include/net/inet_connection_sock.h
> >> +++ b/include/net/inet_connection_sock.h
> >> @@ -264,7 +264,7 @@ struct sock *inet_csk_reqsk_queue_add(struct sock *sk,
> >>   				      struct request_sock *req,
> >>   				      struct sock *child);
> >>   void inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req,
> >> -				   unsigned long timeout);
> >> +				   unsigned long timeout, bool *found_dup_sk);
> > Nit:
> >
> > I think it would be preferrable to change retval to bool rather than
> > bool *found_dup_sk extra arg, so one can do

+1


> >
> > bool inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req,
> >    				   unsigned long timeout)
> > {
> > 	if (!reqsk_queue_hash_req(req, timeout))
> > 		return false;
> >
> > i.e. let retval indicate wheter reqsk was inserted or not.
> >
> > Patch looks good to me otherwise.
> 
> Thank you for your confirmation!
> 
> Regarding your suggestion, I had considered it before,
> but besides tcp_conn_request() calling inet_csk_reqsk_queue_hash_add(),
> dccp_v4(v6)_conn_request() also calls it. However, there is no
> consideration for a failed insertion within that function, so it's
> reasonable to let the caller decide whether to check for duplicate
> reqsk.

I guess you followed 01770a1661657 where found_dup_sk was introduced,
but note that the commit is specific to TCP SYN Cookie and TCP Fast Open
and DCCP is not related.

Then, own_req is common to TCP and DCCP, so found_dup_sk was added as an
additional argument.

However, another similar commit 5e0724d027f05 actually added own_req check
in DCCP path.

I personally would'nt care if DCCP was not changed to handle such a
failure because DCCP will be removed next year, but I still prefer
Florian's suggestion.

Thanks