[PATCHSET sched_ext/for-7.3] sched_ext: Misc fixes

[PATCHSET sched_ext/for-7.3] sched_ext: Misc fixes

[Tejun Heo]

Hello,

Two unrelated small fixes.

 0001-sched_ext-Drop-unlocked-scx_rq_clock_invalidate-from.patch
 0002-sched_ext-Make-scx_bpf_events-read-the-calling-sched.patch

0001 drops the rq clock invalidation in the root disable path which was
performed without holding the rq lock and is redundant anyway. 0002 makes
scx_bpf_events() read the event counters of the scheduler associated with
the calling program instead of always reading the root scheduler's.

diffstat follows.

 kernel/sched/ext/ext.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

The patches are also applied to sched_ext/for-7.3.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext.git for-7.3

Thanks.

-- 
tejun

[PATCH 1/2] sched_ext: Drop unlocked scx_rq_clock_invalidate() from scx_root_disable()

[Tejun Heo]

scx_root_disable() invalidates each rq's clock before taking the rq lock.
scx_rq_clock_invalidate() is a plain read-modify-write of rq->scx.flags and
every other writer of the word runs under the rq lock, so the unlocked
update can race a concurrent flags update and lose one side's bits.

The invalidation is also redundant. The dl_server rebalancing right below
cycles each rq's lock and rq_unpin_lock() clears SCX_RQ_CLK_VALID on every
unlock, while __scx_enabled is turned off only later in the function, so the
clocks end up invalidated either way. Drop the call and note the
invalidation in the comment.

