Re: [PATCH 3/7] cgroup: introduce cgroup namespaces

Tejun Heo
2015-12-08T16:04:57Z

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