Re: [PATCH] lockdep: Speed up lockdep_unregister_key() with expedited RCU synchronization

Waiman Long
2025-03-25T01:56:31Z

On 3/24/25 8:47 PM, Boqun Feng wrote:
> On Mon, Mar 24, 2025 at 12:30:10PM -0700, Boqun Feng wrote:
>> On Mon, Mar 24, 2025 at 12:21:07PM -0700, Boqun Feng wrote:
>>> On Mon, Mar 24, 2025 at 01:23:50PM +0100, Eric Dumazet wrote:
>>> [...]
>>>>>> ---
>>>>>> kernel/locking/lockdep.c | 6 ++++--
>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
>>>>>> index 4470680f02269..a79030ac36dd4 100644
>>>>>> --- a/kernel/locking/lockdep.c
>>>>>> +++ b/kernel/locking/lockdep.c
>>>>>> @@ -6595,8 +6595,10 @@ void lockdep_unregister_key(struct lock_class_key *key)
>>>>>> if (need_callback)
>>>>>> call_rcu(&delayed_free.rcu_head, free_zapped_rcu);
>>>>>>
>>>>>> - /* Wait until is_dynamic_key() has finished accessing k->hash_entry. */
>>>>>> - synchronize_rcu();
>>> I feel a bit confusing even for the old comment, normally I would expect
>>> the caller of lockdep_unregister_key() should guarantee the key has been
>>> unpublished, in other words, there is no way a lockdep_unregister_key()
>>> could race with a register_lock_class()/lockdep_init_map_type(). The
>>> synchronize_rcu() is not needed then.
>>>
>>> Let's say someone breaks my assumption above, then when doing a
>>> register_lock_class() with a key about to be unregister, I cannot see
>>> anything stops the following:
>>>
>>> CPU 0 CPU 1
>>> ===== =====
>>> register_lock_class():
>>> ...
>>> } else if (... && !is_dynamic_key(lock->key)) {
>>> // ->key is not unregistered yet, so this branch is not
>>> // taken.
>>> return NULL;
>>> }
>>> lockdep_unregister_key(..);
>>> // key unregister, can be free
>>> // any time.
>>> key = lock->key->subkeys + subclass; // BOOM! UAF.
>>>
>>> So either we don't need the synchronize_rcu() here or the
>>> synchronize_rcu() doesn't help at all. Am I missing something subtle
>>> here?
>>>
>> Oh! Maybe I was missing register_lock_class() must be called with irq
>> disabled, which is also an RCU read-side critical section.
>>
> Since register_lock_class() will be call with irq disabled, maybe hazard
> pointers [1] is better because most of the case we only have nr_cpus
> readers, so the potential hazard pointer slots are fixed.
>
> So the below patch can reduce the time of the tc command from real ~1.7
> second (v6.14) to real ~0.05 second (v6.14 + patch) in my test env,
> which is not surprising given it's a dedicated hazard pointers for
> lock_class_key.
>
> Thoughts?

My understanding is that it is not a race between register_lock_class()
and lockdep_unregister_key(). It is the fact that the structure that
holds the lock_class_key may be freed immediately after return from
lockdep_unregister_key(). So any processes that are in the process of
iterating the hash_list containing the hash_entry to be unregistered may
hit a UAF problem. See commit 61cc4534b6550 ("locking/lockdep: Avoid
potential access of invalid memory in lock_class") for a discussion of
this kind of UAF problem.

As suggested by Eric, one possible solution is to add a
lockdep_unregister_key() variant function that presumes the structure
holding the key won't be freed until after a RCU delay. In this case, we
can skip the last synchronize_rcu() call. Any callers that need
immediate return should use kfree_rcu() to free the structure after
calling the lockdep_unregister_key() variant.

Cheers,
Longman