Re: [PATCH] sched_ext: Allow ops.cgroup_set_weight/idle() to be sleepable

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent cgroup writes to `cpu.weight`, `cpu.shares`, or `cpu.idle` can permanently desynchronize the core scheduler and BPF scheduler state, and cause data races on SCX fields.
--

--- Patch [1]: [PATCH] sched_ext: Allow ops.cgroup_set_weight/idle() to be sleepable ---
commit 1a01a0e2918fcf8ca79c6152b56ecfa3e223935c
Author: Tao Cui <cuitao@kylinos.cn>

sched_ext: Allow ops.cgroup_set_weight/idle() to be sleepable

ops.cgroup_set_weight() and ops.cgroup_set_idle() are delivered from
scx_group_set_weight() and scx_group_set_idle(), which run from the
cpu.weight and cpu.idle cgroup interface write paths. This patch adds
them to the allow-list so these callbacks can allocate on demand.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index c539d15cda637..dd4ecc0b5201d 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -8079,7 +8079,9 @@ static int bpf_scx_check_member(const struct btf_type *t,
>  	case offsetof(struct sched_ext_ops, cgroup_init):
>  	case offsetof(struct sched_ext_ops, cgroup_exit):
>  	case offsetof(struct sched_ext_ops, cgroup_prep_move):
> +	case offsetof(struct sched_ext_ops, cgroup_set_weight):
>  	case offsetof(struct sched_ext_ops, cgroup_set_bandwidth):
> +	case offsetof(struct sched_ext_ops, cgroup_set_idle):

[Severity: High]
This isn't a bug introduced by this patch, but does making these operations
sleepable widen a race condition where concurrent cgroup writes could
permanently desynchronize the core scheduler and BPF scheduler state?

Looking at kernel/sched/core.c:cpu_weight_write_u64(), the core scheduler is
updated inside sched_group_set_shares() (which takes and drops shares_mutex),
leaving a gap before the BPF scheduler is updated:

kernel/sched/core.c:cpu_weight_write_u64() {
    ...
	ret = sched_group_set_shares(css_tg(css), scale_load(weight));
	if (!ret)
		scx_group_set_weight(css_tg(css), cgrp_weight);
    ...
}

Inside scx_group_set_weight(), the SCX scheduler state and tg->scx.weight
are updated without serialization (holding only a read lock):

kernel/sched/ext/ext.c:scx_group_set_weight() {
    ...
	SCX_CALL_OP(sch, cgroup_set_weight, NULL, tg_cgrp(tg), weight);

	tg->scx.weight = weight;
    ...
}

The same pattern exists in kernel/sched/core.c:cpu_idle_write_s64() calling
scx_group_set_idle() and updating tg->scx.idle.

If two threads concurrently write to cpu.weight or cpu.idle, they serialize on
shares_mutex but drop it before calling the SCX functions. Because this patch
allows ops.cgroup_set_weight and ops.cgroup_set_idle to sleep, could the
threads execute out of order? 

If Thread B's core scheduler update runs after Thread A's, but Thread B's BPF
callback finishes before Thread A's, the final state in the core scheduler
would be B, while the SCX state ends up as A. Can this lead to incorrect
functional behavior and data races on tg->scx.weight and tg->scx.idle?

>  #endif
>  	case offsetof(struct sched_ext_ops, cpu_online):

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825052336.46746-1-cui.tao@linux.dev?part=1