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@xxxxxxxxxx>
> Fixes: aea3f2676c83 ("kernfs, sysfs, cgroup, intel_rdt: Support fs_context")
> Signed-off-by: Andrei Vagin <avagin@xxxxxxxxx>
> ---
>
> 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...
