Re: [PATCH vfs/for-next v4] cgroup: fix top cgroup refcnt leak

[Andrei Vagin <avagin@gmail.com>]

On Wed, Jan 02, 2019 at 04:43:39PM -0800, Andrei Vagin wrote:
> On Thu, Jan 03, 2019 at 12:26:23AM +0000, David Howells wrote:
> > Andrei Vagin <avagin@gmail.com> wrote:
> > 
> > > It looks like the c6b3d5bcd67c ("cgroup: fix top cgroup refcnt leak")
> > > commit was reverted by mistake.
> > > 
> > > $ mkdir /tmp/cgroup
> > > $ mkdir /tmp/cgroup2
> > > $ mount -t cgroup -o none,name=test test /tmp/cgroup
> > > $ mount -t cgroup -o none,name=test test /tmp/cgroup2
> > > $ umount /tmp/cgroup
> > > $ umount /tmp/cgroup2
> > > $ cat /proc/self/cgroup | grep test
> > > 12:name=test:/
> > > 
> > > You can see the test cgroup was not freed.
> > > 
> > > Cc: Li Zefan <lizefan@huawei.com>
> > > Fixes: aea3f2676c83 ("kernfs, sysfs, cgroup, intel_rdt: Support fs_context")
> > > Signed-off-by: Andrei Vagin <avagin@gmail.com>
> > > ---
> > > 
> > > v2: clean up code and add the vfs/for-next tag
> > > v3: fix a reference leak when kernfs_node_dentry fails
> > > v4: call deactivate_locked_super() in a error case
> > > v5: don't dereference fc->root after dput()
> > > 
> > >  kernel/cgroup/cgroup.c | 25 ++++++++++++++++++-------
> > >  1 file changed, 18 insertions(+), 7 deletions(-)
> > 
> > This patch doesn't work either.
> 
> I'm sorry, but we can't say anything about this patch now, because it
> looks like recent changes in vfs-next break something else here...

I found a reason why this patch doesn't work on Al's vfs/for-next:

[avagin@laptop linux]$ git diff 40effd960becd8a355b7aafc789712afd64f5759..vfs/for-next  kernel/cgroup/cgroup-v1.c | grep -B 5 -A 5 cgroup_get 
 	/*
@@ -1280,8 +1285,8 @@ int cgroup1_get_tree(struct fs_context *fc)
 		mutex_lock(&cgroup_mutex);
 		percpu_ref_reinit(&root->cgrp.self.refcnt);
 		mutex_unlock(&cgroup_mutex);
+		cgroup_get(&root->cgrp);
 	}
-	cgroup_get(&root->cgrp);
 
 	/*
 	 * If @pinned_sb, we're reusing an existing root and holding an

40effd960becd8a355b7aafc789712afd64f5759 is the previous head of vfs/for-next

I reverted this hunk, applied my patch and all criu test passed.

> 
> > 
> > 	percpu ref (css_release) <= 0 (0) after switching to atomic
> > 	RIP: 0010:percpu_ref_switch_to_atomic_rcu+0x90/0x1a0
> > 
> > Btw, note that the subject says "v4" but the changelog says "v5".
> 
> It is v5.
> 
> > 
> > David