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

[sashiko-bot@kernel.org]

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 <byungchul@sk.com>

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/20260706061928.66713-1-byungchul@sk.com?part=4