Re: ns_can_attach (nsproxy cgroup)

[Grzegorz Nosek <root-AfQBxy1nhrQ00sYp1HPQUA@public.gmane.org>]

On pią, gru 12, 2008 at 08:09:08 -0600, Serge E. Hallyn wrote:
> Quoting Grzegorz Nosek (root@localdomain.pl):
> > Hi all,
> > 
> > Is there a good reason for ns_can_attach to restrict moving tasks only
> > to direct descentants of the current cgroup? I.e. could the code:
> > 
> >         orig = task_cgroup(task, ns_subsys_id);
> >         if (orig && orig != new_cgroup->parent)
> >                 return -EPERM;
> > 
> > be replaced with:
> > 
> >         orig = task_cgroup(task, ns_subsys_id);
> >         if (orig && !cgroup_is_descendant_of(new_cgroup, orig))
> >                 return -EPERM;
> > 
> > (for a suitable definition of cgroup_is_descendant_of). It would allow
> > moving tasks down the cgroup hierarchy more than one level at a time and
> > as far as I can see, would pose no additional problems.
> > 
> > Please keep CC'd, I'm not subscribed.
> 
> Well you can always move it down one level at a time, right? :)
> 
> But I can't think of any reason why it would be a problem.  So
> pls feel free to send a patch.

OK, here's the patch. After some basic testing looks like it's working
as advertised. Please have a look at the patch, esp. the subsys_id part,
I'm not sure I got the code/comments all right.

Best regards,
 Grzegorz Nosek

--------------------------------------
From 8750df0d341c3227a0ed33088602addec003b713 Mon Sep 17 00:00:00 2001
From: Grzegorz Nosek <root@localdomain.pl>
Date: Fri, 12 Dec 2008 21:48:09 +0100
Subject: [PATCH] Relax ns_can_attach checks to allow attaching to grandchild cgroups

The ns_proxy cgroup allows moving processes to child cgroups only one
level deep at a time. This commit relaxes this restriction and makes it
possible to attach tasks directly to grandchild cgroups, e.g.:

($pid is in the root cgroup)
echo $pid > /cgroup/CG1/CG2/tasks

Previously this operation would fail with -EPERM and would have to be
performed as two steps:
echo $pid > /cgroup/CG1/tasks
echo $pid > /cgroup/CG1/CG2/tasks

Signed-off-by: Grzegorz Nosek <root@localdomain.pl>
---
 include/linux/cgroup.h |    5 +++--
 kernel/cgroup.c        |   19 ++++++++++++-------
 kernel/ns_cgroup.c     |    9 +++------
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 1ba3ded..ab8a5cd 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -301,8 +301,9 @@ int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
 
 int cgroup_task_count(const struct cgroup *cgrp);
 
-/* Return true if the cgroup is a descendant of the current cgroup */
-int cgroup_is_descendant(const struct cgroup *cgrp);
+/* Return true if cgrp is a descendant of the task's cgroup subsystem subsys_id */
+int cgroup_is_descendant(const struct cgroup *cgrp, struct task_struct *task,
+	int subsys_id);
 
 /* Control Group subsystem type. See Documentation/cgroups.txt for details */
 
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 2ae040d..0002fa5 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3101,28 +3101,33 @@ int cgroup_clone(struct task_struct *tsk, struct cgroup_subsys *subsys,
 }
 
 /**
- * cgroup_is_descendant - see if @cgrp is a descendant of current task's cgrp
+ * cgroup_is_descendant - see if @cgrp is a descendant of @task's cgrp
  * @cgrp: the cgroup in question
+ * @task: the task in question
+ * @subsys_id: cgroup subsystem id used to determine hierarchy; if negative,
+ * 		use get_first_subsys()
  *
- * See if @cgrp is a descendant of the current task's cgroup in
- * the appropriate hierarchy.
+ * See if @cgrp is a descendant of @task's cgroup in the appropriate
+ * hierarchy.
  *
  * If we are sending in dummytop, then presumably we are creating
  * the top cgroup in the subsystem.
  *
  * Called only by the ns (nsproxy) cgroup.
  */
-int cgroup_is_descendant(const struct cgroup *cgrp)
+int cgroup_is_descendant(const struct cgroup *cgrp, struct task_struct *task,
+	int subsys_id)
 {
 	int ret;
 	struct cgroup *target;
-	int subsys_id;
 
 	if (cgrp == dummytop)
 		return 1;
 
-	get_first_subsys(cgrp, NULL, &subsys_id);
-	target = task_cgroup(current, subsys_id);
+	if (subsys_id < 0)
+		get_first_subsys(cgrp, NULL, &subsys_id);
+
+	target = task_cgroup(task, subsys_id);
 	while (cgrp != target && cgrp!= cgrp->top_cgroup)
 		cgrp = cgrp->parent;
 	ret = (cgrp == target);
diff --git a/kernel/ns_cgroup.c b/kernel/ns_cgroup.c
index 43c2111..5247d84 100644
--- a/kernel/ns_cgroup.c
+++ b/kernel/ns_cgroup.c
@@ -46,21 +46,18 @@ int ns_cgroup_clone(struct task_struct *task, struct pid *pid)
 static int ns_can_attach(struct cgroup_subsys *ss,
 		struct cgroup *new_cgroup, struct task_struct *task)
 {
-	struct cgroup *orig;
-
 	if (current != task) {
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 
-		if (!cgroup_is_descendant(new_cgroup))
+		if (!cgroup_is_descendant(new_cgroup, current, -1))
 			return -EPERM;
 	}
 
 	if (atomic_read(&new_cgroup->count) != 0)
 		return -EPERM;
 
-	orig = task_cgroup(task, ns_subsys_id);
-	if (orig && orig != new_cgroup->parent)
+	if (!cgroup_is_descendant(new_cgroup, task, ns_subsys_id))
 		return -EPERM;
 
 	return 0;
@@ -78,7 +75,7 @@ static struct cgroup_subsys_state *ns_create(struct cgroup_subsys *ss,
 
 	if (!capable(CAP_SYS_ADMIN))
 		return ERR_PTR(-EPERM);
-	if (!cgroup_is_descendant(cgroup))
+	if (!cgroup_is_descendant(cgroup, current, -1))
 		return ERR_PTR(-EPERM);
 
 	ns_cgroup = kzalloc(sizeof(*ns_cgroup), GFP_KERNEL);
-- 
1.5.4.4

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers