Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] dept_key_destroy() synchronously recycles dept_class objects to the lockless pool before the RCU grace period completes, causing UAF and list traversal corruption for concurrent lockless readers. - [High] rwsem_acquire_read and rwsem_acquire incorrectly use ldt_lock(), breaking reader-writer dependency tracking and causing false-positive deadlocks. - [High] ldt_init() is called before the `key` parameter is validated, causing DEPT to dereference a garbage pointer if `key` is NULL. - [Medium] dept_key_init() and dept_key_destroy() are called on static keys before validation, erroneously disabling DEPT or corrupting tracking state. - [Low] STATIC_LOCKDEP_MAP_INIT creates double-quoted strings for map names due to `#n` stringification in DEPT_MAP_INITIALIZER. --
commit 6c1a95ee3182a2a82a1d61196d75acd387d7f3e3 Author: Byungchul Park <[email protected]> dept: tie to lockdep and IRQ tracing This patch integrates DEPT (Dependency Tracker) into the lockdep framework and IRQ tracing paths. It modifies various locking primitives and lockdep interfaces to initialize and track DEPT maps alongside lockdep maps. > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h > index 621566345406d..60b17a2ff555f 100644 > --- a/include/linux/lockdep.h > +++ b/include/linux/lockdep.h [ ... ] > @@ -428,7 +431,8 @@ enum xhlock_context_t { > * Note that _name must not be NULL. > */ > #define STATIC_LOCKDEP_MAP_INIT(_name, _key) \ > - { .name = (_name), .key = (void *)(_key), } > + { .name = (_name), .key = (void *)(_key), \ > + .dmap = DEPT_MAP_INITIALIZER(_name, _key) } [Severity: Low] Since _name is usually passed as a string literal to STATIC_LOCKDEP_MAP_INIT, does passing it to DEPT_MAP_INITIALIZER, which uses the stringification operator on its first argument, result in double-quoted strings for map names in DEPT's debug output? [ ... ] > -#define rwsem_acquire(l, s, t, i) lock_acquire_exclusive(l, s, t, > NULL, i) > -#define rwsem_acquire_nest(l, s, t, n, i) lock_acquire_exclusive(l, s, t, > n, i) > -#define rwsem_acquire_read(l, s, t, i) lock_acquire_shared(l, > s, t, NULL, i) > -#define rwsem_release(l, i) lock_release(l, i) [ ... ] > +#define rwsem_acquire(l, s, t, i) \ > +do { \ > + ldt_lock(&(l)->dmap, s, t, NULL, i); \ > + lock_acquire_exclusive(l, s, t, NULL, i); \ > +} while (0) > +#define rwsem_acquire_nest(l, s, t, n, i) \ > +do { \ > + ldt_lock(&(l)->dmap, s, t, n, i); \ > + lock_acquire_exclusive(l, s, t, n, i); \ > +} while (0) > +#define rwsem_acquire_read(l, s, t, i) > \ > +do { \ > + ldt_lock(&(l)->dmap, s, t, NULL, i); \ > + lock_acquire_shared(l, s, t, NULL, i); \ > +} while (0) [Severity: High] Is it intentional that rwsem_acquire_read() uses ldt_lock() instead of ldt_rlock(), and rwsem_acquire() uses ldt_lock() instead of ldt_wlock()? This appears to mistakenly model a shared lock as an exclusive lock. Does this cause DEPT to incorrectly model concurrent read locks as mutually exclusive, leading to false-positive circular dependency (deadlock) reports when concurrent readers take an rwsem and interact with other locks? > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > index 2d4c5bab5af88..dc97f2753ef81 100644 > --- a/kernel/locking/lockdep.c > +++ b/kernel/locking/lockdep.c > @@ -1224,6 +1224,8 @@ void lockdep_register_key(struct lock_class_key *key) > struct hlist_head *hash_head; > struct lock_class_key *k; > unsigned long flags; > > + dept_key_init(&key->dkey); > + > if (WARN_ON_ONCE(static_obj(key))) > return; [Severity: Medium] In lockdep_register_key(), dept_key_init() is called before the static_obj() check. If a static key is improperly passed, lockdep gracefully rejects it by warning and returning early. However, processing a static key in dept_key_init() triggers a DEPT_STOP abort. Should the DEPT initialization occur after validating the key? [ ... ] > @@ -4940,6 +4950,8 @@ void lockdep_init_map_type(struct lockdep_map *lock, > const char *name, > { > int i; > > + ldt_init(&lock->dmap, &key->dkey, subclass, name); > + > for (i = 0; i < NR_LOCKDEP_CACHING_CLASSES; i++) > lock->class_cache[i] = NULL; [Severity: High] In lockdep_init_map_type(), ldt_init() is called before the parameter validation checks for a NULL key further down in the function. If key is NULL, &key->dkey yields a non-NULL offset pointer. Does DEPT accept this garbage pointer in valid_key(), causing a kernel panic when check_new_class() dereferences it? [ ... ] > @@ -6588,6 +6608,8 @@ void lockdep_unregister_key(struct lock_class_key *key) > bool found = false; > bool need_callback = false; > > + dept_key_destroy(&key->dkey); > + > might_sleep(); > > if (WARN_ON_ONCE(static_obj(key))) > return; [Severity: Medium] Similarly to lockdep_register_key(), dept_key_destroy() is called here unconditionally before the static_obj() check. If a driver incorrectly calls dynamic lock destruction on a static key, it eagerly destroys its DEPT state while lockdep retains it. Should this also be moved after the static_obj() check? [Severity: Critical] Additionally, in dept_key_destroy() called here, the function removes a class from the RCU hash table and drops its refcount to 0 via del_class(). This recycles the object to the DEPT pool before synchronize_rcu() is called. Can a concurrent lockless reader in hash_lookup_class() traversing the same bucket via hlist_for_each_entry_rcu() access this recycled object, leading to a use-after-free and hash list corruption? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
