Re: [PATCH 05/11] sched_ext: Fix TOCTOU race in consume_remote_task()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Infinite livelock loop in scx_consume_dispatch_q() when falling back to the global DSQ.
--

--- Patch [5]: [PATCH 05/11] sched_ext: Fix TOCTOU race in consume_remote_task() ---
commit bf5934bbeaad49172aa99cf40eef02a6a7ea4bd6
Author: Andrea Righi <[email protected]>

sched_ext: Fix TOCTOU race in consume_remote_task()

This commit attempts to fix a TOCTOU race in consume_remote_task() when
pulling a task from a non-local DSQ by deferring the validation until the
task's source rq is locked. It splits the check into lockless and locked
variants, and falls back to enqueueing the task to the global DSQ if the
locked check fails.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 04fc34a7ded65..6699144b36e5e 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -2480,6 +2496,28 @@ static bool consume_remote_task(struct scx_sched *sch, struct rq *this_rq,
>  				struct scx_dispatch_q *dsq, struct rq *src_rq)
>  {
>  	if (unlink_dsq_and_switch_rq_lock(p, dsq, this_rq, src_rq)) {
> +		/*
> +		 * Whether @p may be migrated to @this_rq may have changed while
> +		 * switching rq locks. This is a kernel-side race, not an invalid BPF
> +		 * placement request, so don't abort the scheduler on failure. Fall
> +		 * back to the global DSQ, where normal consumption filters can select
> +		 * a valid destination without forcing the task onto the source local
> +		 * DSQ.
> +		 */
> +		if (unlikely(!task_can_move_from_locked_rq(sch, p, this_rq, false))) {
> +			p->scx.dsq = NULL;
> +			p->scx.holding_cpu = -1;
> +			scx_dispatch_enqueue(sch, src_rq,
> +					     find_global_dsq(sch, task_cpu(p)), p,
> +					     enq_flags | SCX_ENQ_CLEAR_OPSS |
> +					     SCX_ENQ_GDSQ_FALLBACK);
> +			if (sched_class_above(p->sched_class,
> +					      src_rq->donor->sched_class))
> +				resched_curr(src_rq);
> +			switch_rq_lock(src_rq, this_rq);
> +			return false;

[Severity: High]
Can this fallback path cause an infinite livelock if the DSQ being scanned
is already the global DSQ?

When scx_consume_dispatch_q() locklessly picks a task that is executing on a
remote CPU, task_can_move_from_locked_rq() correctly fails.

Because consume_remote_task() enqueues the task back to find_global_dsq()
and returns false, scx_consume_dispatch_q() will react to the false return
value by jumping to its retry label.

If scx_consume_dispatch_q() was scanning the global DSQ:

kernel/sched/ext/ext.c:scx_consume_dispatch_q() {
retry:
    ...
    nldsq_for_each_task(p, dsq) {
        ...
        if (task_can_run_on_remote_rq(sch, p, rq, false)) {
            if (likely(consume_remote_task(sch, rq, p, enq_flags,
                                           dsq, task_rq)))
                return true;
            goto retry;
        }
    }
    ...
}

Does this cause the scanner to jump back to retry, rescan the global DSQ,
find the exact same task again, fail the locked check again, and repeat
the sequence indefinitely while holding runqueue locks?

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