Re: [PATCH v4 sched_ext/for-7.3 39/40] tools/sched_ext: scx_qmap - Expand hierarchical sub-scheduling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] `dispatch_highpri()` ignores hierarchical CID delegation, allowing the parent scheduler to dispatch its high-priority tasks to CPUs exclusively delegated to child schedulers.
- [Medium] Unsynchronized modification of `qa.sub_sched_ctxs` array during `qmap_sub_attach` and `qmap_sub_detach` causes data races with partition rebuilding.
- [Medium] Double read (TOCTOU race) of `qa.sub_sched_ctxs[owner].cgroup_id` in `qmap_dispatch` can pass 0 to `scx_bpf_sub_dispatch`.
- [Medium] Userspace `feed_weights()` overwrites child weight with stale data due to concurrent sub-scheduler detach/attach (TOCTOU).
- [Medium] Negative error codes from `scx_bpf_task_cid()` are used directly as array and bitmask indices without bounds checking.
--

--- Patch [39]: [PATCH v4 sched_ext/for-7.3 39/40] tools/sched_ext: scx_qmap - Expand hierarchical sub-scheduling ---
commit d8ef8ae12fe2035b97cf3007ec38b1e29eef5a35
Author: Tejun Heo <tj@kernel.org>

tools/sched_ext: scx_qmap - Expand hierarchical sub-scheduling

This commit expands the scx_qmap test scheduler to demonstrate hierarchical
sub-scheduling. It allows a parent to delegate exclusive and shared CPUs
(cids) to child cgroups based on cpu.weight.

> diff --git a/tools/sched_ext/scx_qmap.bpf.c b/tools/sched_ext/scx_qmap.bpf.c
> index f6cfe63425d36..5b1c3b10ac746 100644
> --- a/tools/sched_ext/scx_qmap.bpf.c
> +++ b/tools/sched_ext/scx_qmap.bpf.c
[ ... ]
> @@ -416,7 +476,8 @@ void BPF_STRUCT_OPS(qmap_enqueue, struct task_struct *p, u64 enq_flags)
>  	 */
>  	if (taskc->force_local) {
>  		taskc->force_local = false;
> -		scx_bpf_dsq_insert(p, SCX_DSQ_LOCAL, slice_ns, enq_flags);
> +		scx_bpf_dsq_insert(p, SCX_DSQ_LOCAL, slice_ns,
> +				   enq_flags | needs_immed(scx_bpf_task_cid(p)));
>  		return;
>  	}

[Severity: Medium]
The core kernel kfunc scx_bpf_task_cid() can return an s32 negative error 
code (such as -EINVAL) if the CID table is uninitialized or the CPU mapping 
is missing.

When this return value is passed directly to needs_immed(), the negative 
value is used to index into the qa.cid_shared array without bounds checking. 
Can this lead to an out-of-bounds memory read within the BPF arena?

Similarly, in pick_direct_dispatch_cid(), prev_cid is passed to bitmask 
operations like cmask_test_and_clear() and cmask_test(). Should there be a 
check to ensure the returned CID is >= 0 before using it as an index?

