[Devel] Re: [PATCH] Relax ns_can_attach checks to allow attaching to grandchild cgroups

[Serge E. Hallyn]

Quoting Andrew Morton (akpm at linux-foundation.org):
> (cc containers at lists.osdl.org)
> 
> Please don't send patches via private email!
> 
> 
> 
> From: Grzegorz Nosek <root at localdomain.pl>
> 
> 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 at localdomain.pl>
> Reviewed-by: Li Zefan <lizf at cn.fujitsu.com>
> Cc: Paul Menage <menage at google.com>
> Cc: Balbir Singh <balbir at in.ibm.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com>
> Cc: Pavel Emelyanov <xemul at openvz.org>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>

Acked-by: Serge Hallyn <serue at us.ibm.com>

I trust (since you're not removing it) that the restriction that
the target cgroup be empty is not a problem?

Also, 'rule 1' in the comment above ns_can_attach should be modified
accordingly (s/child/descendant).

thanks,
-serge

> ---
> 
>  include/linux/cgroup.h |    4 ++--
>  kernel/cgroup.c        |   12 +++++++-----
>  kernel/ns_cgroup.c     |    9 +++------
>  3 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff -puN include/linux/cgroup.h~relax-ns_can_attach-checks-to-allow-attaching-to-grandchild-cgroups include/linux/cgroup.h
> --- a/include/linux/cgroup.h~relax-ns_can_attach-checks-to-allow-attaching-to-grandchild-cgroups
> +++ a/include/linux/cgroup.h
> @@ -344,8 +344,8 @@ int cgroup_path(const struct cgroup *cgr
> 
>  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 */
> +int cgroup_is_descendant(const struct cgroup *cgrp, struct task_struct *task);
> 
>  /* Control Group subsystem type. See Documentation/cgroups.txt for details */
> 
> diff -puN kernel/cgroup.c~relax-ns_can_attach-checks-to-allow-attaching-to-grandchild-cgroups kernel/cgroup.c
> --- a/kernel/cgroup.c~relax-ns_can_attach-checks-to-allow-attaching-to-grandchild-cgroups
> +++ a/kernel/cgroup.c
> @@ -3072,18 +3072,19 @@ int cgroup_clone(struct task_struct *tsk
>  }
> 
>  /**
> - * 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
>   *
> - * 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 ret;
>  	struct cgroup *target;
> @@ -3093,7 +3094,8 @@ int cgroup_is_descendant(const struct cg
>  		return 1;
> 
>  	get_first_subsys(cgrp, NULL, &subsys_id);
> -	target = task_cgroup(current, subsys_id);
> +	target = task_cgroup(task, subsys_id);
> +
>  	while (cgrp != target && cgrp!= cgrp->top_cgroup)
>  		cgrp = cgrp->parent;
>  	ret = (cgrp == target);
> diff -puN kernel/ns_cgroup.c~relax-ns_can_attach-checks-to-allow-attaching-to-grandchild-cgroups kernel/ns_cgroup.c
> --- a/kernel/ns_cgroup.c~relax-ns_can_attach-checks-to-allow-attaching-to-grandchild-cgroups
> +++ a/kernel/ns_cgroup.c
> @@ -45,21 +45,18 @@ int ns_cgroup_clone(struct task_struct *
>  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))
>  			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))
>  		return -EPERM;
> 
>  	return 0;
> @@ -77,7 +74,7 @@ static struct cgroup_subsys_state *ns_cr
> 
>  	if (!capable(CAP_SYS_ADMIN))
>  		return ERR_PTR(-EPERM);
> -	if (!cgroup_is_descendant(cgroup))
> +	if (!cgroup_is_descendant(cgroup, current))
>  		return ERR_PTR(-EPERM);
> 
>  	ns_cgroup = kzalloc(sizeof(*ns_cgroup), GFP_KERNEL);
> _
> 
> _______________________________________________
> Containers mailing list
> Containers at lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/containers
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers