Re: [PATCH v4 sched_ext/for-7.3 20/40] sched_ext: Stop resolving a disabled scheduler's programs

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

Pre-existing issues:
- [High] The `sch->dead` check was omitted from the `!CONFIG_EXT_SUB_SCHED` fallback version of `scx_prog_sched()`, leaving the Use-After-Exit hazard unfixed.
- [High] A parent scheduler can invoke a child scheduler's `ops.dispatch()` after the child's `ops.exit()` has already executed, bypassing the newly introduced `dead` flag protection.
--

--- Patch [20]: [PATCH v4 sched_ext/for-7.3 20/40] sched_ext: Stop resolving a disabled scheduler's programs ---
commit 4eb6882510c7d2ef68878ed595f1b7ecec9ef3cc
Author: Tejun Heo <[email protected]>

sched_ext: Stop resolving a disabled scheduler's programs

A scheduler's BPF programs can outlive it. A timer it armed or a tracing
program it loaded can fire after ops.exit() has run, before the programs are
unloaded, and scx_prog_sched() still resolves the program to its scheduler
through ops->priv. Harmless while kfuncs touch only lifetime-stable state,
but a hazard once a kfunc reads global state a newly loaded scheduler can
change underneath it.

Add scx_sched->dead, set right after ops.exit() and drained with
synchronize_rcu(). It follows exit() rather than preceding it so exit()'s own
kfunc calls still resolve to @sch. scx_prog_sched() returns NULL for a dead
scheduler, so every kfunc's existing !sch bail rejects it at one choke
point.

> diff --git a/kernel/sched/ext/internal.h b/kernel/sched/ext/internal.h
> index dce07b83c1663..87582bca5cc4f 100644
> --- a/kernel/sched/ext/internal.h
> +++ b/kernel/sched/ext/internal.h

[ ... ]

> @@ -1968,14 +1969,20 @@ static inline bool scx_task_on_sched(struct scx_sched *sch,
>  static inline struct scx_sched *scx_prog_sched(const struct bpf_prog_aux *aux)
>  {
>  	struct sched_ext_ops *ops;
> -	struct scx_sched *root;
> +	struct scx_sched *sch, *root;
>  
>  	ops = bpf_prog_get_assoc_struct_ops(aux);
> -	if (likely(ops))
> -		return rcu_dereference_all(ops->priv);
> +	if (likely(ops)) {
> +		sch = rcu_dereference_all(ops->priv);
> +		if (sch && unlikely(READ_ONCE(sch->dead)))
> +			return NULL;
> +		return sch;
> +	}
>  
>  	root = rcu_dereference_all(scx_root);
>  	if (root) {
> +		if (unlikely(READ_ONCE(root->dead)))
> +			return NULL;
>  		/*
>  		 * COMPAT-v6.19: Schedulers built before sub-sched support was
>  		 * introduced may have unassociated non-struct_ops programs.

[Severity: High]
This isn't a bug introduced by this patch, but does the fallback definition of
scx_prog_sched() also need the sch->dead check?

When the kernel is built with CONFIG_EXT_SUB_SCHED=n, scx_prog_sched() in
kernel/sched/ext/internal.h is defined as:

static inline struct scx_sched *scx_prog_sched(const struct bpf_prog_aux *aux)
{
	return rcu_dereference_all(scx_root);
}

If a BPF timer fires during the synchronize_rcu() window in scx_root_disable(),
it seems this fallback will unconditionally return scx_root without checking
its dead flag, allowing kfuncs to execute on torn-down scheduler state.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index 17ed0d28f3839..8d98250936497 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c
> @@ -344,6 +344,15 @@ void scx_sub_disable(struct scx_sched *sch)
>  
>  	if (sch->ops.exit)
>  		SCX_CALL_OP(sch, exit, NULL, sch->exit_info);
> +
> +	/*
> +	 * @sch's programs (an armed timer, a tracing prog) can fire after
> +	 * exit(). Now that exit()'s own kfuncs have resolved, stop
> +	 * scx_prog_sched() from resolving to @sch and drain in-flight resolvers.
> +	 */
> +	WRITE_ONCE(sch->dead, true);
> +	synchronize_rcu();
> +

[Severity: High]
This is a pre-existing issue, but can a parent scheduler bypass the newly
introduced dead flag and invoke a torn-down child scheduler's ops.dispatch()?

If a parent scheduler calls scx_dispatch_sched(child) from a BPF program
holding the RCU read lock while scx_sub_disable() is waiting on
synchronize_rcu():

Parent BPF program                  scx_sub_disable()
                                      child->ops.exit()
                                      WRITE_ONCE(child->dead, true);
                                      synchronize_rcu(); (blocks on parent)
scx_find_sub_sched()
scx_dispatch_sched(child)
   child->ops.dispatch()

It looks like scx_dispatch_sched() unconditionally invokes the dispatch
operation without checking the dead flag. Does scx_dispatch_sched() or
scx_bpf_sub_dispatch() need to check if the child is already dead to
prevent access to torn-down state?

>  	if (sch->sub_kset)
>  		kobject_del(&sch->sub_kset->kobj);
>  	/* not added if enable failed before scx_sched_sysfs_add() */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=20