Re: [PATCH 1/3] cgroup/cpuset: Protect is_in_v2_mode() in cpuset_num_cpus()

[Waiman Long <longman@redhat.com>]

On 9/29/26 1:35 PM, Andrea Righi wrote:
> Hi Michal and Waiman,
>
> On Tue, Sep 29, 2026 at 11:32:33AM -0400, Waiman Long wrote:
>> On 9/29/26 9:47 AM, Michal Koutný wrote:
>>> Hi.
>>>
>>> On Tue, Sep 29, 2026 at 10:37:38AM +0200, Andrea Righi <arighi@nvidia.com> wrote:
>>>> cpuset_num_cpus() enters its RCU read-side section only after checking
>>>> is_in_v2_mode(). When cpuset is bound to a v1 hierarchy, is_in_v2_mode()
>>>> dereferences cpuset_cgrp_subsys.root, which is freed via kfree_rcu()
>>>> once that hierarchy is destroyed and cpuset is rebound to the default
>>>> hierarchy. A preemptible caller outside RCU can therefore read the flags
>>>> of a freed root.
>>>>
>>>> The only current caller, fair's group share calculation, runs under the
>>>> rq lock with preemption disabled, so it can't hit this. However, the
>>>> helper already means to protect itself with RCU, and upcoming sched_ext
>>>> support exposes it to sleepable BPF programs.
>>>>
>>>> Take the RCU read lock before is_in_v2_mode() so that the whole lookup
>>>> is protected regardless of the caller's context.
>>> This feels like mere querying of the mode shouldn't require such
>>> constraints (despite it's needed anyway later down). But it could truly
>>> happen with the novel usage (CONFIG_CPUSET_V1 && unmounting cpuset
>>> hierarchy for some reason, I wonder how you noticed :)).
>>>
>>> Then I'd welcome more structured approach with at least:
>>>
>>> diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
>>> index 3754d697854b3..7f4d346cfb119 100644
>>> --- a/include/linux/cgroup-defs.h
>>> +++ b/include/linux/cgroup-defs.h
>>> @@ -841,7 +841,7 @@ struct cgroup_subsys {
>>>           const char *legacy_name;
>>>
>>>           /* link to parent, protected by cgroup_lock() */
>>> -       struct cgroup_root *root;
>>> +       struct cgroup_root __rcu *root;
>>>
>>>           /* idr for css->id */
>>>           struct idr css_idr;
>>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>>> index 227d09704ca59..a718b5f521fb2 100644
>>> --- a/kernel/cgroup/cgroup.c
>>> +++ b/kernel/cgroup/cgroup.c
>>> @@ -1909,7 +1909,7 @@ int rebind_subsystems(struct cgroup_root *dst_root, u32 ss_mask)
>>>                   /* rebind */
>>>                   RCU_INIT_POINTER(scgrp->subsys[ssid], NULL);
>>>                   rcu_assign_pointer(dcgrp->subsys[ssid], css);
>>> -               ss->root = dst_root;
>>> +               rcu_assign_pointer(ss->root, dst_root);
>>>
>>>                   spin_lock_irq(&css_set_lock);
>>>                   css->cgroup = dcgrp;
>>>
>>>
>>> However, if I zoom out, I see that the intention of reading cpuset's
>>> nr_cpus from the scheduler is meant for setups where cpuset tree ~ cpu
>>> tree:
>>>
>>> | * This only really works for cgroup-v2 where all the controllers are mounted
>>> | * in the same hierarchy. If not cgroup-v2 or no cpuset controller is
>>> | * configured it reverts to num_online_cpus().
>>>
>>> Hence it may be just OK to do:
>>>
>>>           int nr = num_online_cpus();
>>>           struct cpuset *cs;
>>>
>>> -       if (is_in_v2_mode()) {
>>> +       if (cpuset_v2()) {
>>>                   guard(rcu)();
>>>                   cs = css_cs(cgroup_e_css(cgrp, &cpuset_cgrp_subsys));
>>>                   if (cs)
>>>
>>> I hope Waiman seconds this -- if a feature depends on shared tree,
>>> there's only so much that 'cpuset_v2_mode' can guarantee.
>> I think it is simpler to just change is_in_v2_mode() to cpuset_v2(). Almost
>> all the cpuset functions should either take the callback_lock with interrupt
>> disabled (which is a RCU read-side critical section) or with rcu_read_lock()
>> and cpuset_mutex() acquired. This cpuset_num_cpus() function is an
>> exception. Given what is said in the comment, this function is not supposed
>> to be used with v1 mounted. We should change it to cpuset_v2().
> The comment says that, outside cgroup v2, cpuset_num_cpus() falls back to
> num_online_cpus(). However, on v1 with cpu and cpuset mounted together using
> cpuset_v2_mode, it returns the group's effective cpuset count and fair.c uses
> that count in the default "concur" group share calculation and in "max" mode.
>
> So replacing is_in_v2_mode() with cpuset_v2() would change scheduler behavior
> for that setup. I guess we could either preserve the current behavior, fix the
> comment and protect the root lookup with RCU; or make the code follow the
> documented v2-only behavior. Which one would you prefer?

I will let Peter decide if he wants to support the cpuset_v2_mode mount 
option of cgroup v1 since he is the original author of 
cpuset_num_cpus(). If this is supported, we have to update the function 
comment as well.

Cheers,
Longman

>
> Thanks,
> -Andrea