[Devel] [PATCH vz10 19/24] ve/cgroup: don't block container start on unused cgroup1 hierarchies

[Pavel Tikhomirov]

On 7/6/26 12:59, Konstantin Khorenko wrote:
> In cgroup1 every task is an implicit member of the root cgroup of every
> registered hierarchy, so a starting container's root_cset references the
> root cgroups of all cgroup1 hierarchies present on the host - including
> ones the container manager never created a per-container subdirectory in.
> 
> ve_check_root_cgroups() rejects a start if any virtualized cgroup in the
> cset is a host root (no parent), to ensure the container has its own
> isolated subdirectory in each virtualized hierarchy. But a cgroup1
> hierarchy can outlive its umount: cgroup_kill_sb() only kills the root
> reference when the root has no children at umount time, and a child
> removed shortly before the umount lingers on ->self.children until its
> css release finishes (several RCU grace periods later). Nothing retries
> the kill, so
> 
>     mount -t cgroup -o devices xxx /mnt
>     mkdir /mnt/child; rmdir /mnt/child
>     umount /mnt
> 
> leaves the devices hierarchy registered indefinitely. Its root then sits
> in every task's css_set and ve_check_root_cgroups() rejects EVERY
> subsequent container start with -EINVAL (returned by cgroup_mark_ve_roots()
> -> ve_start_container() out of the ve.state write) until the host is
> rebooted. A transient cgroup1 mount+child+umount thus denies container
> start host-wide.
> 
> A cgroup1 hierarchy with no cgroups besides its root is not used by
> anyone: the container is only an involuntary member of its root and there
> is nothing to virtualize in it. Skip such roots in ve_check_root_cgroups()
> and, symmetrically, in the mark/unmark loops so a host root is never
> marked as a VE root. A hierarchy that is actually in use (its root has
> children - a populated named/systemd hierarchy, or any cgroup1 someone
> created a subdirectory in) is still rejected, so the isolation guarantee
> is preserved.
> 
> Fixes: 68dd8db57792 ("ve/cgroup: Skip non-virtualized roots in cgroup_{,un}mark_ve_roots()")
> Feature: ve: ve generic structures
> https://virtuozzo.atlassian.net/browse/VSTOR-137234
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>  kernel/cgroup/cgroup.c | 50 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 4db77090f1be..cb761954f15a 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2150,6 +2150,39 @@ static inline bool is_virtualized_cgroup(struct cgroup *cgrp)
>  	return is_virtualized_cgroot(cgrp->root);
>  }
>  
> +/*
> + * In cgroup1 every task belongs to the root cgroup of every active
> + * hierarchy, so the root_cset of a starting container references root
> + * cgroups of all registered cgroup1 hierarchies, including hierarchies
> + * the container manager knows nothing about and thus could not create
> + * a per-container subdirectory in.
> + *
> + * Moreover, a cgroup1 hierarchy can outlive its umount: cgroup_kill_sb()
> + * does not kill the root if the root cgroup still has children, and a
> + * child removed shortly before the umount lingers on ->self.children
> + * until its css release is finished.  Nothing ever retries killing such
> + * a root, so e.g. "mount -t cgroup -o devices xxx /mnt; mkdir /mnt/x;
> + * rmdir /mnt/x; umount /mnt" leaves the hierarchy registered until the
> + * next mount/umount cycle of it.
> + *
> + * Container start must not be refused because of such hierarchies: if a
> + * cgroup1 hierarchy has no cgroups except the root one, nobody uses it,
> + * the container is only an involuntary member of its root and there is
> + * nothing to virtualize in it.  Skip it both in ve_check_root_cgroups()
> + * and in the mark/unmark loops (a host root cgroup must never be marked
> + * as a VE root).
> + *
> + * The children list is modified under cgroup_mutex, which the callers
> + * may not hold, but a racy empty <-> non-empty transition can only turn
> + * the skip into the -EINVAL check failure or vice versa, both acceptable
> + * outcomes for a hierarchy changing in the middle of container start.
> + */
> +static inline bool ve_unused_cgroup1_root(struct cgroup *cgrp)
> +{
> +	return cgrp->root != &cgrp_dfl_root && !cgroup_parent(cgrp) &&
> +	       list_empty(&cgrp->self.children);

This can be racy, we can see no children in ve_check_root_cgroups, then
see children in cgroup_mark_ve_roots, and then again don't see children
in cgroup_unmark_ve_roots. That would lead first to marking root cgroup
as "ve root" and then to leaking ve reference on unmark.

We don't have cgroup-v1 used, so maybe it does not worth fixing it's paths?

Also this patch seems more of a workaround, should not we instead prevent
stale cgroup-v1 root from being leaked?

AI suggests:

Make a stale cgroup1 root actually die after umount (retry the cgroup_kill_sb()
kill once the lingering child's css release completes), and keep
ve_check_root_cgroups() strict. That kills the host-wide DoS without weakening
the isolation guarantee and without introducing the race.

> +}
> +
>  /*
>   * Iterate all cgroups in a given css_set and for all obligatory Virtuozzo
>   * container cgroups check that container has its own cgroup subdirectory:
> @@ -2165,6 +2198,10 @@ static inline bool ve_check_root_cgroups(struct css_set *cset)
>  		if (!is_virtualized_cgroup(link->cgrp))
>  			continue;
>  
> +		/* Roots of unused (possibly stale) cgroup1 hierarchies */
> +		if (ve_unused_cgroup1_root(link->cgrp))
> +			continue;
> +
>  		/* Host cgroups not allowed */
>  		if (!rcu_access_pointer(link->cgrp->kn->__parent))
>  			return true;
> @@ -2203,6 +2240,10 @@ int cgroup_mark_ve_roots(struct ve_struct *ve)
>  		if (!is_virtualized_cgroup(cgrp))
>  			continue;
>  
> +		/* Skipped by ve_check_root_cgroups(), must not be marked */
> +		if (ve_unused_cgroup1_root(cgrp))
> +			continue;
> +
>  		rcu_assign_pointer(cgrp->ve_owner, ve);
>  		set_bit(CGRP_VE_ROOT, &cgrp->flags);
>  	}
> @@ -2236,6 +2277,15 @@ void cgroup_unmark_ve_roots(struct ve_struct *ve)
>  		if (!is_virtualized_cgroup(cgrp))
>  			continue;
>  
> +		/*
> +		 * Not marked at container start.  Note: even if the root has
> +		 * gained children since then (the helper now returns false),
> +		 * clearing an unmarked root cgroup below is a harmless no-op:
> +		 * root cgroups are never marked as VE roots.
> +		 */
> +		if (ve_unused_cgroup1_root(cgrp))
> +			continue;
> +
>  		rcu_assign_pointer(cgrp->ve_owner, NULL);
>  		clear_bit(CGRP_VE_ROOT, &cgrp->flags);
>  	}

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.