Fixes: 3a9910b5904d ("sched_ext: Implement scx_bpf_now()")
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Changwoo Min <changwoo@igalia.com>
---
 kernel/sched/ext/ext.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index 2adf2bde0cb1..d301fe757193 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -6476,12 +6476,13 @@ static void scx_root_disable(struct scx_sched *sch)
 	percpu_up_write(&scx_fork_rwsem);
 
 	/*
-	 * Invalidate all the rq clocks to prevent getting outdated
-	 * rq clocks from a previous scx scheduler.
+	 * Re-balance the dl_server bandwidth reservations: detach ext_server
+	 * (no more sched_ext tasks) and reinstate fair_server if it was
+	 * previously detached because we were running in full mode.
 	 *
-	 * Also re-balance the dl_server bandwidth reservations: detach
-	 * ext_server (no more sched_ext tasks) and reinstate fair_server if it
-	 * was previously detached because we were running in full mode.
+	 * The rq lock cycle also invalidates each rq's clock (rq_unpin_lock()
+	 * clears SCX_RQ_CLK_VALID on unlock), preventing the next enable from
+	 * seeing outdated rq clocks from this scheduler.
 	 *
 	 * Unlike the enable path, this runs on a recovery path that cannot
 	 * fail, so we use dl_server_swap_bw() to atomically free ext_server's
@@ -6494,8 +6495,6 @@ static void scx_root_disable(struct scx_sched *sch)
 	for_each_possible_cpu(cpu) {
 		struct rq *rq = cpu_rq(cpu);
 
-		scx_rq_clock_invalidate(rq);
-
 		scoped_guard(rq_lock_irqsave, rq) {
 			update_rq_clock(rq);
 			if (was_switched_all) {
-- 
2.55.0

Re: [PATCH 1/2] sched_ext: Drop unlocked scx_rq_clock_invalidate() from scx_root_disable()

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `SCX_RQ_CLK_VALID` can be permanently stranded as 1 during `scx_root_disable()`, causing the next SCX scheduler to read a massively outdated clock via `scx_bpf_now()`.
--

--- Patch [1]: [PATCH 1/2] sched_ext: Drop unlocked scx_rq_clock_invalidate() from scx_root_disable() ---
commit c8d4a8f1095e17e1c662f243f67711302652b61f
Author: Tejun Heo <tj@kernel.org>

sched_ext: Drop unlocked scx_rq_clock_invalidate() from scx_root_disable()

This commit drops the unlocked scx_rq_clock_invalidate() call during
scx_root_disable(), relying instead on the rq lock cycle and unlock
path to clear the clock valid flags.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 18183062f751c..40c9467d8b186 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -6162,12 +6162,13 @@ static void scx_root_disable(struct scx_sched *sch)
>  	percpu_up_write(&scx_fork_rwsem);
>  
>  	/*
> -	 * Invalidate all the rq clocks to prevent getting outdated
> -	 * rq clocks from a previous scx scheduler.
> +	 * Re-balance the dl_server bandwidth reservations: detach ext_server
> +	 * (no more sched_ext tasks) and reinstate fair_server if it was
> +	 * previously detached because we were running in full mode.
>  	 *
> -	 * Also re-balance the dl_server bandwidth reservations: detach
> -	 * ext_server (no more sched_ext tasks) and reinstate fair_server if it
> -	 * was previously detached because we were running in full mode.
> +	 * The rq lock cycle also invalidates each rq's clock (rq_unpin_lock()
> +	 * clears SCX_RQ_CLK_VALID on unlock), preventing the next enable from
> +	 * seeing outdated rq clocks from this scheduler.

[Severity: High]
This problem wasn't introduced by this patch, but is it possible for the
SCX_RQ_CLK_VALID flag to be permanently stranded as 1 during
scx_root_disable()?

The commit message and comment note that rq_unpin_lock() clears the flag
on unlock, and __scx_enabled is turned off later. However, if a concurrent
CPU takes the rq lock after the cycle but before __scx_enabled is disabled,
it seems a race can occur:

CPU N (Normal scheduling):
Takes rq lock
update_rq_clock() sets SCX_RQ_CLK_VALID (since __scx_enabled is still true)

CPU 0 (scx_root_disable):
static_branch_disable(&__scx_enabled);

CPU N (Normal scheduling):
Releases rq lock -> rq_unpin_lock() -> scx_rq_clock_invalidate()
scx_rq_clock_invalidate() in kernel/sched/sched.h checks:
    if (!scx_enabled())
        return;

Because scx_enabled() now evaluates to false, the function returns early and
fails to clear SCX_RQ_CLK_VALID.

When a new BPF scheduler is loaded later, early hooks like ops.select_cpu
that run without holding the rq lock might call scx_bpf_now() and see
SCX_RQ_CLK_VALID == 1. Will this cause them to use a massively outdated
clock from the previous scheduler's run?

>  	 *
>  	 * Unlike the enable path, this runs on a recovery path that cannot
>  	 * fail, so we use dl_server_swap_bw() to atomically free ext_server's
> @@ -6180,8 +6181,6 @@ static void scx_root_disable(struct scx_sched *sch)
>  	for_each_possible_cpu(cpu) {
>  		struct rq *rq = cpu_rq(cpu);
>  
> -		scx_rq_clock_invalidate(rq);
> -
>  		scoped_guard(rq_lock_irqsave, rq) {
>  			update_rq_clock(rq);
>  			if (was_switched_all) {

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

[PATCH v2 1/2] sched_ext: Drop unlocked scx_rq_clock_invalidate() from scx_root_disable()

[Tejun Heo]

scx_root_disable() invalidates each rq's clock before taking the rq lock.
scx_rq_clock_invalidate() is a plain read-modify-write of rq->scx.flags and
every other writer of the word runs under the rq lock, so the unlocked
update can race a concurrent flags update and lose one side's bits.

The invalidation doesn't matter in the first place. The cached clock is read
only by scx_bpf_now() from a loaded scheduler's BPF programs, nothing can
re-validate the clock while sched_ext is disabled as scx_rq_clock_update()
is gated on scx_enabled() too, and the usual rq lock cycles under the next
scheduler refresh or invalidate it before it's practically observable. Drop
the invalidation instead of fixing the locking.

v2: Description and comment updated - the invalidation is unnecessary rather
    than subsumed by the rq lock cycle below.

Fixes: 3a9910b5904d ("sched_ext: Implement scx_bpf_now()")
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Changwoo Min <changwoo@igalia.com>
---
 kernel/sched/ext/ext.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -6367,12 +6367,9 @@ static void scx_root_disable(struct scx_
 	percpu_up_write(&scx_fork_rwsem);
 
 	/*
-	 * Invalidate all the rq clocks to prevent getting outdated
-	 * rq clocks from a previous scx scheduler.
-	 *
-	 * Also re-balance the dl_server bandwidth reservations: detach
-	 * ext_server (no more sched_ext tasks) and reinstate fair_server if it
-	 * was previously detached because we were running in full mode.
+	 * Re-balance the dl_server bandwidth reservations: detach ext_server
+	 * (no more sched_ext tasks) and reinstate fair_server if it was
+	 * previously detached because we were running in full mode.
 	 *
 	 * Unlike the enable path, this runs on a recovery path that cannot
 	 * fail, so we use dl_server_swap_bw() to atomically free ext_server's
@@ -6385,8 +6382,6 @@ static void scx_root_disable(struct scx_
 	for_each_possible_cpu(cpu) {
 		struct rq *rq = cpu_rq(cpu);
 
-		scx_rq_clock_invalidate(rq);
-
 		scoped_guard(rq_lock_irqsave, rq) {
 			update_rq_clock(rq);
 			if (was_switched_all) {

[PATCH 2/2] sched_ext: Make scx_bpf_events() read the calling scheduler's counters

[Tejun Heo]

scx_bpf_events() always reads the root scheduler's event counters, so a
sub-scheduler program querying its own events silently gets the root's
instead and has no BPF-visible way to read its own (the per-scheduler sysfs
"events" file is the only interface). Resolve the scheduler from the calling
program with scx_prog_sched(). Unassociated programs follow the usual
scx_prog_sched() resolution: the root scheduler under a pre-sub-attach
compat root and zeroed counters otherwise.

Also fix up the malformed comment into proper kerneldoc.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/sched/ext/ext.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index d301fe757193..b9060c318c3c 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -10608,19 +10608,23 @@ static void scx_read_events(struct scx_sched *sch, struct scx_event_stats *event
 	}
 }
 
-/*
- * scx_bpf_events - Get a system-wide event counter to
+/**
+ * scx_bpf_events - Read the event counters of the calling scheduler
  * @events: output buffer from a BPF program
- * @events__sz: @events len, must end in '__sz'' for the verifier
+ * @events__sz: @events len, must end in '__sz' for the verifier
+ * @aux: implicit BPF argument to access bpf_prog_aux hidden from BPF progs
+ *
+ * Read the event counters of the scheduler associated with the calling program.
+ * @events is zeroed when no scheduler can be resolved.
  */
