Re: [PATCH v2] sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable

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

Pre-existing issues:
- [Medium] Concurrent cgroup writes to `cpu.max`, `cpu.weight`, or `cpu.idle` race to update the cached `tg->scx` state without an exclusive lock, causing the BPF scheduler's state to silently and permanently diverge from the cgroup state.
--

--- Patch [1]: [PATCH v2] sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable ---
commit dc54116ac36c38be3f813a52900e454dacbf26ad
Author: Changwoo Min <[email protected]>

sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable

This commit adds cgroup_set_bandwidth to the allow-list in
bpf_scx_check_member, allowing its BPF callback to sleep. It also adds a
BTF compatibility marker so userspace can detect this capability.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 10af28a9f2c0c..b646711a45fee 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -8079,6 +8079,7 @@ 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_bandwidth):

[Severity: Medium]
This is a pre-existing issue, but does making this callback sleepable
significantly widen a data race in cgroup state updates?

Looking at concurrent writes to cpu.max, tg_set_bandwidth() drops the
exclusive lock before calling into sched_ext:

kernel/sched/core.c:tg_set_bandwidth() {
	...
#ifdef CONFIG_CFS_BANDWIDTH
	ret = tg_set_cfs_bandwidth(tg, period_us, quota_us, burst_us);
#endif
	if (!ret)
		scx_group_set_bandwidth(tg, period_us, quota_us, burst_us);
}

The cfs_constraints_mutex is dropped inside tg_set_cfs_bandwidth(), so
scx_group_set_bandwidth() runs without an exclusive lock.

Then in scx_group_set_bandwidth(), only a read lock is held while executing
the BPF hook and updating the state:

kernel/sched/ext/ext.c:scx_group_set_bandwidth() {
	percpu_down_read(&scx_cgroup_ops_rwsem);
	...
	SCX_CALL_OP(sch, cgroup_set_bandwidth, NULL, tg_cgrp(tg), ...);
	
	tg->scx.bw_period_us = period_us;
	...
}

Since percpu_down_read() allows concurrent readers, two concurrent writes
can interleave. With the BPF program now allowed to block inside
SCX_CALL_OP(), could this cause the cached tg->scx state to permanently
diverge from the actual cgroup state?

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