cgroup attach/fork hooks consistency with the ns_cgroup

cgroup attach/fork hooks consistency with the ns_cgroup

[Daniel Lezcano]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

Hi,

I noticed two different behaviours, the second one looks weird for me:

  1) when the cgroup is manually created:
	mkdir /cgroup/foo
	echo $$ > /cgroup/foo/tasks

  only the "attach" callback is called as expected.

  2) when the cgroup is automatically created via the ns_cgroup with the 
clone function and the namespace flags,

   the "attach" *and* the "fork" callbacks are called.


IMHO, these two different behaviours look inconsistent. Won't this lead 
to some problems or a specific code to handle both cases if a cgroup is 
using the fork and the attach hooks ?

For example, let's imagine we create a control group which shows the 
number of tasks running. We have a global atomic and we display its 
value in the cgroupfs.

When a task attaches to the cgroup, we do atomic_inc in the attach 
callback. For all its child, the fork hook will do atomic_inc and exit 
hook will do atomic_dec.

If we create the cgroup manually like the case 1) that works. But if we 
use the control group with the ns_cgroup the task counter will be set to 
2 for the first tasks entering the cgroup because the attach callback 
will increment the counter and the fork callback will increment it again.

In attachment a source code to illustrate the example.

Shouldn't the ns_cgroup_clone be called after the cgroup_fork_callbacks 
in copy_process function ? So we don't call the fork callback for the 
first tasks and we keep the consistency ?

Thanks
  -- Daniel



[-- Attachment #2: make-evident-the-ns-cgroup-bug.patch --]
[-- Type: text/x-diff, Size: 3350 bytes --]

---
 include/linux/cgroup_subsys.h |    1 
 kernel/Makefile               |    2 
 kernel/cgroup_bug.c           |   87 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 1 deletion(-)

Index: linux-2.6/kernel/cgroup_bug.c
===================================================================
--- /dev/null
+++ linux-2.6/kernel/cgroup_bug.c
@@ -0,0 +1,87 @@
+#include <linux/cgroup.h>
+#include <asm/atomic.h>
+
+struct bug_cg {
+	struct cgroup_subsys_state css;
+	atomic_t ntasks;
+};
+
+static inline struct bug_cg *bug_cgroup(struct cgroup *cgroup)
+{
+	return container_of(cgroup_subsys_state(cgroup, bug_subsys_id),
+			    struct bug_cg, css);
+}
+
+static struct cgroup_subsys_state *bug_cgroup_create(struct cgroup_subsys *ss,
+						     struct cgroup *cgroup)
+{
+	struct bug_cg *bug_cg;
+
+	bug_cg = kzalloc(sizeof(*bug_cg), GFP_KERNEL);
+	if (!bug_cg)
+		return ERR_PTR(-ENOMEM);
+
+	atomic_set(&bug_cg->ntasks, 0);
+
+	return &bug_cg->css;
+}
+
+static void bug_cgroup_destroy(struct cgroup_subsys *ss, struct cgroup *cgroup)
+{
+	struct bug_cg *bug_cg = bug_cgroup(cgroup);
+	kfree(bug_cg);
+}
+
+static void bug_cgroup_attach(struct cgroup_subsys *ss, struct cgroup *cgroup,
+			      struct cgroup *old_cgroup,
+			      struct task_struct *task)
+{
+	struct bug_cg *bug_cg = bug_cgroup(cgroup);
+
+	atomic_inc(&bug_cg->ntasks);
+}
+
+static int bug_cgroup_fork(struct cgroup_subsys *ss, struct task_struct *task,
+			   unsigned long clone_flags)
+{
+	struct cgroup *cgroup = task_cgroup(task, bug_subsys_id);
+
+	atomic_inc(&bug_cgroup(cgroup)->ntasks);
+
+	return 0;
+}
+
+static void bug_cgroup_exit(struct cgroup_subsys *ss, struct task_struct *task)
+{
+	struct cgroup *cgroup = task_cgroup(task, bug_subsys_id);
+
+	atomic_dec(&bug_cgroup(cgroup)->ntasks);
+}
+
+static u64 task_count_read(struct cgroup *cgroup, struct cftype *cft)
+{
+	return atomic_read(&bug_cgroup(cgroup)->ntasks);
+}
+
+static struct cftype files[] = {
+	{
+		.name = "ntask",
+		.read_u64 = task_count_read,
+	},
+};
+
+static int bug_cgroup_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
+{
+	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
+}
+
+struct cgroup_subsys bug_subsys = {
+	.name		= "bug",
+	.subsys_id	= bug_subsys_id,
+	.create		= bug_cgroup_create,
+	.destroy	= bug_cgroup_destroy,
+	.populate	= bug_cgroup_populate,
+	.attach         = bug_cgroup_attach,
+	.fork		= bug_cgroup_fork,
+	.exit		= bug_cgroup_exit,
+};
Index: linux-2.6/include/linux/cgroup_subsys.h
===================================================================
--- linux-2.6.orig/include/linux/cgroup_subsys.h
+++ linux-2.6/include/linux/cgroup_subsys.h
@@ -21,6 +21,7 @@ SUBSYS(debug)
 
 #ifdef CONFIG_CGROUP_NS
 SUBSYS(ns)
+SUBSYS(bug)
 #endif
 
 /* */
Index: linux-2.6/kernel/Makefile
===================================================================
--- linux-2.6.orig/kernel/Makefile
+++ linux-2.6/kernel/Makefile
@@ -60,7 +60,7 @@ obj-$(CONFIG_CGROUPS) += cgroup.o
 obj-$(CONFIG_CGROUP_DEBUG) += cgroup_debug.o
 obj-$(CONFIG_CGROUP_FREEZER) += cgroup_freezer.o
 obj-$(CONFIG_CPUSETS) += cpuset.o
-obj-$(CONFIG_CGROUP_NS) += ns_cgroup.o
+obj-$(CONFIG_CGROUP_NS) += ns_cgroup.o cgroup_bug.o
 obj-$(CONFIG_UTS_NS) += utsname.o
 obj-$(CONFIG_USER_NS) += user_namespace.o
 obj-$(CONFIG_PID_NS) += pid_namespace.o

[-- Attachment #3: Type: text/plain, Size: 206 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Re: cgroup attach/fork hooks consistency with the ns_cgroup

[Serge E. Hallyn]

Quoting Daniel Lezcano (daniel.lezcano-GANU6spQydw@public.gmane.org):
> Hi,
>
> I noticed two different behaviours, the second one looks weird for me:
>
>  1) when the cgroup is manually created:
> 	mkdir /cgroup/foo
> 	echo $$ > /cgroup/foo/tasks
>
>  only the "attach" callback is called as expected.
>
>  2) when the cgroup is automatically created via the ns_cgroup with the  
> clone function and the namespace flags,
>
>   the "attach" *and* the "fork" callbacks are called.
>
>
> IMHO, these two different behaviours look inconsistent. Won't this lead  
> to some problems or a specific code to handle both cases if a cgroup is  
> using the fork and the attach hooks ?
>
> For example, let's imagine we create a control group which shows the  
> number of tasks running. We have a global atomic and we display its  
> value in the cgroupfs.
>
> When a task attaches to the cgroup, we do atomic_inc in the attach  
> callback. For all its child, the fork hook will do atomic_inc and exit  
> hook will do atomic_dec.
>
> If we create the cgroup manually like the case 1) that works. But if we  
> use the control group with the ns_cgroup the task counter will be set to  
> 2 for the first tasks entering the cgroup because the attach callback  
> will increment the counter and the fork callback will increment it again.
>
> In attachment a source code to illustrate the example.
>
> Shouldn't the ns_cgroup_clone be called after the cgroup_fork_callbacks  
> in copy_process function ? So we don't call the fork callback for the  
> first tasks and we keep the consistency ?

