Re: [PATCH v2 sched_ext/for-7.3 33/36] sched_ext: Replay ecaps notifications suppressed by bypass

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Sub-schedulers without an arena map will cause NULL pointer dereferences on CPU hotplug and capability grants because `sch->pshard` is never allocated.
--

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

sched_ext: Replay ecaps notifications suppressed by bypass

This commit queues an ecaps sync when a CPU exits bypass mode. This ensures
that initial capabilities granted during the enable bypass window are correctly
delivered to the BPF scheduler.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index 5a48304b72a67..9e0643e3cecba 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c
[ ... ]
> @@ -562,6 +562,41 @@ void scx_process_sync_ecaps(struct rq *rq, struct task_struct *prev)
>  		scx_schedule_reenq_local(rq, SCX_REENQ_CAP_REVOKE);
>  }
>  
> +/**
> + * scx_unbypass_replay_ecaps - Replay a bypass-suppressed ecaps notification
> + * @rq: rq of the cpu leaving bypass
> + * @sch: scheduler that just left bypass on @rq's cpu
> + *
> + * scx_process_sync_ecaps() consumes syncs while bypassing without delivering
> + * ops.sub_ecaps_updated(), leaving reported_ecaps stale. Nothing re-queues a
> + * sync when bypass lifts, so without a replay a cid that never changes again
> + * would never be notified. The attach-time initial grants are the acute case
> + * as they are consumed during the enable bypass window. Re-queue a sync for
> + * any undelivered delta so the next balance delivers it.
> + */
> +void scx_unbypass_replay_ecaps(struct rq *rq, struct scx_sched *sch)
> +{
> +	s32 cpu = cpu_of(rq);
> +	struct scx_sched_pcpu *pcpu = per_cpu_ptr(sch->pcpu, cpu);
> +	struct scx_pshard *ps;
> +	s32 cid;
> +
> +	lockdep_assert_rq_held(rq);
> +
> +	/* root holds every cap and never uses ecaps */
> +	if (!sch->level)
> +		return;
> +
> +	if (READ_ONCE(pcpu->ecaps) == pcpu->reported_ecaps)
> +		return;
> +
> +	cid = __scx_cpu_to_cid(cpu);
> +	ps = sch->pshard[scx_cid_to_shard[cid]];
> +
> +	guard(raw_spinlock)(&ps->lock);

[Severity: High]
This is a pre-existing issue, but can this dereference a NULL pointer if the
scheduler was loaded without an arena map?

If a BPF scheduler using bpf_sched_ext_ops_cid is loaded without defining an
arena map, scx_arena_pool_init() returns 0 without allocating a pool. 

Then in scx_alloc_pshards(), it silently succeeds without allocating pshard:

kernel/sched/ext/sub.c:scx_alloc_pshards() {
    if (!sch->is_cid_type || !sch->arena_pool)
        return 0;
    ...
}

Since pshard is never allocated, unconditional dereferences of sch->pshard
like the one here, or during CPU hotplug in scx_online_ecaps(), or during
capability grants in scx_bpf_sub_grant(), will cause a kernel panic.

Should scx_alloc_pshards() return an error when the arena pool is missing,
or do these paths need NULL checks?

> +	queue_sync_ecaps(sch, cid);
> +}

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