Re: [Patch net] mptcp: initialize sock lock with its own lockdep keys

[Matthieu Baerts <matttbe@kernel.org>]

Hi Cong Wang,

On 11/09/2024 05:34, Cong Wang wrote:
> On Mon, Sep 09, 2024 at 05:03:32PM +0200, Matthieu Baerts wrote:
>> Hi Cong Wang,
>>
>> On 08/09/2024 20:06, Cong Wang wrote:
>>> From: Cong Wang <cong.wang@bytedance.com>
>>>
>>> In mptcp_pm_nl_create_listen_socket(), we already initialize mptcp sock
>>> lock with mptcp_slock_keys and mptcp_keys. But that is not sufficient,
>>> at least mptcp_init_sock() and mptcp_sk_clone_init() still miss it.
>>>
>>> As reported by syzbot, mptcp_sk_clone_init() is challenging due to that
>>> sk_clone_lock() immediately locks the new sock after preliminary
>>> initialization. To fix that, introduce ->init_clone() for struct proto
>>> and call it right after the sock_lock_init(), so now mptcp sock could
>>> initialize the sock lock again with its own lockdep keys.
>>
>> Thank you for this patch!
>>
>> The fix looks good to me, but I need to double-check if we can avoid
>> modifying the proto structure. Here is a first review.
>>
>>
>> From what I understand, it looks like syzbot reported a lockdep false
>> positive issue, right? In this case, can you clearly mention that in the
>> commit message, to avoid misinterpretations?
>>
>>> Reported-by: syzbot+f4aacdfef2c6a6529c3e@syzkaller.appspotmail.com
>>
>> checkpatch.pl reports that "Reported-by: should be immediately followed
>> by Closes: with a URL to the report".
> 
> Sure, didn't know this is helpful.

It is useful for the reviewers/devs to find more info about the issue,
and for other bots to mark a bug report as closed.

>> Also, even if it is a false positive, it sounds better to consider this
>> as a fix, to avoid having new bug reports about that. In this case, can
>> you please add a "Fixes: <commit>" tag and a "Cc: stable" tag here please?
> 
> I intended not to provide one because I don't think this needs to go to
> -stable, it only fixes a lockdep warning instead of a real deadlock.
> Please let me know if you prefer to target -stable.

Yes, it is useful. Because if it is not backported, it is likely we will
get this bug report again with stable versions. And such bug reports are
always taking time to analyse.

(...)

>>> diff --git a/net/core/sock.c b/net/core/sock.c
>>> index 9abc4fe25953..747d7e479d69 100644
>>> --- a/net/core/sock.c
>>> +++ b/net/core/sock.c
>>> @@ -2325,6 +2325,8 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
>>>  	}
>>>  	sk_node_init(&newsk->sk_node);
>>>  	sock_lock_init(newsk);
>>> +	if (prot->init_clone)
>>> +		prot->init_clone(newsk);
>>
>> If the idea is to introduce a new ->init_clone(), should it not be
>> called ->lock_init() (or ->init_lock()) and replace the call to
>> sock_lock_init() when defined?
> 
> 'lock_init' or 'init_lock' reads like we are initalizing a lock. :)

If it is replacing sock_lock_init() call when ->init_lock is defined, it
will be about initializing a lock ;)

Thank you for the v2 (in MPTCP ML), I will continue the reviews there.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.