From: "Serge E. Hallyn" <serge.hallyn@ubuntu.com>
To: Tejun Heo <tj@kernel.org>
Cc: serge.hallyn@ubuntu.com, linux-api@vger.kernel.org,
containers@lists.linux-foundation.org, hannes@cmpxchg.org,
linux-kernel@vger.kernel.org, ebiederm@xmission.com,
lxc-devel@lists.linuxcontainers.org, gregkh@linuxfoundation.org,
cgroups@vger.kernel.org, akpm@linux-foundation.org
Subject: Re: [PATCH 5/7] cgroup: mount cgroupns-root when inside non-init cgroupns
Date: Tue, 8 Dec 2015 10:48:31 -0600 [thread overview]
Message-ID: <20151208164831.GB13616@mail.hallyn.com> (raw)
In-Reply-To: <20151208162040.GC30240@mtj.duckdns.org>
On Tue, Dec 08, 2015 at 11:20:40AM -0500, Tejun Heo wrote:
> Hello,
>
> On Mon, Dec 07, 2015 at 05:06:20PM -0600, serge.hallyn@ubuntu.com wrote:
> > fs/kernfs/mount.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/kernfs.h | 2 ++
> > kernel/cgroup.c | 39 ++++++++++++++++++++++++-
> > 3 files changed, 114 insertions(+), 1 deletion(-)
>
> Please put kernfs changes in a spearate patch.
>
> > diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
> > index 8eaf417..9219444 100644
> > --- a/fs/kernfs/mount.c
> > +++ b/fs/kernfs/mount.c
> > @@ -62,6 +63,79 @@ struct kernfs_root *kernfs_root_from_sb(struct super_block *sb)
> > return NULL;
> > }
> >
> > +/*
> > + * find the next ancestor in the path down to @child, where @parent was the
> > + * parent whose child we want to find.
>
> s/parent/ancestor/ s/child/descendant/ ?
>
> > + *
> > + * Say the path is /a/b/c/d. @child is d, @parent is NULL. We return the root
> > + * node. If @parent is b, then we return the node for c.
> > + * Passing in d as @parent is not ok.
> > + */
> > +static struct kernfs_node *
> > +find_next_ancestor(struct kernfs_node *child, struct kernfs_node *parent)
> > +{
> > + if (child == parent) {
> > + pr_crit_once("BUG in find_next_ancestor: called with parent == child");
> > + return NULL;
> > + }
> > +
> > + while (child->parent != parent) {
> > + if (!child->parent)
> > + return NULL;
> > + child = child->parent;
> > + }
> > +
> > + return child;
> > +}
> > +
> > +/**
> > + * kernfs_obtain_root - get a dentry for the given kernfs_node
> > + * @sb: the kernfs super_block
> > + * @kn: kernfs_node for which a dentry is needed
> > + *
> > + * This can be used by callers which want to mount only a part of the kernfs
> > + * as root of the filesystem.
> > + */
> > +struct dentry *kernfs_obtain_root(struct super_block *sb,
> > + struct kernfs_node *kn)
>
> Wouldn't @kn, @sb be a better order? Also, kernfs super_blocks are
> determined by the kernfs_root and its namespace. I wonder whether
> specifying @ns would be better.
>
> > +{
> > + struct dentry *dentry;
> > + struct kernfs_node *knparent = NULL;
> > +
> > + BUG_ON(sb->s_op != &kernfs_sops);
> > +
> > + dentry = dget(sb->s_root);
> > + if (!kn->parent) // this is the root
> ^^^
> Do we do this now?
>
> > + return dentry;
> > +
> > + knparent = find_next_ancestor(kn, NULL);
> > + if (!knparent) {
> > + pr_crit("BUG: find_next_ancestor for root dentry returned NULL\n");
>
> Wouldn't stack dump helpful here? Why not
Hm, yeah, that's a good reason to use WARN - thanks.
> if (WARN_ONCE(!knparent, "find_next..."))
> return ERR_PTR(-EINVAL);
>
> or even just WARN_ON_ONCE().
>
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + do {
> > + struct dentry *dtmp;
> > + struct kernfs_node *kntmp;
> > +
> > + if (kn == knparent)
> > + return dentry;
> > + kntmp = find_next_ancestor(kn, knparent);
> > + if (!kntmp) {
> > + pr_crit("BUG: find_next_ancestor returned NULL for node\n");
>
> Ditto. It'd be a kernel bug. WARN is usually the better way.
>
> > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > index a5ab74d..09cd718 100644
> > --- a/kernel/cgroup.c
> > +++ b/kernel/cgroup.c
> > @@ -2011,6 +2011,15 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
> > int ret;
> > int i;
> > bool new_sb;
> > + struct cgroup_namespace *ns = current->nsproxy->cgroup_ns;
>
> Please move this upwards so that it's below other initialized
> variables.
>
> > +
> > + get_cgroup_ns(ns);
> > +
> > + /* Check if the caller has permission to mount. */
> > + if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN)) {
> > + put_cgroup_ns(ns);
> > + return ERR_PTR(-EPERM);
> > + }
> >
> > /*
> > * The first time anyone tries to mount a cgroup, enable the list
> > @@ -2127,6 +2136,11 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
> > goto out_unlock;
> > }
> >
> > + if (!opts.none && !capable(CAP_SYS_ADMIN)) {
> > + ret = -EPERM;
> > + goto out_unlock;
> > + }
>
> Hmmm... why is !opts.none necessary? Please add a comment explaining
> why the above is necessary.
>
> > out_mount:
> > dentry = kernfs_mount(fs_type, flags, root->kf_root,
> > is_v2 ? CGROUP2_SUPER_MAGIC : CGROUP_SUPER_MAGIC,
> > &new_sb);
> > +
> > + if (!IS_ERR(dentry)) {
> > + /*
> > + * In non-init cgroup namespace, instead of root cgroup's
> > + * dentry, we return the dentry corresponding to the
> > + * cgroupns->root_cgrp.
> > + */
> > + if (ns != &init_cgroup_ns) {
>
> if (!IS_ERR(dentry) && ns != &init_cgroup_ns) {
>
> > + struct dentry *nsdentry;
> > + struct cgroup *cgrp;
> > +
> > + cgrp = cset_cgroup_from_root(ns->root_cset, root);
> > + nsdentry = kernfs_obtain_root(dentry->d_sb,
> > + cgrp->kn);
>
> Heh, is kernfs_obtain_root() the right name? Maybe
> kernfs_node_to_inode()?
>
> Thanks.
>
> --
> tejun
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
next prev parent reply other threads:[~2015-12-08 16:48 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-07 23:06 CGroup Namespaces (v6) serge.hallyn
2015-12-07 23:06 ` [PATCH 1/7] kernfs: Add API to generate relative kernfs path serge.hallyn
2015-12-08 11:51 ` Greg KH
2015-12-09 1:17 ` Serge E. Hallyn
2015-12-08 15:52 ` Tejun Heo
2015-12-08 16:46 ` Serge E. Hallyn
2015-12-08 18:45 ` Serge E. Hallyn
2015-12-07 23:06 ` [PATCH 2/7] sched: new clone flag CLONE_NEWCGROUP for cgroup namespace serge.hallyn
2015-12-07 23:06 ` [PATCH 3/7] cgroup: introduce cgroup namespaces serge.hallyn
2015-12-08 16:04 ` Tejun Heo
2015-12-08 19:34 ` Serge E. Hallyn
2015-12-08 19:46 ` Tejun Heo
2015-12-08 19:47 ` Serge E. Hallyn
2015-12-07 23:06 ` [PATCH 4/7] cgroup: cgroup namespace setns support serge.hallyn
2015-12-07 23:06 ` [PATCH 5/7] cgroup: mount cgroupns-root when inside non-init cgroupns serge.hallyn
2015-12-08 16:20 ` Tejun Heo
2015-12-08 16:48 ` Serge E. Hallyn [this message]
2015-12-08 23:21 ` Serge E. Hallyn
2015-12-09 15:48 ` Tejun Heo
2015-12-07 23:06 ` [PATCH 6/7] cgroup: Add documentation for cgroup namespaces serge.hallyn
2015-12-08 16:22 ` Tejun Heo
2015-12-07 23:06 ` [PATCH 7/7] Add FS_USERNS_FLAG to cgroup fs serge.hallyn
2015-12-08 10:10 ` CGroup Namespaces (v6) Alban Crequy
2015-12-08 15:22 ` Serge E. Hallyn
-- strict thread matches above, loose matches on Subject: below --
2015-11-27 20:52 CGroup Namespaces (v5) serge.hallyn
2015-11-27 20:52 ` [PATCH 5/7] cgroup: mount cgroupns-root when inside non-init cgroupns serge.hallyn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151208164831.GB13616@mail.hallyn.com \
--to=serge.hallyn@ubuntu.com \
--cc=akpm@linux-foundation.org \
--cc=cgroups@vger.kernel.org \
--cc=containers@lists.linux-foundation.org \
--cc=ebiederm@xmission.com \
--cc=gregkh@linuxfoundation.org \
--cc=hannes@cmpxchg.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lxc-devel@lists.linuxcontainers.org \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).