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

[Andrea Righi]

Hi Michal,

On Tue, Sep 29, 2026 at 03:47:59PM +0200, 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);

About the __rcu annotations of ss->root, I agree it'd be nice, but it makes
sparse complain about all the other plain ss->root accesses. Maybe we could do
this as a separate cgroup patch/series on top of this one?

Thanks,
-Andrea