Re: [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup

[Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>]

Hello Tejun

on 2012/12/01 03:27, Tejun Heo wrote:
> Hello, Gao.
> 
> On Sat, Dec 01, 2012 at 12:21:29AM +0800, Gao feng wrote:
>> cgroup_clear_directroy is called by cgroup_d_remove_dir
>> and cgroup_remount.
>>
>> when we call cgroup_remount to remount the cgroup,the subsystem
>> may be unlinked from cgroupfs_root->subsys_list in rebind_subsystem,this
>> subsystem's files will not be removed in cgroup_clear_directroy.
>> And the system will panic when we try to access these files.
>>
>> this patch fix it.
> 
> I'd really appreciated if you explain how it's fixed.
> 
>> Signed-off-by: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
>> ---
>>  kernel/cgroup.c |   86 ++++++++++++++++++++++++++++++++++--------------------
>>  1 files changed, 54 insertions(+), 32 deletions(-)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index e5233b7..90429d5 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -982,10 +982,14 @@ static void cgroup_clear_directory(struct dentry *dir, bool base_files,
>>  				   unsigned long subsys_mask)
>>  {
>>  	struct cgroup *cgrp = __d_cgrp(dir);
>> -	struct cgroup_subsys *ss;
>> +	int i;
>>  
>> -	for_each_subsys(cgrp->root, ss) {
>> +	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>>  		struct cftype_set *set;
>> +		struct cgroup_subsys *ss = subsys[i];
>> +
>> +		if (ss == NULL)
>> +			continue;
> 
> Ummm... I can't say I like it.  It tries to work around the problem
> that the backing subsys may go away earlier by changing how the
> subsystems are iterated so that the "gone" subsystems can be iterated,
> which really doesn't seem like a good diea.  I mean, what if somebody
> tries to access the files inbetween?  root_cgrp->subsys[] would be
> %NULL and segfault all the same.
> 

Yes,You are right.I didn't consider this problem.

>>  static void drop_parsed_module_refcounts(unsigned long subsys_mask)
>> @@ -1392,23 +1403,34 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data)
>>  	removed_mask = root->subsys_mask & ~opts.subsys_mask;
>>  
>>  	/* Don't allow flags or name to change at remount */
>> +	ret = -EINVAL;
>>  	if (opts.flags != root->flags ||
>>  	    (opts.name && strcmp(opts.name, root->name))) {
>> -		ret = -EINVAL;
>>  		drop_parsed_module_refcounts(opts.subsys_mask);
>>  		goto out_unlock;
>>  	}
>>  
>> -	ret = rebind_subsystems(root, opts.subsys_mask);
>> +	/*
>> +	 * Add the reference of these removed subsystem,we
>> +	 * need use subsystems to remove their cgroup files.
>> +	 */
>> +	ret = cgroup_get_module_refcounts(removed_mask);
>>  	if (ret) {
>>  		drop_parsed_module_refcounts(opts.subsys_mask);
>>  		goto out_unlock;
>>  	}
>>  
>> +	ret = rebind_subsystems(root, opts.subsys_mask);
>> +	if (ret) {
>> +		drop_parsed_module_refcounts(opts.subsys_mask | removed_mask);
>> +		goto out_unlock;
>> +	}
>> +
>>  	/* clear out any existing files and repopulate subsystem files */
>>  	cgroup_clear_directory(cgrp->dentry, false, removed_mask);
>>  	/* re-populate subsystem files */
>>  	cgroup_populate_dir(cgrp, false, added_mask);
>> +	drop_parsed_module_refcounts(removed_mask);
> 
> So, how about performing cgroup_clear_directory() *before*
> rebind_subsystems() and then restore it afterwards?
> 

Get it,will fix this problem as you said.

Thanks!