Thread (46 messages) 46 messages, 5 authors, 2015-12-07

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

From: Tejun Heo <hidden>
Date: 2015-11-24 16:49:35
Also in: cgroups, lkml

Hello,

On Mon, Nov 16, 2015 at 01:51:42PM -0600, serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org wrote:
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
+
 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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org)
+ *
+ *  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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help