Re: [PATCH v2 sched_ext/for-7.3 21/36] sched_ext: Maintain per-cpu effective cap copies for single-read checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Asynchronous processing of `ecaps_to_sync` races with a new scheduler's `scx_cid_init()`, causing slab out-of-bounds reads in `scx_process_sync_ecaps()`.
- [High] `queue_sync_ecaps()` causes a NULL pointer dereference by passing the global `scx_root` pointer to `scx_kick_cpu()` during scheduler teardown.
--

commit d27a6ceed9961e91cdb8abb928e2d28ddbbb826a
Author: Tejun Heo <tj@kernel.org>

sched_ext: Maintain per-cpu effective cap copies for single-read checks

This patch adds per-CPU effective capability copies (ecaps) to avoid testing
multiple bits or encountering race conditions with caps[]. Grant and revoke
operations queue a sync request for the target CPU, which is processed under
the runqueue lock.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c

[ ... ]

> @@ -287,6 +287,125 @@ static void scx_sub_seed_caps(struct scx_sched *sch)
>  	caps_updated_deliver(&to_deliver);
>  }
>  

[ ... ]

> +static void queue_sync_ecaps(struct scx_sched *sch, s32 cid)
> +{
> +	s32 cpu = __scx_cid_to_cpu(cid);
> +	struct scx_sched_pcpu *pcpu = per_cpu_ptr(sch->pcpu, cpu);
> +
> +	/*
> +	 * Pairs with smp_mb() in scx_process_sync_ecaps(). Either the check
> +	 * below sees the node off the list and queues it, or the in-flight sync
> +	 * sees the caps[] update made before this call.
> +	 */
> +	smp_mb();
> +
> +	/* @cid's pshard->lock excludes concurrent queueing attempts */
> +	if (llist_on_list(&pcpu->ecaps_to_sync_node))
> +		return;
> +	if (llist_add(&pcpu->ecaps_to_sync_node, &cpu_rq(cpu)->scx.ecaps_to_sync))
> +		scx_kick_cpu(scx_root, cpu, 0);

[Severity: High]
During scheduler teardown, scx_ops_disable() clears the global scx_root
pointer via RCU_INIT_POINTER(scx_root, NULL). If an active BPF program
triggers queue_sync_ecaps() while scx_root is NULL, could passing scx_root
here cause a NULL pointer dereference in scx_kick_cpu()?

Since queue_sync_ecaps() already receives a valid struct scx_sched *sch
parameter, would it be safer to pass sch to scx_kick_cpu() instead of
scx_root?

> +}
> +
> +/* discard @rq's queued ecaps syncs */

[ ... ]

> +/**
> + * scx_process_sync_ecaps - Sync this cpu's ecaps to pshard->caps[]
> + * @rq: the cid's cpu rq
> + *
> + * pshard->caps[] is the target configuration. pcpu->ecaps is the effective
> + * transposed copy owned by the cid's cpu and written only here under @rq's
> + * lock.
> + */
> +void scx_process_sync_ecaps(struct rq *rq)
> +{
> +	s32 cid = __scx_cpu_to_cid(cpu_of(rq));
> +	s32 shard = scx_cid_to_shard[cid];

[Severity: High]
When a new root scheduler is enabled, scx_cid_init() temporarily resets
scx_cpu_to_cid_tbl[cpu] = -1 for all CPUs. If scx_process_sync_ecaps() is
called concurrently (e.g., from an active scheduler's balance_one() or a
dying scheduler's scx_sched_free_rcu_work()), could cid be evaluated as -1?

If cid is -1, evaluating scx_cid_to_shard[cid] would result in an
out-of-bounds read. Even if we move the llist_empty() check above these
lookups, a concurrent BPF execution calling scx_bpf_sub_grant() could queue
a new sync, allowing execution to reach the lookups with cid still being -1
and resulting in garbage values being used to index ps->caps.

Can we defer the cid and shard evaluations until after we are sure we have
work to process, or add bounds checking for cid?

> +	struct llist_node *batch, *pos, *tmp;
> +
> +	lockdep_assert_rq_held(rq);
> +
> +	if (likely(llist_empty(&rq->scx.ecaps_to_sync)))
> +		return;

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