Re: [PATCH v19 04/40] dept: tie to lockdep and IRQ tracing

On Tue, Jul 07, 2026 at 07:33:22AM +0000, [email protected] wrote:
> 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?

I will check it.  Thanks.

> [ ... ]
> > -#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()?

It's intended.  rwsem is a queued lock for fairness.  So it should be
considered as a normal lock to detect a potential deadlock.

> 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?

As I told you, rwsem is a queued lock.  So this is not true.

> > 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?

I will check it.

> [ ... ]
> > @@ -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?

I will check 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?

Sure.  I will.

> [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.

No.

del_class() never drops refcount to 0.  That happens only through:

   disconnect_class()->call_rcu(del_dep_rcu)

   // after the grace period

   del_dep_rcu()->del_dep()->put_ecxt()->put_class()
   del_dep_rcu()->del_dep()->put_wait()->put_class()

So it's safe.

> 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?

No.

	Byungchul
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4