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] [thread-next>] [day] [month] [year] [list]

Message-ID: <b20c01d5-5a8f-03e6-6573-ea46e0df5ebb@kylinos.cn>
Date: Mon, 17 Jun 2024 10:01:48 +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:
>> 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.

Hello Eric and Kuniyuk,

Thank you for the information!

I've tested the kernel versions 4.19 and 6.10, and they both have
similar issues (I suspect this problem has been around for quite some
time). My intention is to propose a fix to the more stable branches as
soon as possible to cover a wider range. Like Eric mentioned, I hope to
minimize conflicts, so I expect to keep the original DCCP logic intact
and refer to the check for found_dup_sk in 01770a1661657. For DCCP, if
insertion into ehash fails, we might also need to consider handling
rsk_refcnt, as tcp_conn_request() requires rsk_refcnt to be 0 to release
reqsk.

Of course, if DCCP will be removed from net-next, I agree with
Kuniyuki and Florian's suggestions and will envision a better commit
content.

BRs!