netdev - Re: [PATCH net v2] Fix race for duplicate reqsk on identical SYN

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: <22779d46-8e8b-4ea5-07d6-bebb17a04051@kylinos.cn>
Date: Mon, 17 Jun 2024 16:07:49 +0800
From: luoxuanqiang <luoxuanqiang@...inos.cn>
To: Eric Dumazet <edumazet@...gle.com>, Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, dccp@...r.kernel.org, dsahern@...nel.org,
 fw@...len.de, kuba@...nel.org, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net v2] Fix race for duplicate reqsk on identical SYN


在 2024/6/15 14:40, Eric Dumazet 写道:
> On Sat, Jun 15, 2024 at 12:24 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> 你好 Eric和Kuniyuk,
>> From: luoxuanqiang <luoxuanqiang@...inos.cn>
>> Date: Fri, 14 Jun 2024 20:42:07 +0800
>>> 在 2024/6/14 18:54, Florian Westphal 写道:
>>>> luoxuanqiang <luoxuanqiang@...inos.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.
>>
> Other things to consider :
>
> - I presume this patch targets net tree, and luoxuanqiang needs the
> fix to reach stable trees.
>
> - This means a Fixes: tag is needed
>
> - This also means that we should favor a patch with no or trivial
> conflicts for stable backports.
>
> Should the patch target the net-next tree, then the requirements can
> be different.

Hi Kuniyuk and Florian,

I've created version 3 based on your suggestions, but I've kept the use
of 'found_dup_sk' since we need to pass NULL in DCCP to maintain its
logic unchanged. Could you please review this update and let me know if
it's okay? Thank you!

BRs!