Re: [PATCH 3/3] sched_ext: Guard the cid kfuncs against unallocated cid tables

[sashiko-bot@kernel.org]

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

New issues:
- [Low] `__scx_cid_to_cpu()` is dead code after this patch.

Pre-existing issues:
- [High] Deterministic NULL pointer dereference in `__scx_cpu_to_cid()` during the scheduler error disable path if CID tables fail to allocate. This problem wasn't introduced by this patch, but the commit message explicitly and incorrectly claims the tables are guaranteed to be allocated on the enable path.
- [High] Information leak and data races due to early publication and non-atomic writes to CID tables. This problem wasn't introduced by this patch, but the patch's attempt to fix the concurrent access window in `scx_bpf_cid_topo()` is incomplete.
--

--- Patch [3]: [PATCH 3/3] sched_ext: Guard the cid kfuncs against unallocated cid tables ---
commit 88c849437e9df48602b5417fe50640fa8c0c6abe
Author: Tejun Heo <tj@kernel.org>

sched_ext: Guard the cid kfuncs against unallocated cid tables

This commit adds NULL checks for the global CID tables in
scx_cid_to_cpu(), scx_cpu_to_cid(), and scx_bpf_cid_topo() to prevent
NULL pointer dereferences when BPF tracing programs execute during the
enable window.

> sched_ext: Guard the cid kfuncs against unallocated cid tables
> 
> The cid tables are allocated by the first enable's scx_cid_init(), which
> runs after scx_alloc_and_add_sched() has published ops->priv. A TRACING or
> SYSCALL program associated with the enabling struct_ops map gets a non-NULL
> sched from scx_prog_sched() as soon as ops->priv is set, so a cid kfunc
> called in that window dereferences the still-NULL table pointer. Only the
> first enable since boot is exposed as the tables are never freed.
> 
> scx_bpf_this_cid() and scx_bpf_task_cid() already handle the window by
> testing the table pointer. Do the same in scx_cid_to_cpu(),
> scx_cpu_to_cid() and scx_bpf_cid_topo(), returning -EINVAL / all-(-1) topo
> as before any scheduler is enabled. __scx_cid_to_cpu() and
> __scx_cpu_to_cid() stay unchecked for callers with the tables guaranteed
> allocated - ops invocations on a live scheduler and the enable path itself.

[Severity: High]
This isn't a bug introduced by this patch, but the commit message explicitly
claims the tables are guaranteed to be allocated on the enable path. Is this
fully accurate if memory allocation fails?

If memory allocation fails in scx_cid_arrays_alloc() during scx_cid_init(),
it leaves the table pointers as NULL and returns -ENOMEM:

kernel/sched/ext/cid.c:scx_cid_arrays_alloc() {
    if (!cid_to_cpu || !cpu_to_cid || !cid_topo) {
        kfree(cid_to_cpu);
        kfree(cpu_to_cid);
        kfree(cid_topo);
        return -ENOMEM;
    }
}

The error handling path jumps to err_disable which calls scx_error(). This
queues an IRQ work that executes scx_dump_state(), which iterates over all
CPUs and calls scx_dump_cpu().

scx_dump_cpu() invokes the scheduler's dump_cpu op:

kernel/sched/ext/ext.c:scx_dump_cpu() {
    SCX_CALL_OP(sch, dump_cpu, rq, dctx, scx_cpu_arg(cpu), idle);
}

Because __scx_is_cid_type was already enabled via static_branch_enable()
prior to the failed initialization, scx_cpu_arg() calls __scx_cpu_to_cid(),
which unconditionally dereferences the NULL scx_cpu_to_cid_tbl.

Could this lead to a deterministic NULL pointer dereference during the
scheduler error disable path?

> diff --git a/kernel/sched/ext/cid.c b/kernel/sched/ext/cid.c
> index af83084ec740a..712920b79c9c7 100644
> --- a/kernel/sched/ext/cid.c
> +++ b/kernel/sched/ext/cid.c
> @@ -665,17 +665,18 @@ bool scx_cmask_empty(const struct scx_cmask *m)
>  __bpf_kfunc void scx_bpf_cid_topo(s32 cid, struct scx_cid_topo *out__uninit,
>  				  const struct bpf_prog_aux *aux)
>  {
> +	struct scx_cid_topo *topo = READ_ONCE(scx_cid_topo);
>  	struct scx_sched *sch;
>  
>  	guard(rcu)();
>  
>  	sch = scx_prog_sched(aux);
> -	if (unlikely(!sch) || !cid_valid(sch, cid)) {
> +	if (unlikely(!sch) || !cid_valid(sch, cid) || unlikely(!topo)) {
>  		*out__uninit = SCX_CID_TOPO_NEG;
>  		return;
>  	}
>  
> -	*out__uninit = READ_ONCE(scx_cid_topo)[cid];
> +	*out__uninit = topo[cid];

[Severity: High]
This isn't a bug introduced by this patch, but does the patch's attempt to
fix the concurrent access window in scx_bpf_cid_topo() expose uninitialized
memory?

scx_cid_arrays_alloc() allocates scx_cid_topo without zeroing the memory
and immediately publishes it globally:

kernel/sched/ext/cid.c:scx_cid_arrays_alloc() {
    WRITE_ONCE(scx_cid_topo, cid_topo);
}

If a TRACING or SYSCALL BPF program executes concurrently before the array
is fully populated, the !topo check will pass, but topo[cid] may still
contain uninitialized kernel heap memory which is then copied to the BPF
output structure.

Additionally, scx_bpf_cid_override() modifies this structure:

kernel/sched/ext/cid.c:scx_bpf_cid_override() {
    scx_cid_topo[cid] = SCX_CID_TOPO_NEG;
}

Using non-atomic 16-byte writes while concurrent lockless readers can access
it could cause data races or tearing.

Should the array be zero-initialized or its population synchronized before
publishing?

>  }
>  
>  __bpf_kfunc_end_defs();
> diff --git a/kernel/sched/ext/cid.h b/kernel/sched/ext/cid.h
> index 9c4f4b907f12d..4ac79f6d70566 100644
> --- a/kernel/sched/ext/cid.h
> +++ b/kernel/sched/ext/cid.h

[ ... ]

> @@ -117,13 +118,17 @@ static inline s32 __scx_cpu_to_cid(s32 cpu)
>   * Return the cpu for @cid or a negative errno on failure. Invalid cid triggers
>   * scx_error() on @sch. The cid arrays are allocated on first scheduler enable
>   * and never freed, so the returned cpu is stable for the lifetime of the loaded
> - * scheduler.
> + * scheduler. Return -EINVAL without triggering scx_error() if the tables are
> + * not allocated yet, which a prog-facing kfunc can observe while racing the
> + * first enable.
>   */
>  static inline s32 scx_cid_to_cpu(struct scx_sched *sch, s32 cid)
>  {
> -	if (!cid_valid(sch, cid))
> +	s16 *tbl = READ_ONCE(scx_cid_to_cpu_tbl);
> +
> +	if (!cid_valid(sch, cid) || unlikely(!tbl))
>  		return -EINVAL;
> -	return __scx_cid_to_cpu(cid);
> +	return tbl[cid];

[Severity: Low]
Does replacing this call to __scx_cid_to_cpu() with a direct array read
leave the __scx_cid_to_cpu() inline function completely unused in the tree?
Could it be safely removed as dead code?

>  }

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