Re: [PATCH] lockdep: Speed up lockdep_unregister_key() with expedited RCU synchronization
From: Waiman Long
Date: Mon Mar 24 2025 - 21:57:21 EST
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