sched_ext: Sub-scheduler and cid fixes
Subsystems (2)
Tejun Heo
reviewerPATCH 1/1 sched_ext: Sub-scheduler and cid fixes
Hello,
A few fixes for the sub-scheduler and cid-form code on for-7.3.
- 0001 attributes a runnable stall to the scheduler that owns the DSQ the task is stuck on rather than the task's owner, so a bypassing sub's task parked on an ancestor's bypass DSQ blames the ancestor, not the sub.
- 0002 skips the default CPU selection while bypassing. The pick is discarded by the bypass enqueue anyway, and it consults idle masks a scheduler doing its own idle tracking leaves frozen.
- 0003 guards the cid kfuncs against the cid tables not being allocated yet, which a program can hit while racing the first scheduler enable.
Based on sched_ext/for-7.3 (b20dfde5ec54).
Tejun Heo (3):
sched_ext: Blame the DSQ's owning scheduler for a runnable stall
sched_ext: Skip the default CPU selection while bypassing
sched_ext: Guard the cid kfuncs against unallocated cid tables
Git tree: git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext.git scx-sub-fixes
kernel/sched/ext/cid.c | 5 +++-- kernel/sched/ext/cid.h | 23 +++++++++++++++-------- kernel/sched/ext/ext.c | 26 ++++++++++++++++++++++++-- 3 files changed, 42 insertions(+), 12 deletions(-)
--
tejun
Tejun Heo
reviewerPATCH 1/3 sched_ext: Blame the DSQ's owning scheduler for a runnable stall
check_rq_for_timeouts() blames a runnable stall on the task's owner. Under a sub-scheduler hierarchy the stalled task can be sitting on a DSQ that a different scheduler has to drain, e.g. an ancestor's bypass DSQ while the owner is bypassing. The drainer then escapes blame while the owner is exited, and when the owner's exit is already claimed, nothing actionable is reported at all.
Blame the DSQ's owning scheduler instead. The local DSQ is consumed by the cpu itself and keeps blame on the owner. Detection keeps the owner's timeout and single-scheduler behavior is unchanged.
--- kernel/sched/ext/ext.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
kernel/sched/ext/ext.c +11 -0
@@ -3391,8 +3391,19 @@ static bool check_rq_for_timeouts(struct rq *rq)if (unlikely (time_after (jiffies, last_runnable +READ_ONCE (sch->watchdog_timeout)))) { +struct scx_dispatch_q *dsq =READ_ONCE (p->scx.dsq);u32 dur_ms =jiffies_to_msecs (jiffies - last_runnable); +/* +* A task can be stuck on a DSQ that a sched other than +* its owner is responsible for draining, e.g. an +* ancestor's bypass DSQ while the owner is bypassing. +* Blame the drainer. The local DSQ is consumed by the +* cpu itself and keeps blame on the owner. +*/ +if (dsq && dsq->sched && dsq->id !=SCX_DSQ_LOCAL ) + sch = dsq->sched; + __scx_exit(sch, SCX_EXIT_ERROR_STALL, 0, cpu_of(rq), "%s[%d] failed to run for %u.%03us", p->comm, p->pid, dur_ms / 1000,
Signature · 1 line
2.55.0
Tejun Heo
reviewerPATCH 2/3 sched_ext: Skip the default CPU selection while bypassing
In reply to Tejun Heoselect_task_rq_scx() falls into the default path when the scheduler has no ops.select_cpu or is bypassing. There it calls scx_select_cpu_dfl() and direct-dispatches to the picked CPU's local DSQ.
While bypassing, neither does anything: the enqueue path routes the task to a bypass DSQ before consulting the direct-dispatch target, so the direct dispatch never happens, and the CPU pick at most shifts which CPU's bypass DSQ receives the task. Worse, when the scheduler does its own idle tracking, the built-in idle cpumasks the pick consults are not even updated, so it doesn't work anyway.
Return prev_cpu without the default selection while bypassing and let the bypass enqueue place the task.
--- kernel/sched/ext/ext.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
kernel/sched/ext/ext.c +13 -2
@@ -3277,6 +3277,19 @@ static int select_task_rq_scx(struct task_struct *p, int prev_cpu, int wake_flag }else {s32 cpu; +/* +* While bypassing, the enqueue path routes @p to a bypass DSQ +* without consulting the direct-dispatch target, making the +* default selection pointless. It doesn't work anyway when the +* scheduler does its own idle tracking and the built-in idle +* cpumasks are not updated. Leave @p on @prev_cpu. +*/ +if (bypassing) { +__scx_add_event (sch,SCX_EV_BYPASS_DISPATCH ,1 ); + p->scx.selected_cpu = prev_cpu; +return prev_cpu; + } + cpu =scx_select_cpu_dfl (p, prev_cpu, wake_flags,NULL ,0 );if (cpu >=0 ) {refill_task_slice_dfl (sch, p);
@@ -3286,8 +3299,6 @@ static int select_task_rq_scx(struct task_struct *p, int prev_cpu, int wake_flag } p->scx.selected_cpu = cpu; -if (bypassing) -__scx_add_event (sch,SCX_EV_BYPASS_DISPATCH ,1 );return cpu; } }
Signature · 1 line
2.55.0
Tejun Heo
reviewerPATCH 3/3 sched_ext: Guard the cid kfuncs against unallocated cid tables
In reply to Tejun HeoThe 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.
--- kernel/sched/ext/cid.c | 5 +++-- kernel/sched/ext/cid.h | 23 +++++++++++++++-------- 2 files changed, 18 insertions(+), 10 deletions(-)
kernel/sched/ext/cid.c +3 -2
@@ -875,17 +875,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]; }__bpf_kfunc_end_defs ();
kernel/sched/ext/cid.h +15 -8
@@ -90,9 +90,10 @@ static inline bool cid_valid(struct scx_sched *sch, s32 cid)* __scx_cid_to_cpu - Unchecked cid->cpu table lookup * @cid: cid to look up. Must be in [0, num_possible_cpus()). * -* Intended for callsites that have already validated @cid and that hold a -* non-NULL @sch from scx_prog_sched() - a live sched implies the table has -* been allocated, so no NULL check is needed here. +* Intended for callsites that have already validated @cid and where the +* tables are guaranteed allocated - ops invocations on a live scheduler or +* the enable path itself. Prog-facing kfuncs, which can run while the first +* enable is still allocating the tables, use the checked wrappers instead. */ static inline s32 __scx_cid_to_cpu (s32 cid) {
@@ -119,13 +120,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]; }/**
@@ -138,9 +143,11 @@ static inline s32 scx_cid_to_cpu(struct scx_sched *sch, s32 cid)*/ static inline s32 scx_cpu_to_cid (struct scx_sched *sch,s32 cpu) { -if (!scx_cpu_valid (sch, cpu,NULL )) +s16 *tbl =READ_ONCE (scx_cpu_to_cid_tbl); + +if (!scx_cpu_valid (sch, cpu,NULL ) ||unlikely (!tbl))return -EINVAL ; -return __scx_cpu_to_cid (cpu); +return tbl[cpu]; }/**
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH 3/3] sched_ext: Guard the cid kfuncs against unallocated cid tables
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
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 <[email protected]>
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.
Quoted 9 lines
sched_ext: Guard the cid kfuncs against unallocated cid tablesThe cid tables are allocated by the first enable's
scx_cid_init(), which runs afterscx_alloc_and_add_sched()has publishedops->priv. A TRACING or SYSCALL program associated with the enablingstruct_opsmap gets a non-NULL sched fromscx_prog_sched()as soon asops->privis 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.
Quoted 6 lines
scx_bpf_this_cid()andscx_bpf_task_cid()already handle the window by testing the table pointer. Do the same inscx_cid_to_cpu(),scx_cpu_to_cid()andscx_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?
Quoted 12 lines
kernel/sched/ext/cid.c +1
-0@@ -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)();
kernel/sched/ext/cid.c bool scx_cmask_empty(const struct scx_cmask *m) · line 679
Quoted 10 lines
kernel/sched/ext/cid.c +2
-2sch =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?
Quoted 7 lines
kernel/sched/ext/cid.c +0
-0}__bpf_kfunc_end_defs ();kernel/sched/ext/cid.h +0
-0
[ ... ]
Quoted 8 lines
kernel/sched/ext/cid.h +3
-1@@ -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.
kernel/sched/ext/cid.h static inline s32 __scx_cpu_to_cid(s32 cpu) · line 131
Quoted 10 lines
kernel/sched/ext/cid.h +4
-2*/ 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?
Quoted 1 line
kernel/sched/ext/cid.h +0
-0}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Andrea Righi
reviewerRe: [PATCH 3/3] sched_ext: Guard the cid kfuncs against unallocated cid tables
In reply to Tejun HeoHi Tejun,
Quoted 26 lines · Tejun Heo
The cid tables are allocated by the first enable's
scx_cid_init(), which runs afterscx_alloc_and_add_sched()has publishedops->priv. A TRACING or SYSCALL program associated with the enablingstruct_opsmap gets a non-NULL sched fromscx_prog_sched()as soon asops->privis 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()andscx_bpf_task_cid()already handle the window by testing the table pointer. Do the same inscx_cid_to_cpu(),scx_cpu_to_cid()andscx_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.Fixes e9b55af47edf ("sched_ext: Add topological CPU IDs (cids)")Signed-off-by Tejun Heo <[email protected]>--- kernel/sched/ext/cid.c | 5 +++-- kernel/sched/ext/cid.h | 23 +++++++++++++++-------- 2 files changed, 18 insertions(+), 10 deletions(-)kernel/sched/ext/cid.c +0
-0
kernel/sched/ext/cid.c bool scx_cmask_empty(const struct scx_cmask *m) · line 878
Quoted 4 lines
kernel/sched/ext/cid.c +1
-0@@ -875,17 +875,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);
I think a non-NULL pointer here doesn't necessarily mean that the table is initialized. IIUC scx_cid_arrays_alloc() publishes these pointers before scx_cid_init() populates them, so a racing program can still observe zero-filled uninitialized cid_topo entries.
This also applies to the other enables, where the tables renamin non-NULL while being rewritten. Should we add a defer publication until initialization is complete or a readiness flag?
Thanks,
-Andrea
Quoted 72 lines
kernel/sched/ext/cid.c +2
-2struct 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]; }__bpf_kfunc_end_defs ();kernel/sched/ext/cid.h +15
-8@@ -90,9 +90,10 @@ static inline bool cid_valid(struct scx_sched *sch, s32 cid)* __scx_cid_to_cpu - Unchecked cid->cpu table lookup * @cid: cid to look up. Must be in [0, num_possible_cpus()). * -* Intended for callsites that have already validated @cid and that hold a -* non-NULL @sch from scx_prog_sched() - a live sched implies the table has -* been allocated, so no NULL check is needed here. +* Intended for callsites that have already validated @cid and where the +* tables are guaranteed allocated - ops invocations on a live scheduler or +* the enable path itself. Prog-facing kfuncs, which can run while the first +* enable is still allocating the tables, use the checked wrappers instead. */ static inline s32 __scx_cid_to_cpu (s32 cid) {@@ -119,13 +120,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]; }/** @@ -138,9 +143,11 @@ static inline s32 scx_cid_to_cpu(struct scx_sched *sch, s32 cid)*/ static inline s32 scx_cpu_to_cid (struct scx_sched *sch,s32 cpu) { -if (!scx_cpu_valid (sch, cpu,NULL )) +s16 *tbl =READ_ONCE (scx_cpu_to_cid_tbl); + +if (!scx_cpu_valid (sch, cpu,NULL ) ||unlikely (!tbl))return -EINVAL ; -return __scx_cpu_to_cid (cpu); +return tbl[cpu]; }/** Signature · 1 line
2.55.0