Re: [PATCH 5/8] cgroup: introduce cgroup namespaces

[Tejun Heo]

Hello,

On Mon, Nov 16, 2015 at 01:51:42PM -0600, serge@xxxxxxxxxx wrote:
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 99096be..b3ce9d9 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -17,6 +17,9 @@
>  #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/cgroup-defs.h>
>  
> @@ -237,6 +240,10 @@ static inline bool cgroup_is_dead(const struct cgroup *cgrp)
>  	return !(cgrp->self.flags & CSS_ONLINE);
>  }
>  
> +static inline void css_get(struct cgroup_subsys_state *css);
> +static inline void css_put(struct cgroup_subsys_state *css);
> +static inline bool css_tryget(struct cgroup_subsys_state *css);

Heh, what's going on here?

> +
>  static inline void cgroup_get(struct cgroup *cgrp)
>  {
>  	WARN_ON_ONCE(cgroup_is_dead(cgrp));
> @@ -284,9 +291,11 @@ static inline void cgroup_put(struct cgroup *cgrp)
>  			;						\
>  		else
>  
> -/*
> - * Inline functions.
> - */
> +extern char * __must_check cgroup_path_ns(struct cgroup_namespace *ns,
> +		struct cgroup *cgrp, char *buf, size_t buflen);
> +
> +extern char * __must_check cgroup_path(struct cgroup *cgrp, char *buf,
> +		size_t buflen);

Please move them next to other prototypes and drop extern.

> diff --git a/include/linux/cgroup_namespace.h b/include/linux/cgroup_namespace.h
> new file mode 100644
> index 0000000..ed181c3
> --- /dev/null
> +++ b/include/linux/cgroup_namespace.h
> @@ -0,0 +1,46 @@
> +#ifndef _LINUX_CGROUP_NAMESPACE_H
> +#define _LINUX_CGROUP_NAMESPACE_H
> +
> +#include <linux/nsproxy.h>
> +#include <linux/cgroup.h>
> +#include <linux/types.h>
> +#include <linux/user_namespace.h>
> +
> +struct css_set;

Blank line here or linux/cgroup-defs.h can be included.

> +struct cgroup_namespace {
> +	atomic_t		count;
> +	struct ns_common	ns;
> +	struct user_namespace	*user_ns;
> +	struct css_set          *root_cgrps;
> +};
> +
> +extern struct cgroup_namespace init_cgroup_ns;
> +
> +static inline struct cgroup_namespace *get_cgroup_ns(
> +		struct cgroup_namespace *ns)

I personally prefer putting just the return type on a separate line
when things get too long.

static inline struct cgroup_namespace *
get_cgroup_ns(struct cgroup_namespace *ns)

> +{
> +	if (ns)
> +		atomic_inc(&ns->count);
> +	return ns;
> +}

Ugh... if the function doesn't do anything about the return type,
please make it a void function.  We tried the above style with kobj
and driver model and it ended up pretty horrible.

> +#ifdef CONFIG_CGROUPS
> +extern void free_cgroup_ns(struct cgroup_namespace *ns);
> +extern struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
> +					       struct user_namespace *user_ns,
> +					       struct cgroup_namespace *old_ns);

Please drop extern.

> +#else /* CONFIG_CGROUP */
> +static inline void free_cgroup_ns(struct cgroup_namespace *ns) { }
> +static inline struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
> +					struct user_namespace *user_ns,
> +					struct cgroup_namespace *old_ns)
> +{ return old_ns; }
> +#endif
> +
> +static inline void put_cgroup_ns(struct cgroup_namespace *ns)
> +{
> +	if (ns && atomic_dec_and_test(&ns->count))
> +		free_cgroup_ns(ns);
> +}
> +
> +#endif  /* _LINUX_CGROUP_NAMESPACE_H */

I don't know.  Does this warrant a separate file?

> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index e972259..1d696de 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -57,6 +57,8 @@
>  #include <linux/vmalloc.h> /* TODO: replace with more sophisticated array */
>  #include <linux/kthread.h>
>  #include <linux/delay.h>
> +#include <linux/proc_ns.h>
> +#include <linux/cgroup_namespace.h>
>  
>  #include <linux/atomic.h>
>  
> @@ -290,6 +292,15 @@ static bool cgroup_on_dfl(const struct cgroup *cgrp)
>  {
>  	return cgrp->root == &cgrp_dfl_root;
>  }
> +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_cgrps = &init_css_set,
> +};

Can you please tab align the assignments?

> @@ -2148,6 +2159,28 @@ static struct file_system_type cgroup_fs_type = {
>  	.kill_sb = cgroup_kill_sb,
>  };
>  
> +char * __must_check cgroup_path_ns(struct cgroup_namespace *ns,
> +						 struct cgroup *cgrp, char *buf,
> +						 size_t buflen)

Please align to the same column as the argument on the first line and
make the optional @ns the last argument.

> +{
> +	if (ns) {
> +		struct cgroup *root;
> +		root = cset_cgroup_from_root(ns->root_cgrps, cgrp->root);
> +		return kernfs_path_from_node(root->kn, cgrp->kn, buf,
> +					     buflen);
> +	} else {
> +		return kernfs_path(cgrp->kn, buf, buflen);
> +	}
> +}
> +
> +char * __must_check cgroup_path(struct cgroup *cgrp, char *buf,
> +					      size_t buflen)
> +{
> +	return cgroup_path_ns(current->nsproxy->cgroup_ns, cgrp, buf,
> +				      buflen);

Ditto with alignment.

> diff --git a/kernel/cgroup_namespace.c b/kernel/cgroup_namespace.c
> new file mode 100644
> index 0000000..ef20777
> --- /dev/null
> +++ b/kernel/cgroup_namespace.c
> @@ -0,0 +1,127 @@
> +/*
> + *  Copyright (C) 2014 Google Inc.
> + *
> + *  Author: Aditya Kali (adityakali@xxxxxxxxxx)
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms of the GNU General Public License as published by the Free
> + *  Software Foundation, version 2 of the License.
> + */
> +
> +#include <linux/cgroup.h>
> +#include <linux/cgroup_namespace.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/nsproxy.h>
> +#include <linux/proc_ns.h>
> +
> +const struct proc_ns_operations cgroupns_operations;
> +
> +static struct cgroup_namespace *alloc_cgroup_ns(void)
> +{
> +	struct cgroup_namespace *new_ns;
> +	int ret;
> +
> +	new_ns = kzalloc(sizeof(struct cgroup_namespace), GFP_KERNEL);
> +	if (!new_ns)
> +		return ERR_PTR(-ENOMEM);
> +	ret = ns_alloc_inum(&new_ns->ns);
> +	if (ret) {
> +		kfree(new_ns);
> +		return ERR_PTR(ret);
> +	}
> +	atomic_set(&new_ns->count, 1);
> +	new_ns->ns.ops = &cgroupns_operations;
> +	return new_ns;
> +}
> +
> +extern void put_css_set(struct css_set *cset);
> +extern  void get_css_set(struct css_set *cset);

Heh, idk, so we're moving cgroup_get/put() to cgroup.h while
redclaring css_set functions in this file?

> +struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
> +					struct user_namespace *user_ns,
> +					struct cgroup_namespace *old_ns)
> +{
> +	struct cgroup_namespace *new_ns = NULL;
> +	struct css_set *cgrps = NULL;
> +	int err;
> +
> +	BUG_ON(!old_ns);
> +
> +	if (!(flags & CLONE_NEWCGROUP))
> +		return get_cgroup_ns(old_ns);
> +
> +	/* Allow only sysadmin to create cgroup namespace. */
> +	err = -EPERM;
> +	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
> +		goto err_out;
> +
> +	cgrps = task_css_set(current);
> +	get_css_set(cgrps);
> +
> +	err = -ENOMEM;
> +	new_ns = alloc_cgroup_ns();
> +	if (!new_ns)
> +		goto err_out;
> +
> +	new_ns->user_ns = get_user_ns(user_ns);
> +	new_ns->root_cgrps = cgrps;

Let's name it ->root_cset.  The data structures involved are already
really confusing.  No need to add more to it.

Thanks.

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