Re: [PATCH v2] sched_ext: Don't BUG_ON a destroyed DSQ in process_deferred_reenq_users

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Lockless double-read of `dsq->id` creates a Time-Of-Check to Time-Of-Use (TOCTOU) race that can still falsely trigger the `BUG_ON`, crashing the kernel.

Pre-existing issues:
- [Critical] Dereferencing `dsq->id` in `process_deferred_reenq_users()` causes a Use-After-Free because an RCU grace period can complete while the target CPU is idle, freeing `dsq` before it is accessed.
--

--- Patch [1]: [PATCH v2] sched_ext: Don't BUG_ON a destroyed DSQ in process_deferred_reenq_users ---
commit f1934b9161ead6ceeb2cb6d00c64a01e1a233104
Author: Tao Cui <[email protected]>

sched_ext: Don't BUG_ON a destroyed DSQ in process_deferred_reenq_users

This commit attempts to fix a BUG_ON triggered when process_deferred_reenq_users()
encounters a destroyed DSQ. It adds a check to skip processing if the DSQ ID 
has been set to SCX_DSQ_INVALID.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 18183062f751c..1a059e5f09e80 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -4332,6 +4332,10 @@ static void process_deferred_reenq_users(struct rq *rq)
>  		/* see schedule_dsq_reenq() */
>  		smp_mb();
>  
> +		/* destroy_dsq() may have raced and invalidated @dsq, nothing to reenq */

[Severity: Critical]
This is a pre-existing issue, but does dereferencing dsq->id here outside of
the rq->scx.deferred_reenq_lock critical section expose the pointer to a
concurrent kfree() in the RCU teardown path?

If CPU B is idle (in an RCU Extended Quiescent State), and CPU A queues a
deferred reenq for a DSQ to CPU B and calls destroy_dsq(), the RCU grace
period can end immediately without waiting for CPU B. The RCU callback
free_dsq_rcufn then runs on CPU C and calls exit_dsq().

Concurrently, if CPU B wakes up, enters process_deferred_reenq_users(),
removes dru from the list, and drops the lock:

CPU B:
scoped_guard (raw_spinlock, &rq->scx.deferred_reenq_lock) {
	...
	list_del_init(&dru->node);
}
// CPU B is now at the dsq->id check

CPU C's exit_dsq() then evaluates the list without locks:

CPU C:
exit_dsq() {
	...
	if (WARN_ON_ONCE(!list_empty(&dru->node))) {
		// Skips lock because list is empty
	}
	...
	kfree(dsq);
}

Would CPU B then read from the freed dsq memory when checking dsq->id?

> +		if (unlikely(dsq->id == SCX_DSQ_INVALID))
> +			continue;
> +
>  		BUG_ON(dsq->id & SCX_DSQ_FLAG_BUILTIN);

[Severity: High]
Because dsq->id is accessed twice consecutively without READ_ONCE(), can
the compiler emit two separate memory loads?

If a concurrent destroy_dsq() sets dsq->id = SCX_DSQ_INVALID exactly
between the two loads, the first load would see a valid user DSQ ID and
bypass the continue. The second load would then observe SCX_DSQ_INVALID.

Since SCX_DSQ_INVALID evaluates to true for SCX_DSQ_FLAG_BUILTIN,
wouldn't this still trigger the BUG_ON?

>  		reenq_user(rq, dsq, reenq_flags);
>  	}

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