Received-SPF: pass (google.com: domain of linux-kernel+bounces-214911-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <7075bb26-ede9-0dc7-fe93-e18703e5ddaa@kylinos.cn>
Date: Fri, 14 Jun 2024 20:42:07 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH net v2] Fix race for duplicate reqsk on identical SYN
Content-Language: en-US
To: Florian Westphal <fw@strlen.de>
Cc: edumazet@google.com, davem@davemloft.net, dsahern@kernel.org,
 kuba@kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
 pabeni@redhat.com, kuniyu@amazon.com, dccp@vger.kernel.org
References: <20240614102628.446642-1-luoxuanqiang@kylinos.cn>
 <20240614105441.GA24596@breakpoint.cc>
From: luoxuanqiang <luoxuanqiang@kylinos.cn>
In-Reply-To: <20240614105441.GA24596@breakpoint.cc>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable


=E5=9C=A8 2024/6/14 18:54, Florian Westphal =E5=86=99=E9=81=93:
> 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_con=
nection_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_s=
ock *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
>
> 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.

The purpose of my modification this time is solely to confirm if a
reqsk for the same connection has already been inserted into the ehash.
If the insertion fails, inet_ehash_insert() will handle the
non-insertion gracefully, and I only need to release the duplicate
reqsk. I believe this change is minimal and effective.

Those are my considerations.