The ns cgroup is really only good for preventing root in a container
from escaping its cgroup-imposed limits.  The same can be done today
using smack or selinux, and eventually will be possible using user
namespaces.  Would anyone object to removing ns_cgroup?

It won't just remove kernel/ns_cgroup.c, but some subtle code in
fork.c, nsproxy.c, and of course cgroup.c as well.

There admittedly is minute convenience gain in not having to
manually create a new cgroup and attach a cloned child to it, but
that wasn't the intent of the cgroup.

-serge

Re: cgroup attach/fork hooks consistency with the ns_cgroup

[Li Zefan]

> The ns cgroup is really only good for preventing root in a container
> from escaping its cgroup-imposed limits.  The same can be done today
> using smack or selinux, and eventually will be possible using user
> namespaces.  Would anyone object to removing ns_cgroup?
> 

I vote for removing it. :)

> It won't just remove kernel/ns_cgroup.c, but some subtle code in
> fork.c, nsproxy.c, and of course cgroup.c as well.
> 

Yeah, regarding to cgroup, cgroup_clone() and cgroup_is_descendant()
can be removed. cgroup_clone() is somewhat ugly I think.

> There admittedly is minute convenience gain in not having to
> manually create a new cgroup and attach a cloned child to it, but
> that wasn't the intent of the cgroup.
> 

Re: cgroup attach/fork hooks consistency with the ns_cgroup

[Paul Menage]

On Wed, Jun 17, 2009 at 2:26 PM, Serge E. Hallyn<serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
>
> The ns cgroup is really only good for preventing root in a container
> from escaping its cgroup-imposed limits.  The same can be done today
> using smack or selinux, and eventually will be possible using user
> namespaces.  Would anyone object to removing ns_cgroup?

Sounds reasonable to me. It feels to me that there ought to be some
good way to integrate namespaces and cgroups, but I'm not quite sure
exactly how, and ns_cgroup sort of hovers in the "toy" category rather
than something very useful.

Paul

Re: cgroup attach/fork hooks consistency with the ns_cgroup

[Serge E. Hallyn]

Quoting Paul Menage (menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org):
> On Wed, Jun 17, 2009 at 2:26 PM, Serge E. Hallyn<serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
> >
> > The ns cgroup is really only good for preventing root in a container
> > from escaping its cgroup-imposed limits.  The same can be done today
> > using smack or selinux, and eventually will be possible using user
> > namespaces.  Would anyone object to removing ns_cgroup?
> 
> Sounds reasonable to me. It feels to me that there ought to be some
> good way to integrate namespaces and cgroups, but I'm not quite sure
> exactly how, and ns_cgroup sort of hovers in the "toy" category rather
> than something very useful.

So the question becomes: does the presence of the ns cgroup constitute
an API?  Can we just yank it out?

Daniel, AFAIK liblxc is the only thing that actually uses it.  Do
you mind manually moving the container init into a new cgroup?

-serge

Re: cgroup attach/fork hooks consistency with the ns_cgroup

[Daniel Lezcano]

Serge E. Hallyn wrote:
> Quoting Paul Menage (menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org):
>   
>> On Wed, Jun 17, 2009 at 2:26 PM, Serge E. Hallyn<serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
>>     
>>> The ns cgroup is really only good for preventing root in a container
>>> from escaping its cgroup-imposed limits.  The same can be done today
>>> using smack or selinux, and eventually will be possible using user
>>> namespaces.  Would anyone object to removing ns_cgroup?
>>>       
>> Sounds reasonable to me. It feels to me that there ought to be some
>> good way to integrate namespaces and cgroups, but I'm not quite sure
>> exactly how, and ns_cgroup sort of hovers in the "toy" category rather
>> than something very useful.
>>     
>
> So the question becomes: does the presence of the ns cgroup constitute
> an API?  Can we just yank it out?
>
> Daniel, AFAIK liblxc is the only thing that actually uses it.  Do
> you mind manually moving the container init into a new cgroup?
>   