-__bpf_kfunc void scx_bpf_events(struct scx_event_stats *events,
-				size_t events__sz)
+__bpf_kfunc void scx_bpf_events(struct scx_event_stats *events, size_t events__sz,
+				const struct bpf_prog_aux *aux)
 {
 	struct scx_sched *sch;
 	struct scx_event_stats e_sys;
 
 	rcu_read_lock();
-	sch = rcu_dereference(scx_root);
+	sch = scx_prog_sched(aux);
 	if (sch)
 		scx_read_events(sch, &e_sys);
 	else
@@ -10743,7 +10747,7 @@ BTF_ID_FLAGS(func, scx_bpf_cpu_curr, KF_IMPLICIT_ARGS | KF_RET_NULL | KF_RCU_PRO
 BTF_ID_FLAGS(func, scx_bpf_cid_curr, KF_IMPLICIT_ARGS | KF_RET_NULL | KF_RCU_PROTECTED)
 BTF_ID_FLAGS(func, scx_bpf_tid_to_task, KF_RET_NULL | KF_RCU_PROTECTED)
 BTF_ID_FLAGS(func, scx_bpf_now)
-BTF_ID_FLAGS(func, scx_bpf_events)
+BTF_ID_FLAGS(func, scx_bpf_events, KF_IMPLICIT_ARGS)
 #ifdef CONFIG_CGROUP_SCHED
 BTF_ID_FLAGS(func, scx_bpf_task_cgroup, KF_IMPLICIT_ARGS | KF_RCU | KF_ACQUIRE)
 #endif
-- 
2.55.0

Re: [PATCH 2/2] sched_ext: Make scx_bpf_events() read the calling scheduler's counters

[sashiko-bot]

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

Pre-existing issues:
- [High] A tracing or syscall BPF program can cause an out-of-bounds memory read by associating with a non-sched_ext struct_ops map (like `tcp_congestion_ops`), which is then incorrectly cast to `struct sched_ext_ops *` by `scx_prog_sched()`.
--

--- Patch [2]: [PATCH 2/2] sched_ext: Make scx_bpf_events() read the calling scheduler's counters ---
commit c53feaf881922f5f2912216d45a7b765df97ad06
Author: Tejun Heo <tj@kernel.org>

sched_ext: Make scx_bpf_events() read the calling scheduler's counters

scx_bpf_events() now resolves the scheduler from the calling program with
scx_prog_sched(), instead of always reading the root scheduler's event
counters.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 40c9467d8b186..ee865d0f65112 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -10418,19 +10418,23 @@ static void scx_read_events(struct scx_sched *sch, struct scx_event_stats *event
>  	}
>  }
>  
> -/*
> - * scx_bpf_events - Get a system-wide event counter to
> +/**
> + * scx_bpf_events - Read the event counters of the calling scheduler
>   * @events: output buffer from a BPF program
> - * @events__sz: @events len, must end in '__sz'' for the verifier
> + * @events__sz: @events len, must end in '__sz' for the verifier
> + * @aux: implicit BPF argument to access bpf_prog_aux hidden from BPF progs
> + *
> + * Read the event counters of the scheduler associated with the calling program.
> + * @events is zeroed when no scheduler can be resolved.
>   */
> -__bpf_kfunc void scx_bpf_events(struct scx_event_stats *events,
> -				size_t events__sz)
> +__bpf_kfunc void scx_bpf_events(struct scx_event_stats *events, size_t events__sz,
> +				const struct bpf_prog_aux *aux)
>  {
>  	struct scx_sched *sch;
>  	struct scx_event_stats e_sys;
>  
>  	rcu_read_lock();
> -	sch = rcu_dereference(scx_root);
> +	sch = scx_prog_sched(aux);

[Severity: High]
This is a pre-existing issue, but does this expose us to a type confusion
and out-of-bounds read?

Looking at scx_prog_sched() in kernel/sched/ext/internal.h, it retrieves
associated struct_ops:

kernel/sched/ext/internal.h:scx_prog_sched() {
    ...
    ops = bpf_prog_get_assoc_struct_ops(aux);
    if (likely(ops))
        return rcu_dereference_all(ops->priv);
    ...
}

If a BPF_PROG_TYPE_TRACING or BPF_PROG_TYPE_SYSCALL program is associated
with a different struct_ops map (like tcp_congestion_ops), the pointer is
blindly cast to struct sched_ext_ops *.

Since struct sched_ext_ops is much larger than other struct_ops structures,
reading ops->priv could read memory well past the end of the map allocation
and cause a kernel panic when the garbage value is dereferenced as a
struct scx_sched *.

Does scx_prog_sched() need to verify the type of the associated struct_ops
before accessing ops->priv?

>  	if (sch)
>  		scx_read_events(sch, &e_sys);
>  	else

[ ... ]

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

Re: [PATCH 2/2] sched_ext: Make scx_bpf_events() read the calling scheduler's counters

[Tejun Heo]

Hello,

> If a BPF_PROG_TYPE_TRACING or BPF_PROG_TYPE_SYSCALL program is associated
> with a different struct_ops map (like tcp_congestion_ops), the pointer is
> blindly cast to struct sched_ext_ops *.
>
> Since struct sched_ext_ops is much larger than other struct_ops structures,
> reading ops->priv could read memory well past the end of the map allocation
> and cause a kernel panic when the garbage value is dereferenced as a
> struct scx_sched *.

The hole exists but reaching it requires the privileged userspace to
deliberately associate its own tracing or syscall program with a wrong type
of struct_ops map and then call SCX kfuncs from it. Association is an
explicit per-program operation, so this can't happen accidentally. The
right fix is rejecting the association of a program which uses SCX kfuncs
with a non-sched_ext struct_ops map at association time, which the
association path has enough information to do. Will worry about it later.

Thanks.

-- 
tejun