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 <[email protected]>
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/[email protected]?part=39
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.