Re: [PATCH v4 sched_ext/for-7.3 23/40] sched_ext: Maintain per-cpu effective cap copies for single-read checks

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional evaluation of `__scx_cpu_to_cid(cpu_of(rq))` and `scx_cid_to_shard[cid]` in `scx_process_sync_ecaps()` causes a kernel crash (NULL pointer dereference) during initialization failures.
- [High] Naked and unchecked use of the global `scx_root` pointer in `queue_sync_ecaps()` causes a NULL pointer dereference if the root scheduler is concurrently disabled.
--

--- Patch [23]: [PATCH v4 sched_ext/for-7.3 23/40] sched_ext: Maintain per-cpu effective cap copies for single-read checks ---
commit 0633bda49e9d687035b71703d07c991f9fd41b6c
Author: Tejun Heo <[email protected]>

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

This commit introduces per-cpu effective capability copies to optimize
capability checking in the scheduler. It collects the capabilities per cpu
so they can be accessed locklessly or under the runqueue lock. Grant and
revoke operations now queue sync requests which are processed by the target
cpu.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index ce2fa1409a205..80f74064ca781 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c
[ ... ]
> +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]
Is it safe to pass scx_root directly here without checking for NULL?

If the root scheduler is concurrently being disabled, scx_root might be reset
to NULL. Because BPF kfuncs like scx_bpf_sub_grant() can still be executing
in sleepable or tracing contexts during unregistration, could this lead to a
NULL pointer dereference inside scx_kick_cpu()? 

Would using the local sch parameter instead resolve this safely?

> +}
[ ... ]
> +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 these array indexing operations cause a kernel crash during early
setup failures?

If the BPF scheduler enable path fails before scx_cid_init() completes
(e.g., due to memory allocation failure), the global pointers like
scx_cpu_to_cid_tbl and scx_cid_to_shard remain NULL.

The error recovery path invokes scx_discard_ecaps_to_sync(), which calls
scx_process_sync_ecaps(). Because the cid evaluation and scx_cid_to_shard
dereference happen unconditionally before the llist_empty() check below,
this looks like it will cause a NULL pointer dereference.

Would moving the cid and shard evaluations after the llist_empty() check
avoid this?

> +	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);
> +	llist_for_each_safe(pos, tmp, batch) {
[ ... ]

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