| Message ID | 20180621184754.GB21326@outlook.office365.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Andrei Vagin <avagin@virtuozzo.com> wrote: > ret = 0; > + ctx->root = root; > goto out_unlock; Okay, I can see that. > percpu_ref_reinit(&root->cgrp.self.refcnt); > mutex_unlock(&cgroup_mutex); > } > + cgroup_get(&root->cgrp); This probably needs to be conditional on ret == 0. Which version are you testing btw? The patches in git have been fixed a little from what was last posted. David
On Fri, Jun 22, 2018 at 01:52:16PM +0100, David Howells wrote: > Andrei Vagin <avagin@virtuozzo.com> wrote: > > > ret = 0; > > + ctx->root = root; > > goto out_unlock; > > Okay, I can see that. > > > percpu_ref_reinit(&root->cgrp.self.refcnt); > > mutex_unlock(&cgroup_mutex); > > } > > + cgroup_get(&root->cgrp); > > This probably needs to be conditional on ret == 0. yes, you are right > > Which version are you testing btw? The patches in git have been fixed a > little from what was last posted. I'm testing linux-next-20180621 commit 8439c34f07a3f58245e933ca2703239417288363 (tag: next-20180621, linux-next/master) Author: Stephen Rothwell <sfr@canb.auug.org.au> Date: Thu Jun 21 14:09:41 2018 +1000 Add linux-next specific files for 20180621 Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au> > > David
On Fri, Jun 22, 2018 at 08:30:29AM -0700, Andrei Vagin wrote: > On Fri, Jun 22, 2018 at 01:52:16PM +0100, David Howells wrote: > > Andrei Vagin <avagin@virtuozzo.com> wrote: > > > > > ret = 0; > > > + ctx->root = root; > > > goto out_unlock; > > > > Okay, I can see that. > > > > > percpu_ref_reinit(&root->cgrp.self.refcnt); > > > mutex_unlock(&cgroup_mutex); > > > } > > > + cgroup_get(&root->cgrp); > > > > This probably needs to be conditional on ret == 0. > > yes, you are right I've read the code and I think it isn't obvious. A reference will be released id cgroup_fs_context_free() even if ret isn't zero here. I look at do_new_mount() vfs_new_fs_context() ... if (vfs_get_tree()) goto out_fc; .... out_fc: put_fs_context(fc); fc->ops->free(fc); cgroup_fs_context_free() cgroup_put(&ctx->root->cgrp); > > > > > Which version are you testing btw? The patches in git have been fixed a > > little from what was last posted. > > I'm testing linux-next-20180621 > > commit 8439c34f07a3f58245e933ca2703239417288363 (tag: next-20180621, > linux-next/master) > Author: Stephen Rothwell <sfr@canb.auug.org.au> > Date: Thu Jun 21 14:09:41 2018 +1000 > > Add linux-next specific files for 20180621 > > Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au> > > > > > David
Andrei Vagin <avagin@virtuozzo.com> wrote: > > > > percpu_ref_reinit(&root->cgrp.self.refcnt); > > > > mutex_unlock(&cgroup_mutex); > > > > } > > > > + cgroup_get(&root->cgrp); > > > > > > This probably needs to be conditional on ret == 0. > > > > yes, you are right > > > I've read the code and I think it isn't obvious. A reference will be > released id cgroup_fs_context_free() even if ret isn't zero here. > > I look at do_new_mount() > > vfs_new_fs_context() > ... > if (vfs_get_tree()) > goto out_fc; > .... > out_fc: > put_fs_context(fc); > fc->ops->free(fc); > cgroup_fs_context_free() > cgroup_put(&ctx->root->cgrp); Yeah, you're right: ctx->root is set above, so the put will trigger anyway. I'll fold both of these changes in. David
diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c index e12c0a91b8a4..b1340bd5f5fc 100644 --- a/kernel/cgroup/cgroup-v1.c +++ b/kernel/cgroup/cgroup-v1.c @@ -1192,6 +1192,7 @@ int cgroup1_get_tree(struct fs_context *fc) } ret = 0; + ctx->root = root; goto out_unlock; } @@ -1241,6 +1242,7 @@ int cgroup1_get_tree(struct fs_context *fc) percpu_ref_reinit(&root->cgrp.self.refcnt); mutex_unlock(&cgroup_mutex); } + cgroup_get(&root->cgrp); /* * If @pinned_sb, we're reusing an existing root and holding an