Re: [PATCH v3 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 1 potential issue(s) to consider:
- [High] Unconditional dereference of global CID routing tables in `scx_process_sync_ecaps()` leads to NULL pointer dereference on error paths and out-of-bounds reads during layout updates.
--

--- Patch [21]: [PATCH v3 sched_ext/for-7.3 21/36] sched_ext: Maintain per-cpu effective cap copies for single-read checks ---
commit c2d60606de278d78c865d3844ddc3d528eebcab1
Author: Tejun Heo <tj@kernel.org>

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

This commit introduces a per-cpu effective caps copy for a scheduler to
avoid awkward race conditions when checking multiple cap bits. The new
effective copy, ecaps, is updated safely under the rq lock and can be read
locklessly.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index a1c061b6e7be4..4e96234379bb0 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c

[ ... ]

> +/**
> + * 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]
Can this result in a NULL pointer dereference or an out-of-bounds read?

If loading the first SCX scheduler fails due to memory constraints in
scx_cid_arrays_alloc(), scx_cpu_to_cid_tbl remains NULL. The cleanup path
scx_sched_free_rcu_work() unconditionally invokes scx_process_sync_ecaps()
via scx_discard_ecaps_to_sync(), which would dereference the NULL table here.

Additionally, when rapidly replacing an active scheduler, the new scheduler's
scx_cid_init() temporarily resets scx_cpu_to_cid_tbl entries to -1:

	/* -1 sentinels for sparse-possible cpu id holes (0 is a valid cid) */
	for (cpu = 0; cpu < nr_cpu_ids; cpu++)
		scx_cpu_to_cid_tbl[cpu] = -1;

If the old scheduler's asynchronous RCU cleanup work invokes
scx_process_sync_ecaps() concurrently, it reads the -1 value and uses it to
index scx_cid_to_shard[-1], resulting in an out-of-bounds read.

Would it be safer to move the cid and shard evaluation after the
llist_empty() check below?

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

[ ... ]

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