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

[Waiman Long]

On 9/29/26 9:47 AM, Michal Koutný wrote:
> Hi.
>
> On Tue, Sep 29, 2026 at 10:37:38AM +0200, Andrea Righi <arighi@xxxxxxxxxx> 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().

Cheers,
Longman

> 0.02€,
> Michal