Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Alexander Nikiforov <a.nikiforov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]

Hi

sorry for noise, but I have a question


     mode = cgroup_file_mode(cft);
     error = cgroup_create_file(dentry, mode | S_IFREG, cgrp->root->sb);
     if (!error) {
         cfe->type = (void *)cft;
         cfe->dentry = dentry;
         dentry->d_fsdata = cfe;
         list_add_tail(&cfe->node, &parent->files);
         cfe = NULL;
     }
     dput(dentry);
out:
     kfree(cfe);
     return error;

this is from for-3.5 version. dput executes if cgroup_create_file fails 
and done correct. but in cgroup_create_file dget calls just before 
return, so if cgroup_create_file we call dput without dget. is this 
correct behavioural??


On 05/04/2012 08:54 PM, Tejun Heo wrote:
> Hello,
>
> On Fri, May 04, 2012 at 04:55:44PM +0400, Alexander Nikiforov wrote:
>>>> -static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>> +static struct cfent* cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>>   			   const struct cftype *cft)
>>>>   {
>>>>   	struct dentry *dir = cgrp->dentry;
>>>> @@ -2696,12 +2717,13 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>>
>>>>   	cfe = kzalloc(sizeof(*cfe), GFP_KERNEL);
>>>>   	if (!cfe)
>>>> -		return -ENOMEM;
>>>> +		return NULL;
>>>>
>>>>   	dentry = lookup_one_len(name, dir, strlen(name));
>>>>   	if (IS_ERR(dentry)) {
>>>>   		error = PTR_ERR(dentry);
>>>> -		goto out;
>>>> +		kfree(cfe);
>>>> +		return NULL;
>>>>   	}
>>>>
>>>>   	mode = cgroup_file_mode(cft);
>>>> @@ -2711,29 +2733,34 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>>   		cfe->dentry = dentry;
>>>>   		dentry->d_fsdata = cfe;
>>>>   		list_add_tail(&cfe->node,&parent->files);
>>>> -		cfe = NULL;
>>>>   	}
>>>>   	dput(dentry);
>>>> -out:
>>>> -	kfree(cfe);
>>>> -	return error;
>>>> +
>>>> +	return cfe;
>>>>   }
>>> Are you sure you're not creating a memory leak here?
>> code make the same things, just without goto. move kfree inside if
>> (IS_ERR(dentry), but before dput(dentry)  cfe set to NULL, so
>> kfree(cfe) will do nothing. I think it's without any mem leaks.
> I'm confused.  Who's freeing cfe after cgroup_create_file() failure?
> Also, why are you changing this at all?
>
> Thanks.
>


-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics