[Devel] [PATCH RH7] cgroup: do not use cgroup_mutex in cgroup_show_options

[Kirill Tkhai]

On 27.10.2020 17:59, Valeriy Vdovin wrote:
> In patch 87cb5fdb5b5c77ac617b46a0fe118a7d50a77b1c function
> cgroup_show_options started to lock cgroup_mutex, which introduced
> new deadlock possibility, described below:
> 
> Thread A:
>   m_start() --> down_read(&namespace_sem);
>     cgroup_show_options() --> mutex_lock(&cgroup_mutex);
> 
> Thread B:
> attach_task_by_pid()
>   cgroup_lock_live_group -->  mutex_lock(&cgroup_mutex);
>   threadgroup_lock() -->  down_write(&tsk->signal->group_rwsem);
> 
> Thread C:
>  copy_process
>   threadgroup_change_begin() --> down_read(&tsk->signal->group_rwsem);
>   copy_namespaces
>    create_new_namespaces
>      copy_mnt_ns
>       namespace_lock() --> down_write(&namespace_sem)
> 
> Clearly cgroup_mutex can not be locked right after locking
> namespace_sem, because opposite locking order is also present in
> the code. To get rid of cgroup_mutex it's synchonization role should
> transition to cgroup_root_mutex, that was created specifically to defeat
> the described locking order problem.
> 
> In this location cgroup_mutex provided 2 guarantees:
>   1. Safe call for 'task_cgroup_from_root', that asserts cgroup_mutex
>      ownership.
>   2. Safe read access to cgroup->ve_owner
> 
> These guarantees are now transferred to cgroup_root_mutex in the
> following way:
>   1. task_cgroup_from_root assertion is modified to take into account
>      cgroup_root_mutex as one of two possible locks.

I don't see an explanation of cgroup_root_mutex is acceptable to call task_cgroup_from_root().
This check is about "you can use result of its function under that lock". cgroup_mutex guarantees
the result is stable under cgroup_mutex, you should prove the same for cgroup_root_mutex.

task_cgroup_from_root() is similar to css_cgroup_from_root(), but the second function
does not accept cgroup_root_mutex. What is the difference?

>   2. cgroup->ve_owner field modifications are done with
>      cgroup_root_mutex also locked.

Which way? There is no cgroup_root_mutex in cgroup_unmark_ve_roots().
 
> https://jira.sw.ru/browse/PSBM-121438
> 
> Signed-Off-By: Valeriy Vdovin <valeriy.vdovin at virtuozzo.com>
> ---
>  kernel/cgroup.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 27d7a5e..db6a5ba 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -669,7 +669,7 @@ static struct cgroup *task_cgroup_from_root(struct task_struct *task,
>  	struct css_set *css;
>  	struct cgroup *res = NULL;
>  
> -	BUG_ON(!mutex_is_locked(&cgroup_mutex));
> +	BUG_ON(!mutex_is_locked(&cgroup_mutex) && !mutex_is_locked(&cgroup_root_mutex));
>  	read_lock(&css_set_lock);
>  	/*
>  	 * No need to lock the task - since we hold cgroup_mutex the
> @@ -1100,7 +1100,6 @@ static int cgroup_show_options(struct seq_file *seq, struct dentry *dentry)
>  	struct cgroup_subsys *ss;
>  	struct cgroup *root_cgrp = &root->top_cgroup;
>  
> -	mutex_lock(&cgroup_mutex);
>  	mutex_lock(&cgroup_root_mutex);
>  	for_each_subsys(root, ss)
>  		seq_printf(seq, ",%s", ss->name);
> @@ -1142,7 +1141,6 @@ static int cgroup_show_options(struct seq_file *seq, struct dentry *dentry)
>  	if (strlen(root->name))
>  		seq_show_option(seq, "name", root->name);
>  	mutex_unlock(&cgroup_root_mutex);
> -	mutex_unlock(&cgroup_mutex);
>  	return 0;
>  }
>  
> @@ -2529,6 +2527,8 @@ static int cgroup_release_agent_write(struct cgroup *cgrp, struct cftype *cft,
>  	if (!cgroup_lock_live_group(cgrp))
>  		return -ENODEV;
>  
> +	mutex_lock(&cgroup_root_mutex);
> +
>  	/*
>  	 * Call to cgroup_get_local_root is a way to make sure
>  	 * that cgrp in the argument is valid "virtual root"
> @@ -2551,6 +2551,7 @@ static int cgroup_release_agent_write(struct cgroup *cgrp, struct cftype *cft,
>  		ret = -ENODEV;
>  
>  out:
> +	mutex_unlock(&cgroup_root_mutex);
>  	mutex_unlock(&cgroup_mutex);
>  	return ret;
>  }
>