It is not a big deal to manually create and attach to the cgroup and I 
would prefer to remove the ns_cgroup if that helps to keep the cgroup 
behaviour consistent.
But the behaviour will change, no ? I mean the ns_cgroup clones the 
cgroup parent so we inherit its values. If we remove the ns_cgroup, that 
won't happen and I will have to handle some specific cases. For example, 
cpuset.cpus and cpuset.mems won't be filled and I will not be able to 
attach until I set them, so I have to be aware of this control group in 
the code and copy the values from the parent. That's annoying, I tried 
to have the liblxc cgroup code generic so it supports any kind of cgroup :/

There isn't a rule saying that we will inherit the values set by the 
parent ? If it is case, maybe we can remove the ns_cgroup and fix the 
cpuset at the same time, no ?

By the way, Andrew complained when we changed the name of the ns_cgroup 
"node_<pid>" by "<pid>" that we don't have to change the api when 
something is in the kernel. IMO, he won't be happy if we remove the 
ns_cgroup :/
Maybe, we can first fix the ns_cgroup hook problem by moving the 
ns_cgroup_clone after cgroup_fork_callbacks and propose a cgroup related 
to the namespaces. When we activate this new cgroup that disables the 
ns_cgroup and we put this cgroup_namespace as default in the Kconfig, no ?

Thanks
-- Daniel

Re: cgroup attach/fork hooks consistency with the ns_cgroup

[Paul Menage]

On Thu, Jun 18, 2009 at 11:36 AM, Daniel Lezcano<daniel.lezcano-GANU6spQydw@public.gmane.org> wrote:
>
> There isn't a rule saying that we will inherit the values set by the parent
> ? If it is case, maybe we can remove the ns_cgroup and fix the cpuset at the
> same time, no ?

There's no rule either way, but there is the backward-compatibility
aspects of cpusets.

