Re: [PATCH v2 net] udp: Update reuse->has_conns under reuseport_lock.

[Martin KaFai Lau <martin.lau@linux.dev>]

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