Re: [PATCH v2 sched_ext/for-7.3 4/4] sched_ext: Split sub-scheduler implementation into sub.c

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Compilation failure due to implicit function declarations in `sub.h`.

Pre-existing issues:
- [High] The `scx_bpf_sub_dispatch` kfunc breaks runqueue lock atomicity for the parent BPF program.
- [High] Missing `scx_error()` calls in `scx_sub_enable_workfn()` lead to silent sub-scheduler initialization failures and zombie states.
--

commit c73a80aa8b4b758cfebb3ce05e858b8b1302f361
Author: Tejun Heo <tj@kernel.org>
Subject: sched_ext: Split sub-scheduler implementation into sub.c

This commit moves the sub-scheduler implementation from ext.c into a new
sub.c file. It also moves shared prototypes to sub.h and makes
scx_dispatch_sched() an inline function in sub.h.

> diff --git a/kernel/sched/ext/sub.h b/kernel/sched/ext/sub.h
> new file mode 100644
> index 0000000000000..460a9fd196dc6
> --- /dev/null
> +++ b/kernel/sched/ext/sub.h

[ ... ]

> +static __always_inline bool
> +scx_dispatch_sched(struct scx_sched *sch, struct rq *rq,
> +		   struct task_struct *prev, bool nested)
> +{
> +	struct scx_dsp_ctx *dspc = &this_cpu_ptr(sch->pcpu)->dsp_ctx;
> +	int nr_loops = SCX_DSP_MAX_LOOPS;
> +	s32 cpu = cpu_of(rq);
> +	bool prev_on_sch = (prev->sched_class == &ext_sched_class) &&
> +		scx_task_on_sched(sch, prev);
> +
> +	if (scx_consume_global_dsq(sch, rq))
> +		return true;

[ ... ]

> +	if (unlikely(!SCX_HAS_OP(sch, dispatch)) || !scx_rq_online(rq))
> +		return false;

[Severity: Low]
Does this cause a compilation failure due to implicit function declarations?

In kernel/sched/build_policy.c, ext/sub.h is included before ext/ext.c. When
the compiler parses this scx_dispatch_sched() inline function in sub.h,
functions like scx_rq_online(), scx_flush_dispatch_buf(), and scx_kick_cpu()
have not been declared yet because they are defined later as static functions
in ext.c. This will result in an implicit function declaration error.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> new file mode 100644
> index 0000000000000..0504204272732
> --- /dev/null
> +++ b/kernel/sched/ext/sub.c

[ ... ]

> +void scx_sub_enable_workfn(struct kthread_work *work)
> +{
> +	struct scx_enable_cmd *cmd = container_of(work, struct scx_enable_cmd, work);
> +	struct sched_ext_ops *ops = cmd->ops;
> +	struct cgroup *cgrp;

[ ... ]

> +err_disable:
> +	mutex_unlock(&scx_enable_mutex);
> +	scx_flush_disable_work(sch);
> +	cmd->ret = 0;
> +}

[Severity: High]
This is a pre-existing issue, but do missing scx_error() calls lead to silent
sub-scheduler initialization failures?

If initialization fails (for example, inside scx_arena_pool_init()), the code
jumps to the err_disable label. Unlike scx_root_enable_workfn() which
explicitly calls scx_error() to transition the scheduler to a disabled state
and queue cleanup work, scx_sub_enable_workfn() omits this call.

Consequently, scx_flush_disable_work() does nothing because the exit state
remains SCX_EXIT_NONE, and the function returns success to userspace by
setting cmd->ret = 0, leaving the sub-scheduler permanently half-initialized.

[ ... ]

> +__bpf_kfunc bool scx_bpf_sub_dispatch(u64 cgroup_id, const struct bpf_prog_aux *aux)
> +{
> +	struct rq *this_rq = this_rq();
> +	struct scx_sched *parent, *child;
> +
> +	guard(rcu)();
> +	parent = scx_prog_sched(aux);
> +	if (unlikely(!parent))
> +		return false;
> +
> +	child = scx_find_sub_sched(cgroup_id);
> +
> +	if (unlikely(!child))
> +		return false;
> +
> +	if (unlikely(scx_parent(child) != parent)) {
> +		scx_error(parent, "trying to dispatch a distant sub-sched on cgroup %llu",
> +			  cgroup_id);
> +		return false;
> +	}
> +
> +	return scx_dispatch_sched(child, this_rq, this_rq->scx.sub_dispatch_prev,
> +				  true);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does the scx_bpf_sub_dispatch()
kfunc break runqueue lock atomicity for the parent BPF program?

The BPF verifier and scheduler core assume that a non-sleepable ops.dispatch
BPF program executes atomically while continuously holding the rq lock.
However, when the parent BPF scheduler calls this kfunc, it evaluates the
child scheduler's dispatch logic which calls scx_flush_dispatch_buf().

Flushing the buffer can invoke dispatch_to_local_dsq(), which drops and
re-acquires the rq lock. Dropping the lock underneath the still-executing
parent BPF program violates the atomicity assumption, which can allow
concurrent CPUs to modify the runqueue.

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