Re: [PATCH v5 3/3] cgroups: make procs file writable

[Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>]

David Rientjes wrote:
> On Thu, 30 Dec 2010, Li Zefan wrote:
> 
>>> This needs to be done with cgroup_lock() instead of callback_mutex since 
>>> the post_clone() callback will store to cs->mems_allowed on 
>>> cgroup_clone().
>>>
>> Then cpuset_post_clone() breaks the lock rule:
>>
>>  * A task must hold both mutexes to modify cpusets...
>>  ...
>>  * If a task is only holding callback_mutex, then it has read-only
>>  * access to cpusets.
>>
>> But that's Ok, because cgroup_clone() is called during the creation of
>> the new cgroup, so no one can access the cpuset at that time.
>>
> 
> I'm saying that if cpusets implements a cgroup_clone() handler that the 
> locking will break with only callback_mutex here because the only 
> synchronization after the new cgroup dentry is added is cgroup_lock() that 
> is always held when a post_clone callback is invoked and reading a mems 
> file may race since it is accessible before the task is attached in the 
> cgroup_clone() case.  It's not a problem right now but may subtly break if 
> cpusets were to use cgroup_clone().
> 

cgroup_clone() was implemented for ns_cgroup, and ns_cgroup is scheduled to be
removed in the coming 2.6.38, and so will be cgroup_clone().

As a replacement we've added cgroup.clone_children, and the post_clone() callback
will be called in cgroup_create() if the clone_children flag is set.

If we want to avoid subtle break in case post_clone() is used in other cases
than cgroup creation in the future, we can just hold callback_mutex() in
cpuset_post_clone().