Re: [PATCH sched_ext/for-7.3 19/32] sched_ext: Add sub_ecaps_updated() effective-cap change notifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Concurrent freeing of a sub-scheduler can permanently discard ecaps syncs for all active sub-schedulers on a CPU that is coming online.
- [High] scx_discard_ecaps_to_sync() can hang indefinitely in a worker thread if the target CPU is continuously executing higher-priority tasks, locking up the system RCU workqueue.
- [High] Early clearing of `ecaps_to_sync_node` in `scx_process_sync_ecaps()` causes a Use-After-Free of `pcpu` and `sch` during sub-scheduler teardown.
--

commit f00aae3acf0710a9423504a897d70bfb5d9571f6
Author: Tejun Heo <tj@kernel.org>
Subject: sched_ext: Add sub_ecaps_updated() effective-cap change notifier

The patch adds an ops.sub_ecaps_updated() callback fired when a
sub-scheduler's effective caps on a cid change. CPU hotplug uses this
mechanism to zero caps when a CPU goes down, and to resync when coming
back online.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index 08d9367cf2188..55437f1d1965f 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c

[ ... ]

> @@ -351,33 +354,130 @@ void scx_process_sync_ecaps(struct rq *rq)
>  	if (likely(llist_empty(&rq->scx.ecaps_to_sync)))
>  		return;
>  
> +	/*
> +	 * ecaps are zeroed while the cpu is inactive and must stay zero.
> +	 * Discard queued syncs instead of processing them - the
> +	 * scx_online_ecaps() reseed re-syncs every sched on activation.
> +	 * cpu_active() clears before the offline zeroing and sets before the
> +	 * reseed is queued, so this test can neither miss a racing sync nor
> +	 * eat the reseed.
> +	 */
> +	if (unlikely(!cpu_active(cpu))) {
> +		discard_queued_syncs(rq);
> +		return;
> +	}
> +
>  	batch = llist_del_all(&rq->scx.ecaps_to_sync);
>  	llist_for_each_safe(pos, tmp, batch) {
>  		struct scx_sched_pcpu *pcpu =
>  			container_of(pos, struct scx_sched_pcpu, ecaps_to_sync_node);
>  		struct scx_pshard *ps = pcpu->sch->pshard[shard];
> +		u64 ecaps;
>  
>  		init_llist_node(pos);
>  
>  		/* pairs with smp_mb() in queue_sync_ecaps(), see there */
>  		smp_mb();
>  
> -		WRITE_ONCE(pcpu->ecaps, calc_effective_caps(ps, cid));
> +		ecaps = calc_effective_caps(ps, cid);
> +		WRITE_ONCE(pcpu->ecaps, ecaps);
> +
> +		/* tell the sched its effective caps on this cid changed */
> +		if (ecaps != pcpu->reported_ecaps &&
> +		    SCX_HAS_OP(pcpu->sch, sub_ecaps_updated) &&
> +		    !scx_bypassing(pcpu->sch, cpu)) {
> +			struct scx_dsp_ctx *dspc = &pcpu->dsp_ctx;
> +
> +			dspc->rq = rq;
> +			/* stash @prev so nested dispatches can access it */
> +			rq->scx.sub_dispatch_prev = prev;
> +			SCX_CALL_OP(pcpu->sch, sub_ecaps_updated, rq, scx_cpu_arg(cpu),
> +				    pcpu->reported_ecaps, ecaps);
> +			rq->scx.sub_dispatch_prev = NULL;
> +			scx_flush_dispatch_buf(pcpu->sch, rq);
> +			pcpu->reported_ecaps = ecaps;

[Severity: High]
Does this sequence create a use-after-free of pcpu?

In scx_process_sync_ecaps(), the pcpu node is unlinked via init_llist_node(pos).
Later, scx_flush_dispatch_buf() might drop the runqueue lock if it dispatches
to a remote CPU's local DSQ (e.g., via dispatch_to_local_dsq()).

If the lock is dropped, scx_discard_ecaps_to_sync() executing in the RCU worker
can acquire the lock, observe !llist_on_list(), and return, allowing the
pcpu structure to be freed.

When scx_flush_dispatch_buf() finishes and reacquires the lock,
scx_process_sync_ecaps() then writes to the freed pcpu:

    pcpu->reported_ecaps = ecaps;

> +		}
> +	}
> +}

[ ... ]

>  void scx_discard_ecaps_to_sync(s32 cpu, struct scx_sched_pcpu *pcpu)
>  {
> -	scoped_guard (rq_lock_irqsave, cpu_rq(cpu))
> -		scx_process_sync_ecaps(cpu_rq(cpu));
> +	struct rq *rq = cpu_rq(cpu);
>  
> -	WARN_ON_ONCE(llist_on_list(&pcpu->ecaps_to_sync_node));
> +	while (true) {
> +		scoped_guard (rq_lock_irqsave, rq) {
> +			/*
> +			 * scx_process_sync_ecaps() takes the node off the list
> +			 * before it is done accessing @pcpu but does all of it
> +			 * under the rq lock. Off-list observed under the rq
> +			 * lock guarantees that the sync is complete.
> +			 */
> +			if (!llist_on_list(&pcpu->ecaps_to_sync_node))
> +				return;
> +			if (!scx_enabled() || !scx_rq_online(rq)) {

[Severity: High]
Could this destructively drop valid queued syncs for active sub-schedulers?

If a sub-scheduler is being freed via scx_sched_free_rcu_work() concurrently
with CPU hotplug (scx_online_ecaps()), scx_rq_online() checks SCX_RQ_ONLINE.
This flag is set by sched_set_rq_online() after the CPU comes online.

During the window where !scx_rq_online(rq) is true, discard_queued_syncs()
deletes all queued syncs for all sub-schedulers via llist_del_all().

Checking !cpu_active(cpu) instead (as in scx_process_sync_ecaps) might avoid
this race.

> +				discard_queued_syncs(rq);
> +				return;
> +			}
> +		}
> +		resched_cpu(cpu);
> +		msleep(1);

[Severity: High]
Can this loop hang indefinitely if the target CPU is saturated with higher
priority tasks?

The loop spins waiting for the target CPU to process the pending ecaps sync.
The node is removed when scx_process_sync_ecaps() runs on the target CPU,
which is called from balance_one().

Because ext_sched_class has a lower priority than FAIR or RT classes,
if the target CPU is continuously executing higher priority tasks,
balance_one() is never called. The RCU worker thread will spin forever
waiting for !llist_on_list().

> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703080159.2314350-1-tj@kernel.org?part=19