Re: [PATCH v3 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:

Pre-existing issues:
- [High] `scx_sub_disable` violates RCU removal ordering, leading to Use-After-Free of sub-schedulers.
- [High] NULL pointer dereference in `find_parent_sched` when attaching a sub-scheduler to a newly created cgroup.
- [High] Missing `scx_error()` calls in `scx_sub_enable_workfn` error paths cause the sub-scheduler to leak and remain bypassed indefinitely.
--

commit 3673367fbf0f16668d479297e4d6ace4a6a2dbef
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
kernel/sched/ext/sub.c file. It also creates sub.h to hold prototypes and
inline functions like scx_dispatch_sched() for better modularity.

> 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_disable(struct scx_sched *sch)
> +{

[ ... ]

> +	/*
> +	 * All tasks are moved off of @sch but there may still be on-going
> +	 * operations (e.g. ops.select_cpu()). Drain them by flushing RCU. Use
> +	 * the expedited version as ancestors may be waiting in bypass mode.
> +	 * Also, tell the parent that there is no need to keep running bypass
> +	 * DSQs for us.
> +	 */
> +	synchronize_rcu_expedited();
> +	scx_disable_bypass_dsp(sch);
> +
> +	scx_unlink_sched(sch);

[Severity: High]
This is a pre-existing issue, but does this violate RCU removal ordering?

The sub-scheduler is removed from the scx_sched_hash via scx_unlink_sched()
after the RCU grace period completes. Because it remains in the hash during
and after the grace period, couldn't a concurrent BPF program on the parent
scheduler call scx_bpf_sub_dispatch() and find the sub-scheduler?

Should scx_unlink_sched() be called before synchronize_rcu_expedited() to
ensure new RCU readers cannot look it up?

[ ... ]

> +/* verify that a scheduler can be attached to @cgrp and return the parent */
> +static struct scx_sched *find_parent_sched(struct cgroup *cgrp)
> +{
> +	struct scx_sched *parent = cgrp->scx_sched;
> +	struct scx_sched *pos;
> +
> +	lockdep_assert_held(&scx_sched_lock);
> +
> +	/* can't attach twice to the same cgroup */
> +	if (parent->cgrp == cgrp)

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for parent
to be NULL here?

If a BPF program attaches a sub-scheduler to a newly created cgroup that
hasn't completed its ONLINE phase yet, cgrp->scx_sched might be NULL. The
code dereferences it immediately. 

The explicit check for CSS_ONLINE happens later in scx_sub_enable_workfn().
Should we verify parent is not NULL before dereferencing it?

[ ... ]

> +void scx_sub_enable_workfn(struct kthread_work *work)
> +{

[ ... ]

> +	ret = scx_link_sched(sch);
> +	if (ret)
> +		goto err_disable;
> +
> +	if (sch->level >= SCX_SUB_MAX_DEPTH) {
> +		scx_error(sch, "max nesting depth %d violated",
> +			  SCX_SUB_MAX_DEPTH);
> +		goto err_disable;
> +	}
> +
> +	if (sch->ops.init) {
> +		ret = SCX_CALL_OP_RET(sch, init, NULL);
> +		if (ret) {
> +			ret = scx_ops_sanitize_err(sch, "init", ret);
> +			scx_error(sch, "ops.init() failed (%d)", ret);
> +			goto err_disable;
> +		}
> +		sch->exit_info->flags |= SCX_EFLAG_INITIALIZED;
> +	}
> +
> +	ret = scx_arena_pool_init(sch);
> +	if (ret)
> +		goto err_disable;
> +
> +	ret = scx_set_cmask_scratch_alloc(sch);
> +	if (ret)
> +		goto err_disable;
> +
> +	if (scx_validate_ops(sch, ops))
> +		goto err_disable;

[Severity: High]
This is a pre-existing issue, but do we need to call scx_error() in these
failure paths?

If scx_link_sched(), scx_arena_pool_init(), scx_set_cmask_scratch_alloc(),
or scx_validate_ops() fail, the code jumps to err_disable without calling
scx_error().

Without scx_error(), sch->exit_kind remains SCX_EXIT_NONE. When
scx_flush_disable_work(sch) is executed at err_disable, it will immediately
return, leaving the sub-scheduler partially initialized and permanently
bypassed, while returning 0 (success) to userspace.

[ ... ]

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