[ ... ]
> @@ -541,7 +604,7 @@ static bool dispatch_highpri(bool from_timer)
[ ... ]
>  		if (scx_bpf_dsq_move(BPF_FOR_EACH_ITER, p, SCX_DSQ_LOCAL_ON | cid,
> -				     SCX_ENQ_PREEMPT)) {
> +				     SCX_ENQ_PREEMPT | needs_immed(cid))) {
>  			if (cid == this_cid) {
>  				dispatched = true;

[Severity: Medium]
In qmap_dispatch(), dispatch_highpri(false) is called unconditionally before 
checking qa.part.cid_owner. 

Inside dispatch_highpri(), tasks are moved to the local DSQ without verifying 
if the target cid is restricted by qa.self_cids.mask.

Does this sequence allow the parent scheduler's high-priority tasks to be 
dispatched to CPUs that are exclusively delegated to child sub-schedulers, 
thereby violating the delegation policy?

[ ... ]
> @@ -567,12 +630,35 @@ void BPF_STRUCT_OPS(qmap_dispatch, s32 cid, struct task_struct *prev)
[ ... ]
> +	} else if (owner >= 0 && owner < MAX_SUB_SCHEDS &&
> +		   qa.sub_sched_ctxs[owner].cgroup_id) {
> +		if (scx_bpf_sub_dispatch(qa.sub_sched_ctxs[owner].cgroup_id))
> +			__sync_fetch_and_add(&qa.sub_sched_ctxs[owner].nr_dsps, 1);
> +		return;
> +	}

[Severity: Medium]
The cgroup_id is read twice without protection here. The first read evaluates 
qa.sub_sched_ctxs[owner].cgroup_id as a condition for the if statement. 

Immediately after, the same memory location is re-read to be passed to 
scx_bpf_sub_dispatch().

Because qa.sub_sched_ctxs can be concurrently modified by qmap_sub_detach() 
without locks, could the value become 0 between the condition check and the 
function call, causing scx_bpf_sub_dispatch() to be invoked with an invalid 
cgroup ID?

[ ... ]
> @@ -1219,15 +1831,37 @@ void BPF_STRUCT_OPS(qmap_sub_detach, struct scx_sub_detach_args *args)
>  	s32 i;
>  
>  	for (i = 0; i < MAX_SUB_SCHEDS; i++) {
> -		if (qa.sub_sched_cgroup_ids[i] == args->ops->sub_cgroup_id) {
> -			qa.sub_sched_cgroup_ids[i] = 0;
> -			bpf_printk("detaching sub-sched[%d] on %s",
> -				   i, args->cgroup_path);
> -			break;
> -		}
> +		if (qa.sub_sched_ctxs[i].cgroup_id != args->ops->sub_cgroup_id)
> +			continue;
> +
> +		qa.sub_sched_ctxs[i].cgroup_id = 0;
> +		qa.sub_sched_ctxs[i].weight = 100;
> +		cmask_init(&qa.sub_sched_ctxs[i].granted_cids.mask, 0, qa.nr_cids);
> +		qa.nr_sub_scheds--;
> +		bpf_printk("detaching sub-sched[%d] on %s", i, args->cgroup_path);
> +		redistribute();
> +		break;
>  	}
>  }

[Severity: Medium]
During qmap_sub_detach() and qmap_sub_attach(), the global array 
qa.sub_sched_ctxs is modified directly (clearing cgroup_id and zeroing 
granted_cids.mask via cmask_init). 

These modifications occur without acquiring the part_try_start() / 
part_end() synchronization guard used in apply_partition() and rr_advance(). 

If apply_partition() runs concurrently from the timer callback or sys_bpf 
calls, can this cause a data race that corrupts the partition state tracking 
masks?

> diff --git a/tools/sched_ext/scx_qmap.c b/tools/sched_ext/scx_qmap.c
> index 9124183bffecc..1efffaaa8fe85 100644
> --- a/tools/sched_ext/scx_qmap.c
> +++ b/tools/sched_ext/scx_qmap.c
[ ... ]
> @@ -62,6 +92,213 @@ static void sigint_handler(int dummy)
[ ... ]
> +/* read each direct child's cpu.weight into the arena, true if any changed */
> +static bool feed_weights(struct qmap_arena *qa)
> +{
> +	bool changed = false;
> +	int i;
> +
> +	for (i = 0; i < MAX_SUB_SCHEDS; i++) {
> +		u64 cgid = qa->sub_sched_ctxs[i].cgroup_id;
> +		u32 w;
> +
> +		if (!cgid)
> +			continue;
> +		w = read_cgroup_weight(cgid);
> +		if (w && w != qa->sub_sched_ctxs[i].weight) {
> +			qa->sub_sched_ctxs[i].weight = w;
> +			changed = true;
> +		}
> +	}
> +	return changed;
> +}

[Severity: Medium]
In feed_weights(), userspace reads the cgroup_id from the BPF arena memory 
and then performs a blocking sysfs read via read_cgroup_weight().

During this file I/O operation, the original child cgroup could detach and a 
new one could attach to the same slot. Will the userspace daemon then 
blindly overwrite the newly attached child's qa->sub_sched_ctxs[i].weight 
with the stale weight read from the previous child's sysfs file?

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