Re: [PATCH v19 01/40] dept: implement DEPT(DEPendency Tracker)

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data race and Use-After-Clear on `dt->stage_m` between `dept_request_event_wait_commit()` and `dept_ttwu_stage_wait()`
- [High] Use-after-free and RCU list corruption because `dept_class` objects are freed before the RCU grace period elapses
- [High] Deadlock on PREEMPT_RT due to `local_irq_disable()` being used with a raw spinlock (`dept_pool_spin`)
--

commit 1fd2695b58a470d54e232dcaaf112ad8a807718e
Author: Byungchul Park <byungchul@xxxxxx>

dept: implement DEPT(DEPendency Tracker)

This commit implements DEPT (Dependency Tracker) to monitor waits and
events for deadlock detection. It addresses Lockdep's limitations
regarding read locks and cross-event scenarios.

> diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> new file mode 100644
> index 0000000000000..52b9669fe0179
> --- /dev/null
> +++ b/kernel/dependency/dept.c
> @@ -0,0 +1,2977 @@

[ ... ]

> +static void dept_wq_work_fn(struct work_struct *work)
> +{
> +	int i;
> +
> +	for (i = 0; i < OBJECT_NR; i++) {
> +		struct dept_pool *p = dept_pool + i;
> +		int sz = p->tot_nr * p->obj_sz;
> +		void *rpool;
> +		bool need;
> +
> +		local_irq_disable();
> +		arch_spin_lock(&dept_pool_spin);

[Severity: High]
Is this locking pattern safe on PREEMPT_RT?

By using local_irq_disable() with a raw spinlock (dept_pool_spin), hardware
interrupts aren't disabled on PREEMPT_RT. Since DEPT instruments hardirq
paths via dept_enter(), could a hardware interrupt fire here, attempt to
acquire the exact same lock in from_pool(), and cause a hard deadlock?

Should raw_local_irq_disable() or arch_local_irq_save() be used instead?

[ ... ]

> +	list_for_each_entry_safe(c, n, &dept_classes, all_node) {
> +		if (!within((void *)c->key, start, sz) &&
> +		    !within(c->name, start, sz))
> +			continue;
> +
> +		hash_del_class(c);
> +		disconnect_class(c);
> +		list_del(&c->all_node);
> +		invalidate_class(c);
> +
> +		/*
> +		 * Actual deletion will happen on the rcu callback
> +		 * that has been added in disconnect_class().
> +		 */
> +		del_class(c);
> +	}
> +	dept_unlock();
> +	dept_exit(flags);
> +
> +	/*
> +	 * Wait until even lockless hash_lookup_class() for the class
> +	 * returns NULL.
> +	 */
> +	might_sleep();
> +	synchronize_rcu();

[Severity: High]
Does this sequence safely handle RCU lifetimes for the dept_class objects?

Although the comment states actual deletion happens in the RCU callback,
disconnect_class() only queues callbacks for dept_dep objects, not the
class itself. By calling del_class(c) directly before synchronize_rcu(),
the object is pushed to the freelist synchronously via to_pool().

If a concurrent lockless RCU reader like hash_lookup_class() traverses
the bucket and reads hash_node.next after the class is reallocated,
wouldn't this result in list corruption or use-after-free crashes?

[ ... ]

> +void dept_request_event_wait_commit(void)
> +{
> +	struct dept_task *dt = dept_task();
> +	unsigned long flags;
> +	unsigned int wg;
> +	unsigned long ip;
> +	const char *w_fn;
> +	bool sched_map;

[ ... ]

> +	wg = next_wgen();
> +	WRITE_ONCE(dt->stage_m.wgen, wg);
> +	arch_spin_unlock(&dt->stage_lock);
> +
> +	__dept_wait(&dt->stage_m, 1UL, ip, w_fn, 0, true, sched_map);

[Severity: High]
Can this sequence lead to a use-after-clear of dt->stage_m?

Once dt->stage_lock is dropped, a concurrent wakeup can execute
dept_ttwu_stage_wait() which acquires the same stage_lock and calls
__dept_clean_stage(), clearing the original stage_m using memset().

Since __dept_wait() is called here with a reference to &dt->stage_m
after the lock is released, could it read memory that is actively being
zeroed out, resulting in a NULL pointer dereference for fields like
keys or name in check_new_class()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@xxxxxx?part=1