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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Lost wakeup / scheduler stall when a task is skipped during DSQ consumption.
- [High] List corruption and state machine violation due to enqueuing an actively executing task to the global DSQ.
--

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

sched_ext: Fix TOCTOU race in consume_remote_task()

This commit addresses a TOCTOU race when migrating a task during DSQ
consumption. It re-evaluates task_can_run_on_remote_rq() after locking the
source runqueue and falls back to enqueuing the task on the global DSQ if
the task is no longer eligible to migrate.

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

[ ... ]

> @@ -2382,6 +2384,10 @@ static bool task_can_run_on_remote_rq(struct scx_sched *sch,
>  
>  	WARN_ON_ONCE(task_cpu(p) == cpu);
>  
> +	/* Don't migrate a task which is running on a CPU. */
> +	if (task_on_cpu(task_rq(p), p))
> +		return false;
> +

[Severity: High]
Could returning false here cause a lost wakeup or scheduler stall?

When a task is preempted and enqueued to a non-local DSQ, an idle CPU might
attempt to consume it from scx_consume_dispatch_q() before the previous CPU
completes finish_task_switch() (which clears p->on_cpu).

Since scx_consume_dispatch_q() evaluates task_can_run_on_remote_rq()
locklessly, it would return false, causing the consuming CPU to skip the
task and potentially go back to sleep. Once the previous CPU finally clears
p->on_cpu, is there a mechanism to generate a new kick, or will the task
remain stranded on the dispatch queue indefinitely?

>  	/*
>  	 * If @p has migration disabled, @p->cpus_ptr is updated to contain only
>  	 * the pinned CPU in migrate_disable_switch() while @p is being switched

[ ... ]

> @@ -2480,6 +2486,27 @@ 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)) {
> +		/*
> +		 * Eligibility 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 an eligible CPU without
> +		 * forcing the task onto the source local DSQ.
> +		 */
> +		if (unlikely(!task_can_run_on_remote_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);

[Severity: High]
Can this fallback path lead to list corruption if the task is actively
executing?

If a donor task in a DSQ becomes active (task_on_cpu() is true) during proxy
execution, the re-evaluation of task_can_run_on_remote_rq() correctly detects
it as executing and returns false.

However, this fallback path will then clear p->scx.dsq and unconditionally
enqueue the actively executing task to the global DSQ. When the task is later
preempted, put_prev_task_scx() will attempt to enqueue it again, which seems
like it would trigger the WARN_ON_ONCE(p->scx.dsq || !list_empty(...)) and
corrupt the DSQ list.

> +			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;
> +		}
> +
>  		move_remote_task_to_local_dsq(sch, p, enq_flags, src_rq, this_rq);
>  		return true;
>  	} else {

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