One way around that would be to add a "cgroup.clone_children" control
file - if you write 1 to it (it defaults to 0) then all mkdir
operations do a clone (i.e. pre-populate the child with appropriate
defaults even if that's not the normal behaviour for the subsystem.
That would avoid compatibility issues.

> Maybe, we can first fix the ns_cgroup hook problem by moving the
> ns_cgroup_clone after cgroup_fork_callbacks

 You mean fork the task into the parent cgroup, then clone the new
cgroup and reattach to the new cgroup? That could work, although I'm
not sure whether it would be bad to have the new task briefly
appearing in the parent cgroups.

Paul

Re: cgroup attach/fork hooks consistency with the ns_cgroup

[Daniel Lezcano]

Paul Menage wrote:
> On Thu, Jun 18, 2009 at 11:36 AM, Daniel Lezcano<daniel.lezcano-GANU6spQydw@public.gmane.org> wrote:
>   
>> There isn't a rule saying that we will inherit the values set by the parent
>> ? If it is case, maybe we can remove the ns_cgroup and fix the cpuset at the
>> same time, no ?
>>     
>
> There's no rule either way, but there is the backward-compatibility
> aspects of cpusets.
>
> One way around that would be to add a "cgroup.clone_children" control
> file - if you write 1 to it (it defaults to 0) then all mkdir
> operations do a clone (i.e. pre-populate the child with appropriate
> defaults even if that's not the normal behaviour for the subsystem.
> That would avoid compatibility issues.
>   

Yes, that sounds good.
>> Maybe, we can first fix the ns_cgroup hook problem by moving the
>> ns_cgroup_clone after cgroup_fork_callbacks
>>     
>
>  You mean fork the task into the parent cgroup, then clone the new
> cgroup and reattach to the new cgroup? That could work, although I'm
> not sure whether it would be bad to have the new task briefly
> appearing in the parent cgroups.
>   
I tried the following patch:

Index: linux-2.6/kernel/fork.c
===================================================================
--- linux-2.6.orig/kernel/fork.c
+++ linux-2.6/kernel/fork.c
@@ -1150,12 +1150,6 @@ static struct task_struct *copy_process(
        if (clone_flags & CLONE_THREAD)
                p->tgid = current->tgid;
 
-       if (current->nsproxy != p->nsproxy) {
-               retval = ns_cgroup_clone(p, pid);
-               if (retval)
-                       goto bad_fork_free_graph;
-       }
-
        p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? 
child_tidptr : NULL;
        /*
         * Clear TID on mm_release()?
@@ -1204,6 +1198,12 @@ static struct task_struct *copy_process(
        if (retval)
                goto bad_fork_free_graph;
 
+       if (current->nsproxy != p->nsproxy) {
+               retval = ns_cgroup_clone(p, pid);
+               if (retval)
+                       goto bad_fork_cgroup_callbacks;
+       }
+
        /* Need tasklist lock for parent etc handling! */
        write_lock_irq(&tasklist_lock);

That seems to fix the inconsistency problem but maybe I am missing 
something...
Excepting kernel race condition in the copy_process function I may have 
missed, I don't see the difference with a program forking and adding the 
task in the child cgroup (or the child process adds itself right after 
the fork), the child will briefly appear to the parent cgroup, no ?

  -- Daniel

Re: cgroup attach/fork hooks consistency with the ns_cgroup

[Serge E. Hallyn]

Quoting Daniel Lezcano (daniel.lezcano-GANU6spQydw@public.gmane.org):
> Paul Menage wrote:
>> On Thu, Jun 18, 2009 at 11:36 AM, Daniel Lezcano<daniel.lezcano-GANU6spQydw@public.gmane.org> wrote:
>>   
>>> There isn't a rule saying that we will inherit the values set by the parent
>>> ? If it is case, maybe we can remove the ns_cgroup and fix the cpuset at the
>>> same time, no ?
>>>     
>>
>> There's no rule either way, but there is the backward-compatibility
>> aspects of cpusets.
>>
>> One way around that would be to add a "cgroup.clone_children" control
>> file - if you write 1 to it (it defaults to 0) then all mkdir
>> operations do a clone (i.e. pre-populate the child with appropriate
>> defaults even if that's not the normal behaviour for the subsystem.
>> That would avoid compatibility issues.
>>   
>
> Yes, that sounds good.
>>> Maybe, we can first fix the ns_cgroup hook problem by moving the
>>> ns_cgroup_clone after cgroup_fork_callbacks
>>>     
>>
>>  You mean fork the task into the parent cgroup, then clone the new
>> cgroup and reattach to the new cgroup? That could work, although I'm
>> not sure whether it would be bad to have the new task briefly
>> appearing in the parent cgroups.
>>   
> I tried the following patch:

I think the patch should be fine.

-serge

> Index: linux-2.6/kernel/fork.c
> ===================================================================
> --- linux-2.6.orig/kernel/fork.c
> +++ linux-2.6/kernel/fork.c
> @@ -1150,12 +1150,6 @@ static struct task_struct *copy_process(
>        if (clone_flags & CLONE_THREAD)
>                p->tgid = current->tgid;
>
> -       if (current->nsproxy != p->nsproxy) {
> -               retval = ns_cgroup_clone(p, pid);
> -               if (retval)
> -                       goto bad_fork_free_graph;
> -       }
> -
>        p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ?  
> child_tidptr : NULL;
>        /*
>         * Clear TID on mm_release()?
> @@ -1204,6 +1198,12 @@ static struct task_struct *copy_process(
>        if (retval)
>                goto bad_fork_free_graph;
>
> +       if (current->nsproxy != p->nsproxy) {
> +               retval = ns_cgroup_clone(p, pid);
> +               if (retval)
> +                       goto bad_fork_cgroup_callbacks;
> +       }
> +
>        /* Need tasklist lock for parent etc handling! */
>        write_lock_irq(&tasklist_lock);
>
> That seems to fix the inconsistency problem but maybe I am missing  
> something...
> Excepting kernel race condition in the copy_process function I may have  
> missed, I don't see the difference with a program forking and adding the  
> task in the child cgroup (or the child process adds itself right after  
> the fork), the child will briefly appear to the parent cgroup, no ?
>
>  -- Daniel
>
>