[Devel] [PATCH vz10 17/24] ve/cgroup: take cgroup_mutex + threadgroup lock around vz.slice attach

[Pavel Tikhomirov]

This is also fixed by:

ve/cgroup: Grab missing locks in cgroup_(join|leave)_vz_slice

On 7/6/26 12:59, Konstantin Khorenko wrote:
> cgroup_leave_vz_slice() tail-called cgroup_attach_task(cgrp, current, true)
> with no locks at all, and cgroup_join_vz_slice() called it holding only
> cgroup_mutex (via cgroup_kn_lock_live()) but without the threadgroup lock.
> cgroup_attach_task() requires cgroup_mutex AND cgroup_threadgroup_rwsem
> (and, for the ->attach() callbacks, cpus_read_lock()); see its kerneldoc.
> 
> Because the leave path is a tail call, its frame is absent from the
> backtrace, so on a PROVE_LOCKING kernel the splat looked like it came from
> ve_start_container() directly:
> 
>   WARNING: at kernel/cgroup/cgroup.c:535 find_existing_css_set
>     lockdep_assert_held(&cgroup_mutex)
>   cgroup_attach_task <- ve_start_container <- ve_state_write
> 
> and fired ~200 times on every single container start/stop. Beyond the
> annotation, this is a real race on production kernels: find_css_set() /
> cgroup_migrate_prepare_dst() / cgroup_migrate_finish() manipulate the
> per-cset mg_src_cgrp/mg_dst_cgrp and mg_*_preload_node fields that are
> serialized only by cgroup_mutex, so a concurrent migration (any host
> cgroup.procs write, another CT start, or a subtree_control update) can
> corrupt the migration preload lists; the missing threadgroup lock can also
> leave a thread forked mid-migration on the old cset.
> 
> Take cgroup_mutex (leave path) and cgroup_attach_lock(true) (both paths)
> around cgroup_attach_task(), mirroring cgroup_attach_task_all() and
> __cgroup_procs_write(); the attach_lock nests inside cgroup_mutex, and the
> ve->op_sem -> cgroup_mutex order already used by the join path is preserved.
> 
> Fixes: 20a5affcf263 ("ve/cgroup: add vz.slice cgroup to put kernel threads to")
> 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 | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 5585545304a3..4db77090f1be 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2276,7 +2276,9 @@ int cgroup_join_vz_slice(struct ve_struct *ve)
>  		return -ENODEV;
>  	}
>  
> +	cgroup_attach_lock(true);
>  	ret = cgroup_attach_task(cgrp, current, true);
> +	cgroup_attach_unlock(true);
>  	cgroup_kn_unlock(kn);
>  	kernfs_put(kn);
>  	return ret;
> @@ -2286,6 +2288,7 @@ int cgroup_leave_vz_slice(struct ve_struct *ve)
>  {
>  	struct css_set *cset;
>  	struct cgroup *cgrp;
> +	int ret;
>  
>  	cset = rcu_dereference_protected(ve->ve_nsproxy, 1)->cgroup_ns->root_cset;
>  	cgrp = __cset_cgroup_from_root(cset, &cgrp_dfl_root);
> @@ -2294,7 +2297,12 @@ int cgroup_leave_vz_slice(struct ve_struct *ve)
>  	    !test_bit(CGRP_VE_ROOT, &cgrp->flags))
>  		return 0;
>  
> -	return cgroup_attach_task(cgrp, current, true);
> +	cgroup_lock();
> +	cgroup_attach_lock(true);
> +	ret = cgroup_attach_task(cgrp, current, true);
> +	cgroup_attach_unlock(true);
> +	cgroup_unlock();
> +	return ret;
>  }
>  
>  struct cgroup_subsys_state *css_ve_root1(struct cgroup_subsys_state *css)

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