Re: [PATCH] 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:
- [High] Concurrent writes to the cgroup `cpu.max` interface can race, causing desynchronization between CFS and SCX bandwidth states and out-of-order invocations of the BPF callback.
--

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

sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable

This commit allows the cgroup_set_bandwidth BPF scheduler operation to
sleep. It adds a BTF marker function for userspace to detect this
capability and documents the potential blocking behavior.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 18183062f751c..c48561bf108be 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -4572,6 +4572,15 @@ void scx_group_set_bandwidth(struct task_group *tg,
>  
>  	percpu_up_read(&scx_cgroup_ops_rwsem);
>  }

[Severity: High]
This is a pre-existing issue, but does making this operation sleepable widen
the window for a race condition between concurrent cpu.max writers?

The commit message notes that cfs_constraints_mutex has already been released
by tg_set_cfs_bandwidth(), leaving only percpu_down_read(&scx_cgroup_ops_rwsem)
held. Since this is a read lock, concurrent writers to the same cgroup's
cpu.max interface can execute scx_group_set_bandwidth() at the same time.

If two threads write to cpu.max, they are serialized in tg_set_cfs_bandwidth(),
but can race here:

CPU1                                 CPU2
tg_set_cfs_bandwidth() (writer A)
                                     tg_set_cfs_bandwidth() (writer B)
scx_group_set_bandwidth()
   percpu_down_read()
   SCX_CALL_OP(...) // blocks
                                     scx_group_set_bandwidth()
                                        percpu_down_read()
                                        SCX_CALL_OP(...) // returns quickly
                                        tg->scx.bw_quota_us = ...
   tg->scx.bw_quota_us = ...

Can this lead to the BPF callback executing out of order, causing
desynchronization between the CFS bandwidth state and the SCX scheduler state?

Additionally, on 32-bit architectures, could updating the 64-bit
tg->scx.bw_period_us, bw_quota_us, and bw_burst_us fields without a write
lock result in torn writes?

> +
> +/*
> + * Capability marker for userspace. The sleepable allowance for
> + * ops.cgroup_set_bandwidth() (see bpf_scx_check_member()) is a verifier
> + * property with no other symbol a scheduler can probe, so this no-op function
> + * exists solely so its presence in the kernel's BTF can be detected. It has no
> + * callers; __used keeps it from being optimized away.
> + */
> +__used void scx_cgroup_set_bandwidth_may_sleep(void) {}

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