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

[Waiman Long <llong@redhat.com>]

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