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

From: Al Viro <viro@zeniv.linux.org.uk>
To: Andrei Vagin <avagin@gmail.com>
Cc: David Howells <dhowells@redhat.com>,
	linux-fsdevel@vger.kernel.org, cgroups@vger.kernel.org,
	Li Zefan <lizefan@huawei.com>
Subject: Re: [PATCH vfs/for-next v2] cgroup: fix top cgroup refcnt leak
Date: Wed, 2 Jan 2019 02:28:04 +0000	[thread overview]
Message-ID: <20190102022804.GH2217@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20181229000400.26333-1-avagin@gmail.com>

On Fri, Dec 28, 2018 at 04:04:00PM -0800, Andrei Vagin 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
> 
>  kernel/cgroup/cgroup.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index fb0717696895..f63974a3725f 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2047,6 +2047,9 @@ int cgroup_do_get_tree(struct fs_context *fc)
>  	ret = 0;
>  	if (ctx->kfc.new_sb_created)
>  		goto out_cgrp;
> +	else
> +		cgroup_put(&ctx->root->cgrp);
> +
>  	apply_cgroup_root_flags(ctx->flags);
>  	return 0;

That looks horrible, especially since out_cgrp is return ret;
If anything, it should be
	if (!ctx->kfc.new_sb_created) {
		cgroup_put(&ctx->root->cgrp);
		apply_cgroup_root_flags(ctx->flags);
	}
	return 0;

What I don't understand is why apply_cgroup_root_flags() is not
called in "new superblock" case here.  It used to, prior to that
conversion...

Another fishy place I see there is
                nsdentry = kernfs_node_dentry(cgrp->kn, fc->root->d_sb);
                if (IS_ERR(nsdentry))
                        return PTR_ERR(nsdentry);
                dput(fc->root);
                fc->root = nsdentry;
What happens if we get here with non-NULL fc->root (and we'd better,
after successful from kernfs_get_tree() a bit earlier) and hit that
failure exit?  A leak?

With apologies for being MIA for a week - it had been insane here...