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

[Andrea Righi <arighi@nvidia.com>]

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?

Thanks,
-Andrea