netdev - Re: [PATCH v2 net] udp: Update reuse->has_conns under reuseport

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: <c4f74864-0a6a-5075-891c-d20d0dc20f2f@linux.dev>
Date:   Thu, 13 Oct 2022 10:41:53 -0700
From:   Martin KaFai Lau <martin.lau@...ux.dev>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>
Cc:     davem@...emloft.net, dsahern@...nel.org,
        Eric Dumazet <edumazet@...gle.com>, kraig@...gle.com,
        kuba@...nel.org, kuni1840@...il.com, martin.lau@...nel.org,
        netdev@...r.kernel.org, pabeni@...hat.com, willemb@...gle.com,
        yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH v2 net] udp: Update reuse->has_conns under reuseport_lock.

On 10/13/22 9:09 AM, Eric Dumazet wrote:
>>>> diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
>>>> index 5daa1fa54249..abb414ed4aa7 100644
>>>> --- a/net/core/sock_reuseport.c
>>>> +++ b/net/core/sock_reuseport.c
>>>> @@ -21,6 +21,21 @@ static DEFINE_IDA(reuseport_ida);
>>>>   static int reuseport_resurrect(struct sock *sk, struct sock_reuseport *old_reuse,
>>>>                                 struct sock_reuseport *reuse, bool bind_inany);
>>>>
>>>> +void reuseport_has_conns_set(struct sock *sk)
>>>> +{
>>>> +       struct sock_reuseport *reuse;
>>>> +
>>>> +       if (!rcu_access_pointer(sk->sk_reuseport_cb))
>>>> +               return;
>>>> +
>>>> +       spin_lock(&reuseport_lock);

It seems other paths are still using the spin_lock_bh().  It will be useful to 
have a few words here why _bh() is not needed.

>>>> +       reuse = rcu_dereference_protected(sk->sk_reuseport_cb,
>>>> +                                         lockdep_is_held(&reuseport_lock));
>>>
>>> Could @reuse be NULL at this point ?
>>>
>>> Previous  test was performed without reuseport_lock being held.
>>
>> Usually, sk_reuseport_cb is changed under lock_sock().
>>
>> The only exception is reuseport_grow() & TCP reqsk migration case.
>>
>> 1) shutdown() TCP listener, which is moved into the latter part of
>>     reuse->socks[] to migrate reqsk.
>>
>> 2) New listen() overflows reuse->socks[] and call reuseport_grow().
>>
>> 3) reuse->max_socks overflows u16 with the new listener.
>>
>> 4) reuseport_grow() pops the old shutdown()ed listener from the array
>>     and update its sk->sk_reuseport_cb as NULL without lock_sock().
>>
>> shutdown()ed sk->sk_reuseport_cb can be changed without lock_sock().
>>
>> But, reuseport_has_conns_set() is called only for UDP and under
>> lock_sock(), so @reuse never be NULL in this case.
> 
> Given the complexity of this code and how much time is needed to
> review all possibilities, please add an additional
> 
> if (reuse)
>     reuse->has_conns = 1;

+1