Date: Tue, 8 Dec 2015 11:04:53 -0500
From: Tejun Heo <tj@kernel.org>
To: serge.hallyn@ubuntu.com
Cc: linux-kernel@vger.kernel.org, adityakali@google.com,
        linux-api@vger.kernel.org, containers@lists.linux-foundation.org,
        cgroups@vger.kernel.org, lxc-devel@lists.linuxcontainers.org,
        akpm@linux-foundation.org, ebiederm@xmission.com,
        gregkh@linuxfoundation.org, lizefan@huawei.com, hannes@cmpxchg.org
Subject: Re: [PATCH 3/7] cgroup: introduce cgroup namespaces
Message-ID: <20151208160453.GB30240@mtj.duckdns.org>
References: <1449529582-4075-1-git-send-email-serge.hallyn@ubuntu.com>
 <1449529582-4075-4-git-send-email-serge.hallyn@ubuntu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1449529582-4075-4-git-send-email-serge.hallyn@ubuntu.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3645
Lines: 128

On Mon, Dec 07, 2015 at 05:06:18PM -0600, serge.hallyn@ubuntu.com wrote:
>  static const char *proc_ns_follow_link(struct dentry *dentry, void **cookie)
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 2b3e2314..906f240 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -17,11 +17,36 @@
>  #include <linux/seq_file.h>
>  #include <linux/kernfs.h>
>  #include <linux/jump_label.h>
> +#include <linux/nsproxy.h>
> +#include <linux/types.h>
> +#include <linux/ns_common.h>
> +#include <linux/nsproxy.h>
> +#include <linux/user_namespace.h>
>  
>  #include <linux/cgroup-defs.h>
>  
> +struct cgroup_namespace {
> +	atomic_t		count;
> +	struct ns_common	ns;
> +	struct user_namespace	*user_ns;
> +	struct css_set          *root_cset;
> +};
> +
> +extern struct cgroup_namespace init_cgroup_ns;
> +
> +static inline void get_cgroup_ns(struct cgroup_namespace *ns)
> +{
> +	if (ns)
> +		atomic_inc(&ns->count);
> +}
> +
..
> +void free_cgroup_ns(struct cgroup_namespace *ns);
> +struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
> +		struct user_namespace *user_ns,
> +		struct cgroup_namespace *old_ns);
...
> +char * __must_check cgroup_path_ns(struct cgroup *cgrp, char *buf,
> +				   size_t buflen, struct cgroup_namespace *ns);
> +char * __must_check cgroup_path(struct cgroup *cgrp, char *buf, size_t buflen);
...
> +static inline void put_cgroup_ns(struct cgroup_namespace *ns)
> +{
> +	if (ns && atomic_dec_and_test(&ns->count))
> +		free_cgroup_ns(ns);
> +}

I'd prefer collecting all ns related declarations in a single place.

> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 7f2f007..4fd07b5a 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -297,6 +300,13 @@ static bool cgroup_on_dfl(const struct cgroup *cgrp)
>  {
>  	return cgrp->root == &cgrp_dfl_root;
>  }

New line.

> +struct cgroup_namespace init_cgroup_ns = {
> +	.count		= { .counter = 1, },
> +	.user_ns	= &init_user_ns,
> +	.ns.ops		= &cgroupns_operations,
> +	.ns.inum	= PROC_CGROUP_INIT_INO,
> +	.root_cset	= &init_css_set,
> +};

Also, why inbetween two function defs?  cgroup.c is messy but you can
still put it after other variable definitions.

> @@ -430,18 +440,18 @@ static inline bool cgroup_is_dead(const struct cgroup *cgrp)
>  	return !(cgrp->self.flags & CSS_ONLINE);
>  }
>  
> -static void cgroup_get(struct cgroup *cgrp)
> +static inline void cgroup_get(struct cgroup *cgrp)
>  {
>  	WARN_ON_ONCE(cgroup_is_dead(cgrp));
>  	css_get(&cgrp->self);
>  }
>  
> -static bool cgroup_tryget(struct cgroup *cgrp)
> +static inline bool cgroup_tryget(struct cgroup *cgrp)
>  {
>  	return css_tryget(&cgrp->self);
>  }
>  
> -static void cgroup_put(struct cgroup *cgrp)
> +static inline void cgroup_put(struct cgroup *cgrp)
>  {
>  	css_put(&cgrp->self);
>  }
>
> @@ -780,7 +790,7 @@ static void put_css_set_locked(struct css_set *cset)
>  	kfree_rcu(cset, rcu_head);
>  }
>  
> -static void put_css_set(struct css_set *cset)
> +static inline void put_css_set(struct css_set *cset)

Stray diffs?

> @@ -2189,6 +2199,25 @@ static struct file_system_type cgroup2_fs_type = {
>  	.kill_sb = cgroup_kill_sb,
>  };
>  
> +char * __must_check
> +cgroup_path_ns(struct cgroup *cgrp, char *buf, size_t buflen,
> +	       struct cgroup_namespace *ns)
> +{
> +	struct cgroup *root;
> +
> +	BUG_ON(!ns);

Just let it crash on NULL deref.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/