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

[Valeriy Vdovin]

On 28.10.2020 13:18, Kirill Tkhai wrote:
> 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().

You are right. Thank you for pointing that out. More work should be done 
to ensure that cgroup_root_mutex could provide the right guarantees. 
While answering your questions I've found a better solution I think. I 
will revoke this whole patch and send a new one.

>   
>> 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;
>>   }
>>