[patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[Mike Galbraith]

kworkers can be born in a cpuset, leaving them adrift on an unsinkable ship.
Allow them to be moved to the root cpuset so the cpuset can be destroyed.

Signed-off-by: Mike Galbraith <efault@gmx.de>

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 10131fd..b26f4c4 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1384,7 +1384,7 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
 	 * be changed.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND && cont->parent)
 		return -EINVAL;
 
 	return 0;

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[Li Zefan]

cc David, who added this restriction

于 2011年09月23日 14:21, Mike Galbraith wrote:
> 
> kworkers can be born in a cpuset, leaving them adrift on an unsinkable ship.
> Allow them to be moved to the root cpuset so the cpuset can be destroyed.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> 

I've noticed this long ago, so..

Acked-by: Li Zefan <lizf@cn.fujitsu.com>

> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index 10131fd..b26f4c4 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -1384,7 +1384,7 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
>  	 * be changed.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND && cont->parent)

To make the code more self-explanatory, I think this is a bit better:

	if (... && cont != cont->top_cgroup)

>  		return -EINVAL;
>  
>  	return 0;
> 

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[Mike Galbraith]

On Fri, 2011-09-23 at 15:00 +0800, Li Zefan wrote:

> To make the code more self-explanatory, I think this is a bit better:
> 
> 	if (... && cont != cont->top_cgroup)
> 

Done, your ACK added as well.

kworkers can be born in a cpuset, leaving them adrift on an unsinkable ship.
Allow them to be moved to the root cpuset so the cpuset can be destroyed.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Acked-by: Li Zefan <lizf@cn.fujitsu.com>

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 10131fd..b26f4c4 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1384,7 +1384,7 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
 	 * be changed.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND && cont != cont->top_cgroup)
 		return -EINVAL;
 
 	return 0;

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[David Rientjes]

On Fri, 23 Sep 2011, Mike Galbraith wrote:

> Done, your ACK added as well.
> 
> kworkers can be born in a cpuset, leaving them adrift on an unsinkable ship.
> Allow them to be moved to the root cpuset so the cpuset can be destroyed.
> 

Thanks Li for the cc since I introduced this flag.

Does this even work?

You've modified cpuset_can_attach() to allow the attach for the cgroup 
interface, but the actual function that attaches the task in the cpuset 
code should fail since it does set_cpus_allowed_ptr() which should return 
-EINVAL because of the PF_THREAD_BOUND.  That should have emitted a 
WARN_ON() if this patch was tested.

kworkers can always move themselves to the root cpuset, so that's probably 
the better way of handling this than requiring userspace to do so.

And it seems like the workqueue code has a comment specifically about cpu 
hotplug and why changing their cpumask shouldn't be allowed that was added 
by Tejun Heo in db7bccf45cb8 ("workqueue: reimplement CPU hotplugging 
support using trustee").  So let's cc him here as well at his 
non-kernel.org address.

> Signed-off-by: Mike Galbraith <efault@gmx.de>
> Acked-by: Li Zefan <lizf@cn.fujitsu.com>
> 
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index 10131fd..b26f4c4 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -1384,7 +1384,7 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
>  	 * be changed.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND && cont != cont->top_cgroup)
>  		return -EINVAL;
>  
>  	return 0;

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[Mike Galbraith]

On Fri, 2011-09-23 at 02:12 -0700, David Rientjes wrote:
> On Fri, 23 Sep 2011, Mike Galbraith wrote:
> 
> > Done, your ACK added as well.
> > 
> > kworkers can be born in a cpuset, leaving them adrift on an unsinkable ship.
> > Allow them to be moved to the root cpuset so the cpuset can be destroyed.
> > 
> 
> Thanks Li for the cc since I introduced this flag.
> 
> Does this even work?

Yup, but..

> You've modified cpuset_can_attach() to allow the attach for the cgroup 
> interface, but the actual function that attaches the task in the cpuset 
> code should fail since it does set_cpus_allowed_ptr() which should return 
> -EINVAL because of the PF_THREAD_BOUND.  That should have emitted a 
> WARN_ON() if this patch was tested.

..you're right that it warns.  It was tested, but I didn't notice that
it had griped at me.

> kworkers can always move themselves to the root cpuset, so that's probably 
> the better way of handling this than requiring userspace to do so.

kworker is sleeping in a cpuset that the user wants to depopulate and
destroy.  I wasn't requiring the user to do that, was allowing him to.

> And it seems like the workqueue code has a comment specifically about cpu 
> hotplug and why changing their cpumask shouldn't be allowed that was added 
> by Tejun Heo in db7bccf45cb8 ("workqueue: reimplement CPU hotplugging 
> support using trustee").  So let's cc him here as well at his 
> non-kernel.org address.

No, we don't want top muck around with their masks.

	-Mike

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[Mike Galbraith]

On Fri, 2011-09-23 at 11:42 +0200, Mike Galbraith wrote:
> On Fri, 2011-09-23 at 02:12 -0700, David Rientjes wrote:
> > On Fri, 23 Sep 2011, Mike Galbraith wrote:
> > 
> > > Done, your ACK added as well.
> > > 
> > > kworkers can be born in a cpuset, leaving them adrift on an unsinkable ship.
> > > Allow them to be moved to the root cpuset so the cpuset can be destroyed.
> > > 
> > 
> > Thanks Li for the cc since I introduced this flag.
> > 
> > Does this even work?
> 
> Yup, but..
> 
> > You've modified cpuset_can_attach() to allow the attach for the cgroup 
> > interface, but the actual function that attaches the task in the cpuset 
> > code should fail since it does set_cpus_allowed_ptr() which should return 
> > -EINVAL because of the PF_THREAD_BOUND.  That should have emitted a 
> > WARN_ON() if this patch was tested.
> 
> ..you're right that it warns.  It was tested, but I didn't notice that
> it had griped at me.
> 
> > kworkers can always move themselves to the root cpuset, so that's probably 
> > the better way of handling this than requiring userspace to do so.
> 
> kworker is sleeping in a cpuset that the user wants to depopulate and
> destroy.  I wasn't requiring the user to do that, was allowing him to.

So unless there's some reason kworker threads should worry about their
birthplace, seems to me we should just let the cpuset interface handle
the oddball case.

cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

kworkers can be born in a cpuset, leaving them adrift on an unsinkable ship.
Allow them to be moved to the root cpuset so the cpuset can be destroyed

Signed-off-by: Mike Galbraith <efault@gmx.de>
Acked-by: Li Zefan <lizf@cn.fujitsu.com>

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 10131fd..3769f9e 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1384,7 +1384,7 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
 	 * be changed.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND && cont != cont->top_cgroup)
 		return -EINVAL;
 
 	return 0;
@@ -1426,9 +1426,14 @@ static void cpuset_attach_task(struct cgroup *cont, struct task_struct *tsk)
 	/*
 	 * can_attach beforehand should guarantee that this doesn't fail.
 	 * TODO: have a better way to handle failure here
+	 * 
+	 * Special case: bound kthreads born in a cpuset may be moved to
+	 * the top level cpuset without attempting to diddle their mask.
 	 */
-	err = set_cpus_allowed_ptr(tsk, cpus_attach);
-	WARN_ON_ONCE(err);
+	if (!(tsk->flags & PF_THREAD_BOUND && cont == cont->top_cgroup)) {
+		err = set_cpus_allowed_ptr(tsk, cpus_attach);
+		WARN_ON_ONCE(err);
+	}
 
 	cpuset_change_task_nodemask(tsk, &cpuset_attach_nodemask_to);
 	cpuset_update_task_spread_flag(cs, tsk);

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[David Rientjes]

On Fri, 23 Sep 2011, Mike Galbraith wrote:

> cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset
> 
> kworkers can be born in a cpuset, leaving them adrift on an unsinkable ship.
> Allow them to be moved to the root cpuset so the cpuset can be destroyed
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> Acked-by: Li Zefan <lizf@cn.fujitsu.com>

Did Li ack this version?

> 
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index 10131fd..3769f9e 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -1384,7 +1384,7 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
>  	 * be changed.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND && cont != cont->top_cgroup)
>  		return -EINVAL;
>  
>  	return 0;
> @@ -1426,9 +1426,14 @@ static void cpuset_attach_task(struct cgroup *cont, struct task_struct *tsk)
>  	/*
>  	 * can_attach beforehand should guarantee that this doesn't fail.
>  	 * TODO: have a better way to handle failure here
> +	 * 
> +	 * Special case: bound kthreads born in a cpuset may be moved to
> +	 * the top level cpuset without attempting to diddle their mask.
>  	 */
> -	err = set_cpus_allowed_ptr(tsk, cpus_attach);
> -	WARN_ON_ONCE(err);
> +	if (!(tsk->flags & PF_THREAD_BOUND && cont == cont->top_cgroup)) {
> +		err = set_cpus_allowed_ptr(tsk, cpus_attach);
> +		WARN_ON_ONCE(err);
> +	}
>  
>  	cpuset_change_task_nodemask(tsk, &cpuset_attach_nodemask_to);
>  	cpuset_update_task_spread_flag(cs, tsk);

This doesn't make any sense, the user can now change the cpumask of the 
kworker with cpusets but not with sched_setaffinity().

PF_THREAD_BOUND is set specifically so threads cannot move from the cpu 
that they are bound to, that's why the cpuset code and sched_setaffinity() 
reject such a configuration.  So the problem isn't in the cpuset code or 
scheduler at all, you would need to deal with this in the kworker code by 
either not setting PF_THREAD_BOUND (which, according to the comment, Tejun 
thinks is pretty important) or manage the worker threads in a way such 
that the new cpumask (all cpus, since it's in the root cpuset) actually 
make sense for that kworker.  The cpuset code won't care if the kworker 
has a cpumask of all online cpus.

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[Mike Galbraith]

On Fri, 2011-09-23 at 06:27 -0700, David Rientjes wrote:
> On Fri, 23 Sep 2011, Mike Galbraith wrote:
> 
> > cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset
> > 
> > kworkers can be born in a cpuset, leaving them adrift on an unsinkable ship.
> > Allow them to be moved to the root cpuset so the cpuset can be destroyed
> > 
> > Signed-off-by: Mike Galbraith <efault@gmx.de>
> > Acked-by: Li Zefan <lizf@cn.fujitsu.com>
> 
> Did Li ack this version?

Oops, no.
 
> > diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> > index 10131fd..3769f9e 100644
> > --- a/kernel/cpuset.c
> > +++ b/kernel/cpuset.c
> > @@ -1384,7 +1384,7 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
> >  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> >  	 * be changed.
> >  	 */
> > -	if (tsk->flags & PF_THREAD_BOUND)
> > +	if (tsk->flags & PF_THREAD_BOUND && cont != cont->top_cgroup)
> >  		return -EINVAL;
> >  
> >  	return 0;
> > @@ -1426,9 +1426,14 @@ static void cpuset_attach_task(struct cgroup *cont, struct task_struct *tsk)
> >  	/*
> >  	 * can_attach beforehand should guarantee that this doesn't fail.
> >  	 * TODO: have a better way to handle failure here
> > +	 * 
> > +	 * Special case: bound kthreads born in a cpuset may be moved to
> > +	 * the top level cpuset without attempting to diddle their mask.
> >  	 */
> > -	err = set_cpus_allowed_ptr(tsk, cpus_attach);
> > -	WARN_ON_ONCE(err);
> > +	if (!(tsk->flags & PF_THREAD_BOUND && cont == cont->top_cgroup)) {
> > +		err = set_cpus_allowed_ptr(tsk, cpus_attach);
> > +		WARN_ON_ONCE(err);
> > +	}
> >  
> >  	cpuset_change_task_nodemask(tsk, &cpuset_attach_nodemask_to);
> >  	cpuset_update_task_spread_flag(cs, tsk);
> 
> This doesn't make any sense, the user can now change the cpumask of the 
> kworker with cpusets but not with sched_setaffinity().

marge:~/tmp # ps l `cat /cpusets/system/tasks`|grep kworker
1     0  6466     2  20   0      0     0 schedu S    ?          0:00 [kworker/2:4]

Ok, there's a kworker which was born in 'system' cpuset, cpus 0-2.

marge:~/tmp # taskset -p 6466
pid 6466's current affinity mask: 4
marge:~/tmp # cset shield --reset
cset: --> deactivating/reseting shielding
cset: moving 7 tasks from "/user" user set to root set...
[==================================================]%
cset: moving 243 tasks from "/system" system set to root set...
[==================================================]%
cset: deleting "/user" and "/system" sets
cset: done
marge:~/tmp # taskset -p 6466
pid 6466's current affinity mask: 4

Worker still alive, affinity intact, 'system' cpuset destroyed, no
gripage in dmesg.  All seems fine.

> PF_THREAD_BOUND is set specifically so threads cannot move from the cpu 
> that they are bound to, that's why the cpuset code and sched_setaffinity() 
> reject such a configuration.

Yeah.

>   So the problem isn't in the cpuset code or 
> scheduler at all, you would need to deal with this in the kworker code by 
> either not setting PF_THREAD_BOUND (which, according to the comment, Tejun 
> thinks is pretty important) or manage the worker threads in a way such 
> that the new cpumask (all cpus, since it's in the root cpuset) actually 
> make sense for that kworker.  The cpuset code won't care if the kworker 
> has a cpumask of all online cpus.

I don't see why it's a kworker code problem.  The kworker code couldn't
care less about cpusets.  But I don't care who fixes it or how.

If cpusets doesn't want to let PF_THREAD_BOUND threads out, how about
cpusets not letting userland scripts attach kthreadd instead?

cpusets: disallow moving kthreadd into a cpuset.

If kthreadd is moved into a cpuset, it's child may then acquire
PF_THREAD_BOUND and become trapped, making it's cpuset immortal.

Signed-off-by: Mike Galbraith <efault@gmx.de>

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 10131fd..0d9cd04 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -59,6 +59,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/cgroup.h>
+#include <linux/kthread.h>
 
 /*
  * Workqueue for cpuset related tasks.
@@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
 	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
 	 * applicable for such threads.  This prevents checking for success of
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
-	 * be changed.
+	 * be changed.  We also disallow attaching kthreadd, to prevent it's
+	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
 		return -EINVAL;
 
 	return 0;

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[Mike Galbraith]

On Fri, 2011-09-23 at 16:33 +0200, Mike Galbraith wrote:

> @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
>  	 * applicable for such threads.  This prevents checking for success of
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> -	 * be changed.
> +	 * be changed.  We also disallow attaching kthreadd, to prevent it's
> +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
>  		return -EINVAL;
>  
>  	return 0;

And maybe || is_container_init(tsk) for symmetry.

	-Mike

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[David Rientjes]

On Fri, 23 Sep 2011, Mike Galbraith wrote:

> I don't see why it's a kworker code problem.  The kworker code couldn't
> care less about cpusets.  But I don't care who fixes it or how.
> 

The problem is that PF_THREAD_BOUND means that nothing other than the task 
itself may change the set of allowed cpus to be different.  For this 
reason, they have always been declined to be moved amongst cpusets because 
even though their bound cpu may be a subset of the cpuset's cpumask at the 
time of attach, that cpuset mask may subsequently change and therefore try 
to force the PF_THREAD_BOUND off the cpu it is bound to resulting in an 
inconsistency between the cpuset's mask and an attached task's mask.

Allowing a PF_THREAD_BOUND thread to move to the root cpuset isn't so much 
of a problem because it's guaranteed to never exclude the bound cpu, but 
the problem already exists if the kworker is in the child cpuset and 
userspace attempts to change the mems of that cpuset to be disjoint.  That 
results in an inconsistency because the scheduler will refuse it.

> cpusets: disallow moving kthreadd into a cpuset.
> 
> If kthreadd is moved into a cpuset, it's child may then acquire
> PF_THREAD_BOUND and become trapped, making it's cpuset immortal.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> 
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index 10131fd..0d9cd04 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -59,6 +59,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/cgroup.h>
> +#include <linux/kthread.h>
>  
>  /*
>   * Workqueue for cpuset related tasks.
> @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
>  	 * applicable for such threads.  This prevents checking for success of
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> -	 * be changed.
> +	 * be changed.  We also disallow attaching kthreadd, to prevent it's
> +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
>  		return -EINVAL;
>  
>  	return 0;

I like this much better, let's wait to hear from Tejun because he may 
shead some light on whether PF_THREAD_BOUND is really necessary for 
kworkers at all times.

Thanks.

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[Tejun Heo]

Hello,

Sorry about the delay.  I'm mostly offline until the end of this
month.

On Fri, Sep 23, 2011 at 01:20:51PM -0700, David Rientjes wrote:
> > @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
> >  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
> >  	 * applicable for such threads.  This prevents checking for success of
> >  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> > -	 * be changed.
> > +	 * be changed.  We also disallow attaching kthreadd, to prevent it's
> > +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
> >  	 */
> > -	if (tsk->flags & PF_THREAD_BOUND)
> > +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
> >  		return -EINVAL;
> >  
> >  	return 0;
> 
> I like this much better, let's wait to hear from Tejun because he may 
> shead some light on whether PF_THREAD_BOUND is really necessary for 
> kworkers at all times.

Yes, PF_THREAD_BOUND is necessary.  The whole thing depends heavily on
per-cpu behavior.  In addition, I don't think it makes much sense to
put kworkers into a cpuset (or any other resource container) which
isn't global to the system.  If certain CPU intensive tasks require
scheduler based resource limitation, the RTTD would be creating a
dedicated worker thread for it and put restrictions on that specific
kthread.

Putting kthreadd into a sub cpuset and thus putting restrictions on
random kthreads seems like asking for trouble.  So, I agree with the
suggested approach.

Thank you.

-- 
tejun

Re: [patch] cpusets: allow PF_THREAD_BOUND kworkers to escape from a cpuset

[Mike Galbraith]

Maybe the below (which seems to have been stillborn) should be done to
cgroups as well.  Postmortem: kthreadd is attached to a cgroup with no
rt_runtime allocated, gives birth to severely handicapped kstop threads,
humongous crash dump follows.

Fiddling with kthreadd is user error, but since it makes no sense to
move the thing, why not just say no, and save the user's toes some
needless wear and tear.

> If cpusets doesn't want to let PF_THREAD_BOUND threads out, how about
> cpusets not letting userland scripts attach kthreadd instead?
> 
> cpusets: disallow moving kthreadd into a cpuset.
> 
> If kthreadd is moved into a cpuset, it's child may then acquire
> PF_THREAD_BOUND and become trapped, making it's cpuset immortal.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> 
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index 10131fd..0d9cd04 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -59,6 +59,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/cgroup.h>
> +#include <linux/kthread.h>
>  
>  /*
>   * Workqueue for cpuset related tasks.
> @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
>  	 * applicable for such threads.  This prevents checking for success of
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> -	 * be changed.
> +	 * be changed.  We also disallow attaching kthreadd, to prevent it's
> +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
>  		return -EINVAL;
>  
>  	return 0;
> 

[patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Mon, 2011-10-10 at 07:34 +0200, Mike Galbraith wrote:
> Maybe the below (which seems to have been stillborn) should be done to
> cgroups as well.  Postmortem: kthreadd is attached to a cgroup with no
> rt_runtime allocated, gives birth to severely handicapped kstop threads,
> humongous crash dump follows.
> 
> Fiddling with kthreadd is user error, but since it makes no sense to
> move the thing, why not just say no, and save the user's toes some
> needless wear and tear.
> 
> > If cpusets doesn't want to let PF_THREAD_BOUND threads out, how about
> > cpusets not letting userland scripts attach kthreadd instead?
> > 
> > cpusets: disallow moving kthreadd into a cpuset.

So how about this, both dirt simple and effective.

cpusets, cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <efault@gmx.de>

---
 kernel/cpuset.c |    6 ++++--
 kernel/sched.c  |    9 +++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

Index: linux-3.0-tip/kernel/cpuset.c
===================================================================
--- linux-3.0-tip.orig/kernel/cpuset.c
+++ linux-3.0-tip/kernel/cpuset.c
@@ -59,6 +59,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/cgroup.h>
+#include <linux/kthread.h>
 
 /*
  * Workqueue for cpuset related tasks.
@@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgro
 	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
 	 * applicable for such threads.  This prevents checking for success of
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
-	 * be changed.
+	 * be changed.  We also disallow attaching kthreadd, to prevent it's
+	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
 		return -EINVAL;
 
 	return 0;
Index: linux-3.0-tip/kernel/sched.c
===================================================================
--- linux-3.0-tip.orig/kernel/sched.c
+++ linux-3.0-tip/kernel/sched.c
@@ -9132,6 +9132,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
 static int
 cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 {
+	/*
+	 * kthreadd can fork workers for an RT workqueue in a cgroup
+	 * which may or may not have rt_runtime allocated.  Just say no,
+	 * as attaching a global resource to a non-root group  doesn't
+	 * make any sense anyway.
+	 */
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 #ifdef CONFIG_RT_GROUP_SCHED
 	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
 		return -EINVAL;

Re: [patch] cpusets, cgroups: disallow attaching kthreadd

[Tejun Heo]

Hello,

(cc'ing Thomas and Steven and quoting whole body)

On Mon, Oct 10, 2011 at 10:03:34AM +0200, Mike Galbraith wrote:
> On Mon, 2011-10-10 at 07:34 +0200, Mike Galbraith wrote:
> > Maybe the below (which seems to have been stillborn) should be done to
> > cgroups as well.  Postmortem: kthreadd is attached to a cgroup with no
> > rt_runtime allocated, gives birth to severely handicapped kstop threads,
> > humongous crash dump follows.
> > 
> > Fiddling with kthreadd is user error, but since it makes no sense to
> > move the thing, why not just say no, and save the user's toes some
> > needless wear and tear.
> > 
> > > If cpusets doesn't want to let PF_THREAD_BOUND threads out, how about
> > > cpusets not letting userland scripts attach kthreadd instead?
> > > 
> > > cpusets: disallow moving kthreadd into a cpuset.
> 
> So how about this, both dirt simple and effective.
> 
> cpusets, cgroups: disallow attaching kthreadd
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>

Yes, I think we need something like this.  wq workers were using
PF_THREAD_BOUND to prevent diddling from userland which made some
unhappy.  Maybe we need a flag to properly indicate "don't diddle with
this thread from userland"?  But, then, mainline kernel wouldn't need
the current PF_THREAD_BOUND at all.  Peter, Steven, what do you think?

Thanks.

> ---
>  kernel/cpuset.c |    6 ++++--
>  kernel/sched.c  |    9 +++++++++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> Index: linux-3.0-tip/kernel/cpuset.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/cpuset.c
> +++ linux-3.0-tip/kernel/cpuset.c
> @@ -59,6 +59,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/cgroup.h>
> +#include <linux/kthread.h>
>  
>  /*
>   * Workqueue for cpuset related tasks.
> @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgro
>  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
>  	 * applicable for such threads.  This prevents checking for success of
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> -	 * be changed.
> +	 * be changed.  We also disallow attaching kthreadd, to prevent it's
> +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
>  		return -EINVAL;
>  
>  	return 0;
> Index: linux-3.0-tip/kernel/sched.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/sched.c
> +++ linux-3.0-tip/kernel/sched.c
> @@ -9132,6 +9132,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
>  static int
>  cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  {
> +	/*
> +	 * kthreadd can fork workers for an RT workqueue in a cgroup
> +	 * which may or may not have rt_runtime allocated.  Just say no,
> +	 * as attaching a global resource to a non-root group  doesn't
> +	 * make any sense anyway.
> +	 */
> +	if (tsk == kthreadd_task)
> +		return -EINVAL;
> +
>  #ifdef CONFIG_RT_GROUP_SCHED
>  	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
>  		return -EINVAL;
> 
> 

-- 
tejun

Re: [patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Mon, 2011-10-10 at 09:43 -0700, Tejun Heo wrote:

> Yes, I think we need something like this.  wq workers were using
> PF_THREAD_BOUND to prevent diddling from userland which made some
> unhappy.  Maybe we need a flag to properly indicate "don't diddle with
> this thread from userland"?  But, then, mainline kernel wouldn't need
> the current PF_THREAD_BOUND at all.  Peter, Steven, what do you think?

Yeah, I was also thinking that there should really be a generic "I'm not
yours to play with" flag.  This just closes two cases I've run into
regarding kthreadd, there may be others where the user needs to know
implementation details to not shoot self in foot.

	-Mike

Re: [patch] cpusets, cgroups: disallow attaching kthreadd

[Peter Zijlstra]

On Mon, 2011-10-10 at 09:43 -0700, Tejun Heo wrote:

> Yes, I think we need something like this.  wq workers were using
> PF_THREAD_BOUND to prevent diddling from userland which made some
> unhappy. 

But that can be properly fixed.

>  Maybe we need a flag to properly indicate "don't diddle with
> this thread from userland"?  But, then, mainline kernel wouldn't need
> the current PF_THREAD_BOUND at all.  Peter, Steven, what do you think?

Strict per-cpu affinity that is needed for correctness and disallows
sched_setaffinity() is something entirely different from not being
allowed to put something in a cgroup.

As to not allowing to put in a cgroup thing, is there anything other
than kthreadd for which we need to enforce that? So far I've mostly
treated it like: root can do stupid things, this is one of them, don't
do that then.

I don't think its horribly bad to change the affinity mask of kthreadd,
if a sibling kthread needs a specific affinity it should set that and
override whatever it inherits. If it runs with the default, its a neat
way of setting that.

Re: [patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2011-10-11 at 16:08 +0200, Peter Zijlstra wrote:
> On Mon, 2011-10-10 at 09:43 -0700, Tejun Heo wrote:
> 
> > Yes, I think we need something like this.  wq workers were using
> > PF_THREAD_BOUND to prevent diddling from userland which made some
> > unhappy. 
> 
> But that can be properly fixed.
> 
> >  Maybe we need a flag to properly indicate "don't diddle with
> > this thread from userland"?  But, then, mainline kernel wouldn't need
> > the current PF_THREAD_BOUND at all.  Peter, Steven, what do you think?
> 
> Strict per-cpu affinity that is needed for correctness and disallows
> sched_setaffinity() is something entirely different from not being
> allowed to put something in a cgroup.

Agreed.  The proposed patchlet is purely a practical matter.

> As to not allowing to put in a cgroup thing, is there anything other
> than kthreadd for which we need to enforce that? So far I've mostly
> treated it like: root can do stupid things, this is one of them, don't
> do that then.

Yeah, that's the other side of the coin.  The only thing I can think of
justifying a response other than "well, gee, don't do that" is that Joe
User shouldn't need to know or care about kernel workqueue details.

I don't know of anything other than kthreadd where moving it here, or
over yonder matters one bit.

	-Mike

Re: [patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Tue, 11 Oct 2011, Mike Galbraith wrote:

> I don't know of anything other than kthreadd where moving it here, or
> over yonder matters one bit.
> 

Clear PF_THREAD_BOUND for watchdog or migration kthreads and change their 
affinity or move them into a cpuset where cpuset.cpus doesn't include the 
cpu they are bound to and it'll be obvious.

Re: [patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2011-10-11 at 18:22 -0700, David Rientjes wrote:
> On Tue, 11 Oct 2011, Mike Galbraith wrote:
> 
> > I don't know of anything other than kthreadd where moving it here, or
> > over yonder matters one bit.
> > 
> 
> Clear PF_THREAD_BOUND for watchdog or migration kthreads and change their 
> affinity or move them into a cpuset where cpuset.cpus doesn't include the 
> cpu they are bound to and it'll be obvious.

Sure, I mean where Joe User can trip over an existing stumbling block.

	-Mike

Re: [patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Tue, 11 Oct 2011, Peter Zijlstra wrote:

> >  Maybe we need a flag to properly indicate "don't diddle with
> > this thread from userland"?  But, then, mainline kernel wouldn't need
> > the current PF_THREAD_BOUND at all.  Peter, Steven, what do you think?
> 
> Strict per-cpu affinity that is needed for correctness and disallows
> sched_setaffinity() is something entirely different from not being
> allowed to put something in a cgroup.
> 

Right, I introduced PF_THREAD_BOUND specifically so userspace could not 
change the set of allowed cpus of a kthread that has used kthread_bind() 
for specific affinity.  Any other kthread could set that flag directly to 
avoid the same tampering.

The flag was extended for cpusets to avoid having the same kthreads being 
moved out of the root cpuset since cpuset.cpus can be changed for all non-
root cpusets to be disjoint.  We wanted to avoid an inconsistency where 
threads attached to a cpuset had disjoint cpumasks; all threads attached 
to a cpuset should have cpumasks that are a subset of cpuset.cpus.

> As to not allowing to put in a cgroup thing, is there anything other
> than kthreadd for which we need to enforce that? So far I've mostly
> treated it like: root can do stupid things, this is one of them, don't
> do that then.
> 

Certainly the stop machine migration kthreads and per-cpu watchdog 
threads shouldn't be moved out of the root cpuset.  Both get 
PF_THREAD_BOUND from calling kthread_bind().

patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Mon, 2011-10-10 at 10:03 +0200, Mike Galbraith wrote:
> On Mon, 2011-10-10 at 07:34 +0200, Mike Galbraith wrote:
> > Maybe the below (which seems to have been stillborn) should be done to
> > cgroups as well.  Postmortem: kthreadd is attached to a cgroup with no
> > rt_runtime allocated, gives birth to severely handicapped kstop threads,
> > humongous crash dump follows.
> > 
> > Fiddling with kthreadd is user error, but since it makes no sense to
> > move the thing, why not just say no, and save the user's toes some
> > needless wear and tear.
> > 
> > > If cpusets doesn't want to let PF_THREAD_BOUND threads out, how about
> > > cpusets not letting userland scripts attach kthreadd instead?
> > > 
> > > cpusets: disallow moving kthreadd into a cpuset.
> 
> So how about this, both dirt simple and effective.

Ahem, the buildable copy..

I've been asked to add this toe saver to the trees I work on at least
until something better comes along.  ACK/NACK equally understandable
given the infrequency of the toe shooting, and user's right to do so.

Off you go, fly or die little patchlet.

cpusets, cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <efault@gmx.de>

---
 kernel/cpuset.c |    6 ++++--
 kernel/sched.c  |   10 ++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

Index: linux-3.0-tip/kernel/cpuset.c
===================================================================
--- linux-3.0-tip.orig/kernel/cpuset.c
+++ linux-3.0-tip/kernel/cpuset.c
@@ -59,6 +59,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/cgroup.h>
+#include <linux/kthread.h>
 
 /*
  * Workqueue for cpuset related tasks.
@@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgro
 	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
 	 * applicable for such threads.  This prevents checking for success of
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
-	 * be changed.
+	 * be changed.  We also disallow attaching kthreadd, to prevent it's
+	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
 		return -EINVAL;
 
 	return 0;
Index: linux-3.0-tip/kernel/sched.c
===================================================================
--- linux-3.0-tip.orig/kernel/sched.c
+++ linux-3.0-tip/kernel/sched.c
@@ -71,6 +71,7 @@
 #include <linux/ctype.h>
 #include <linux/ftrace.h>
 #include <linux/slab.h>
+#include <linux/kthread.h>
 
 #include <asm/tlb.h>
 #include <asm/irq_regs.h>
@@ -9141,6 +9142,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
 static int
 cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 {
+	/*
+	 * kthreadd can fork workers for an RT workqueue in a cgroup
+	 * which may or may not have rt_runtime allocated.  Just say no,
+	 * as attaching a global resource to a non-root group  doesn't
+	 * make any sense anyway.
+	 */
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 #ifdef CONFIG_RT_GROUP_SCHED
 	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
 		return -EINVAL;

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Tue, 18 Oct 2011, Mike Galbraith wrote:

> Ahem, the buildable copy..
> 
> I've been asked to add this toe saver to the trees I work on at least
> until something better comes along.  ACK/NACK equally understandable
> given the infrequency of the toe shooting, and user's right to do so.
> 
> Off you go, fly or die little patchlet.
> 

Heh.

> cpusets, cgroups: disallow attaching kthreadd
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>

Acked-by: David Rientjes <rientjes@google.com>

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Peter Zijlstra]

On Tue, 2011-10-18 at 10:10 +0200, Mike Galbraith wrote:

> ---
>  kernel/cpuset.c |    6 ++++--
>  kernel/sched.c  |   10 ++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> Index: linux-3.0-tip/kernel/cpuset.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/cpuset.c
> +++ linux-3.0-tip/kernel/cpuset.c
> @@ -59,6 +59,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/cgroup.h>
> +#include <linux/kthread.h>
>  
>  /*
>   * Workqueue for cpuset related tasks.
> @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgro
>  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
>  	 * applicable for such threads.  This prevents checking for success of
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> -	 * be changed.
> +	 * be changed.  We also disallow attaching kthreadd, to prevent it's
> +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
>  		return -EINVAL;
>  
>  	return 0;
> Index: linux-3.0-tip/kernel/sched.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/sched.c
> +++ linux-3.0-tip/kernel/sched.c
> @@ -71,6 +71,7 @@
>  #include <linux/ctype.h>
>  #include <linux/ftrace.h>
>  #include <linux/slab.h>
> +#include <linux/kthread.h>
>  
>  #include <asm/tlb.h>
>  #include <asm/irq_regs.h>
> @@ -9141,6 +9142,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
>  static int
>  cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  {
> +	/*
> +	 * kthreadd can fork workers for an RT workqueue in a cgroup
> +	 * which may or may not have rt_runtime allocated.  Just say no,
> +	 * as attaching a global resource to a non-root group  doesn't
> +	 * make any sense anyway.
> +	 */
> +	if (tsk == kthreadd_task)
> +		return -EINVAL;
> +
>  #ifdef CONFIG_RT_GROUP_SCHED
>  	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
>  		return -EINVAL;
> 
> 

Why special case these two, why not simply disallow moving kthreadd into
_any_ cgroup what so ever and be done with it?

---
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 453100a..18eed58 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1824,6 +1824,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 	struct cgroup *oldcgrp;
 	struct cgroupfs_root *root = cgrp->root;
 
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 	/* Nothing to do if the task is already in that cgroup */
 	oldcgrp = task_cgroup_from_root(tsk, root);
 	if (cgrp == oldcgrp)

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2011-10-18 at 10:42 +0200, Peter Zijlstra wrote:

> Why special case these two, why not simply disallow moving kthreadd into
> _any_ cgroup what so ever and be done with it?

These were the cases where it hurt, and cpusets don't have to be a
component of a cgroup.  So we'd still need the cpuset bit.

> 
> ---
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 453100a..18eed58 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1824,6 +1824,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  	struct cgroup *oldcgrp;
>  	struct cgroupfs_root *root = cgrp->root;
>  
> +	if (tsk == kthreadd_task)
> +		return -EINVAL;
> +
>  	/* Nothing to do if the task is already in that cgroup */
>  	oldcgrp = task_cgroup_from_root(tsk, root);
>  	if (cgrp == oldcgrp)
> 

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Peter Zijlstra]

On Tue, 2011-10-18 at 10:47 +0200, Mike Galbraith wrote:
> On Tue, 2011-10-18 at 10:42 +0200, Peter Zijlstra wrote:
> 
> > Why special case these two, why not simply disallow moving kthreadd into
> > _any_ cgroup what so ever and be done with it?
> 
> These were the cases where it hurt, and cpusets don't have to be a
> component of a cgroup.  So we'd still need the cpuset bit.

but but but,, cpusetes are a cgroup thingy?!

> > 
> > ---
> > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > index 453100a..18eed58 100644
> > --- a/kernel/cgroup.c
> > +++ b/kernel/cgroup.c
> > @@ -1824,6 +1824,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> >  	struct cgroup *oldcgrp;
> >  	struct cgroupfs_root *root = cgrp->root;
> >  
> > +	if (tsk == kthreadd_task)
> > +		return -EINVAL;
> > +
> >  	/* Nothing to do if the task is already in that cgroup */
> >  	oldcgrp = task_cgroup_from_root(tsk, root);
> >  	if (cgrp == oldcgrp)
> > 
> 
> 

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2011-10-18 at 11:06 +0200, Peter Zijlstra wrote:
> On Tue, 2011-10-18 at 10:47 +0200, Mike Galbraith wrote:
> > On Tue, 2011-10-18 at 10:42 +0200, Peter Zijlstra wrote:
> > 
> > > Why special case these two, why not simply disallow moving kthreadd into
> > > _any_ cgroup what so ever and be done with it?
> > 
> > These were the cases where it hurt, and cpusets don't have to be a
> > component of a cgroup.  So we'd still need the cpuset bit.
> 
> but but but,, cpusetes are a cgroup thingy?!

By golly, mount -t cpuset and the thing does say type cgroup.  How 'bout
that, learn something new every day.  Dinkier is better.

> > > ---
> > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > > index 453100a..18eed58 100644
> > > --- a/kernel/cgroup.c
> > > +++ b/kernel/cgroup.c
> > > @@ -1824,6 +1824,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> > >  	struct cgroup *oldcgrp;
> > >  	struct cgroupfs_root *root = cgrp->root;
> > >  
> > > +	if (tsk == kthreadd_task)
> > > +		return -EINVAL;
> > > +
> > >  	/* Nothing to do if the task is already in that cgroup */
> > >  	oldcgrp = task_cgroup_from_root(tsk, root);
> > >  	if (cgrp == oldcgrp)
> > > 
> > 
> > 
> 

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2011-10-18 at 11:23 +0200, Mike Galbraith wrote:
> On Tue, 2011-10-18 at 11:06 +0200, Peter Zijlstra wrote:
> > On Tue, 2011-10-18 at 10:47 +0200, Mike Galbraith wrote:
> > > On Tue, 2011-10-18 at 10:42 +0200, Peter Zijlstra wrote:
> > > 
> > > > Why special case these two, why not simply disallow moving kthreadd into
> > > > _any_ cgroup what so ever and be done with it?
> > > 
> > > These were the cases where it hurt, and cpusets don't have to be a
> > > component of a cgroup.  So we'd still need the cpuset bit.
> > 
> > but but but,, cpusetes are a cgroup thingy?!
> 
> By golly, mount -t cpuset and the thing does say type cgroup.  How 'bout
> that, learn something new every day.  Dinkier is better.

From: Peter Zijlstra <a.p.zijlstra@chello.nl>

cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <efault@gmx.de>

---
 kernel/cgroup.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-3.0-tip/kernel/cgroup.c
===================================================================
--- linux-3.0-tip.orig/kernel/cgroup.c
+++ linux-3.0-tip/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/kthread.h>
 
 #include <linux/atomic.h>
 
@@ -1824,6 +1825,14 @@ int cgroup_attach_task(struct cgroup *cg
 	struct cgroup *oldcgrp;
 	struct cgroupfs_root *root = cgrp->root;
 
+	/*
+	 * Workqueue threads may acquire PF_THREAD_BOUND and become
+	 * trapped in a cpuset, or RT worker may be born in a cgroup
+	 * with no rt_runtime allocated.  Just say no.
+	 */
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 	/* Nothing to do if the task is already in that cgroup */
 	oldcgrp = task_cgroup_from_root(tsk, root);
 	if (cgrp == oldcgrp)

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Tue, 18 Oct 2011, Mike Galbraith wrote:

> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> cgroups: disallow attaching kthreadd
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 

Eek, this seems like complete overkill, there may be cgroups now or in the 
future that it would be perfectly acceptable to move kthreadd into without 
any negative effects.  There's no reason to restrict it this broadly, 
that's why I thought your change to cpusets was fine and acked it.  It's 
the perfect candidate for what the ->can_attach() pointer for a cgroup can 
identify is problematic for _that_ cgroup.

> Save the user some unexpected trouble, just say no.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> 

A patch from Peter that only has your sign-off?

> ---
>  kernel/cgroup.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> Index: linux-3.0-tip/kernel/cgroup.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/cgroup.c
> +++ linux-3.0-tip/kernel/cgroup.c
> @@ -60,6 +60,7 @@
>  #include <linux/eventfd.h>
>  #include <linux/poll.h>
>  #include <linux/flex_array.h> /* used in cgroup_attach_proc */
> +#include <linux/kthread.h>
>  
>  #include <linux/atomic.h>
>  
> @@ -1824,6 +1825,14 @@ int cgroup_attach_task(struct cgroup *cg
>  	struct cgroup *oldcgrp;
>  	struct cgroupfs_root *root = cgrp->root;
>  
> +	/*
> +	 * Workqueue threads may acquire PF_THREAD_BOUND and become
> +	 * trapped in a cpuset, or RT worker may be born in a cgroup
> +	 * with no rt_runtime allocated.  Just say no.
> +	 */
> +	if (tsk == kthreadd_task)
> +		return -EINVAL;
> +
>  	/* Nothing to do if the task is already in that cgroup */
>  	oldcgrp = task_cgroup_from_root(tsk, root);
>  	if (cgrp == oldcgrp)
> 
> 
> 

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2011-10-18 at 13:38 -0700, David Rientjes wrote:
> On Tue, 18 Oct 2011, Mike Galbraith wrote:
> 
> > From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > 
> > cgroups: disallow attaching kthreadd
> > 
> > Allowing kthreadd to be moved to a non-root group makes no sense, it being
> > a global resource, and needlessly leads unsuspecting users toward trouble.
> > 
> > 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> > allocated is not schedulable.  Simple user error, but harmful to the box.
> > 
> > 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> > rendering the cpuset immortal.
> > 
> 
> Eek, this seems like complete overkill, there may be cgroups now or in the 
> future that it would be perfectly acceptable to move kthreadd into without 
> any negative effects.  There's no reason to restrict it this broadly, 
> that's why I thought your change to cpusets was fine and acked it.  It's 
> the perfect candidate for what the ->can_attach() pointer for a cgroup can 
> identify is problematic for _that_ cgroup.

Oh my, I can't win.

Either of the two patchlets will do the trick, so maintainer's choice.

> > Save the user some unexpected trouble, just say no.
> > 
> > Signed-off-by: Mike Galbraith <efault@gmx.de>
> > 
> 
> A patch from Peter that only has your sign-off?

The only thing in it from me was a comment, figured I should post a from
line as a matter of propriety.  Guess not.  If this variant is the one
that flies, the from line can be dropped.  I doubt Peter counts his
credits, and even if he did, he wouldn't notice _this one_ missing :)

	-Mike

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Paul Menage]

On Tue, Oct 18, 2011 at 9:00 PM, Mike Galbraith <efault@gmx.de> wrote:
>
> Oh my, I can't win.
>
> Either of the two patchlets will do the trick, so maintainer's choice.
>

I agree with your original idea - if it's just some cgroup subsystems
that are unsafe to have kthreadd in, then they can make that choice.

Otherwise you restrict things unnecessarily. E.g. a systemd-alike
might have a tracking hierarchy (without any subsystems attached,
potentially) and want to keep the top-level empty and have all kernel
threads their own cgroup. There's absolutely no problem with that.

I can see the argument that the number of use-cases for this is fairly
small, but in line with the principle of keeping the ABI as stable as
possible in the absence of bugs that force a change, having the
restriction in the necessary subsystems seems cleaner.

Paul

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Paul Menage]

On Tue, Oct 18, 2011 at 9:53 PM, Paul Menage <paul@paulmenage.org> wrote:
>
> I can see the argument that the number of use-cases for this is fairly
> small, but in line with the principle of keeping the ABI as stable as
> possible in the absence of bugs that force a change, having the
> restriction in the necessary subsystems seems cleaner.
>

Separately, an alternative fix for the cpusets case be to make it
always possible to move a thread to the root cpuset, even if it's
PF_THREAD_BOUND?

Paul

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2011-10-18 at 21:56 -0700, Paul Menage wrote:
> On Tue, Oct 18, 2011 at 9:53 PM, Paul Menage <paul@paulmenage.org> wrote:
> >
> > I can see the argument that the number of use-cases for this is fairly
> > small, but in line with the principle of keeping the ABI as stable as
> > possible in the absence of bugs that force a change, having the
> > restriction in the necessary subsystems seems cleaner.
> >
> 
> Separately, an alternative fix for the cpusets case be to make it
> always possible to move a thread to the root cpuset, even if it's
> PF_THREAD_BOUND?

Yeah, an escape hatch is what I did first.

https://lkml.org/lkml/2011/9/23/14

	-Mike

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Peter Zijlstra]

On Tue, 2011-10-18 at 13:38 -0700, David Rientjes wrote:
> 
> Eek, this seems like complete overkill, there may be cgroups now or in the 
> future that it would be perfectly acceptable to move kthreadd into without 
> any negative effects.  There's no reason to restrict it this broadly, 
> that's why I thought your change to cpusets was fine and acked it.  It's 
> the perfect candidate for what the ->can_attach() pointer for a cgroup can 
> identify is problematic for _that_ cgroup. 

My thinking is that kthreadd is the mother of all kernel threads and the
kernel assumes it can spawn kthreads without constraints, a valid
assumption IMO.

Therefore the kthreadd thread should live in the root cgroup at all
times, irrespective of whatever controllers are or aren't actually safe.

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Wed, 19 Oct 2011, Peter Zijlstra wrote:

> My thinking is that kthreadd is the mother of all kernel threads and the
> kernel assumes it can spawn kthreads without constraints, a valid
> assumption IMO.
> 

Cgroups don't necessarily imply constraints, though, you could devise a 
cgroup to just do monitoring or statistics tracking for an aggregate of 
tasks and placing kthreadd in such a cgroup would make perfect sense 
because then, since children are forked in the same cgroup, you can 
monitor or gather statistics for all kthreads.  This can be your only 
cgroup on the system.

Cpusets, though, does imply cpu constraints, which is why we decline 
PF_THREAD_BOUND threads from moving in the first place, which is the 
source of Mike's issue.  It's can_attach() function can explicitly decline 
kthreadd as well since the cpu constraints of both types of threads should 
never be altered by either cpusets or sched_setaffinity().

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Peter Zijlstra]

On Wed, 2011-10-19 at 12:47 -0700, David Rientjes wrote:
> On Wed, 19 Oct 2011, Peter Zijlstra wrote:
> 
> > My thinking is that kthreadd is the mother of all kernel threads and the
> > kernel assumes it can spawn kthreads without constraints, a valid
> > assumption IMO.
> > 
> 
> Cgroups don't necessarily imply constraints,

And yet they're called: Control Groups..

>  though, you could devise a 
> cgroup to just do monitoring or statistics tracking for an aggregate of 
> tasks and placing kthreadd in such a cgroup would make perfect sense 
> because then, since children are forked in the same cgroup, you can 
> monitor or gather statistics for all kthreads.  This can be your only 
> cgroup on the system.

I guess you could, but does it really make sense? Also, you could sort
this by extending the cgroup interface to explicitly distinct between
controllers and !controllers.

> Cpusets, though, does imply cpu constraints, which is why we decline 
> PF_THREAD_BOUND threads from moving in the first place, which is the 
> source of Mike's issue.  It's can_attach() function can explicitly decline 
> kthreadd as well since the cpu constraints of both types of threads should 
> never be altered by either cpusets or sched_setaffinity().

Yeah, and I'm saying we want to exclude _all_ controllers from placing
constraints on kthreadd, even those that might not immediately break
stuff.

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Thu, 20 Oct 2011, Peter Zijlstra wrote:

> >  though, you could devise a 
> > cgroup to just do monitoring or statistics tracking for an aggregate of 
> > tasks and placing kthreadd in such a cgroup would make perfect sense 
> > because then, since children are forked in the same cgroup, you can 
> > monitor or gather statistics for all kthreads.  This can be your only 
> > cgroup on the system.
> 
> I guess you could, but does it really make sense? Also, you could sort
> this by extending the cgroup interface to explicitly distinct between
> controllers and !controllers.
> 

I don't know what the definition of "controllers" is that would separate 
the two, that's why it's left up to the individual cgroups to define their 
own can_attach() function to ensure that the admin isn't doing something 
potentially harmful.

In terms of controlling the set of allowed nodes, we certainly want to 
prohibit PF_THREAD_BOUND threads for cpusets because cpusets can easily 
change that (and spews a nasty warning if set_cpus_allowed_ptr() fails).

In terms of controlling kthreadd forking threads that become stuck with 
PF_THREAD_BOUND, I think Mike's patch is correct for cpusets, since we 
know we don't want its children to become trapped.  All other cgroups, 
unless they have a similar exemption for PF_THREAD_BOUND threads, would 
allow the child to be reassigned by their can_attach() function.  A grep 
for PF_THREAD_BOUND shows no others.

Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Paul Menage]

On Tue, Oct 18, 2011 at 1:10 AM, Mike Galbraith <efault@gmx.de> wrote:
>
> Save the user some unexpected trouble, just say no.
>
> Signed-off-by: Mike Galbraith <efault@gmx.de>

Acked-by: Paul Menage <paul@paulmenage.org>

>
> ---
>  kernel/cpuset.c |    6 ++++--
>  kernel/sched.c  |   10 ++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> Index: linux-3.0-tip/kernel/cpuset.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/cpuset.c
> +++ linux-3.0-tip/kernel/cpuset.c
> @@ -59,6 +59,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/cgroup.h>
> +#include <linux/kthread.h>
>
>  /*
>  * Workqueue for cpuset related tasks.
> @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgro
>         * set of allowed nodes is unnecessary.  Thus, cpusets are not
>         * applicable for such threads.  This prevents checking for success of
>         * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> -        * be changed.
> +        * be changed.  We also disallow attaching kthreadd, to prevent it's
> +        * child from becoming trapped should it then acquire PF_THREAD_BOUND.
>         */
> -       if (tsk->flags & PF_THREAD_BOUND)
> +       if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
>                return -EINVAL;
>
>        return 0;
> Index: linux-3.0-tip/kernel/sched.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/sched.c
> +++ linux-3.0-tip/kernel/sched.c
> @@ -71,6 +71,7 @@
>  #include <linux/ctype.h>
>  #include <linux/ftrace.h>
>  #include <linux/slab.h>
> +#include <linux/kthread.h>
>
>  #include <asm/tlb.h>
>  #include <asm/irq_regs.h>
> @@ -9141,6 +9142,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
>  static int
>  cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  {
> +       /*
> +        * kthreadd can fork workers for an RT workqueue in a cgroup
> +        * which may or may not have rt_runtime allocated.  Just say no,
> +        * as attaching a global resource to a non-root group  doesn't
> +        * make any sense anyway.
> +        */
> +       if (tsk == kthreadd_task)
> +               return -EINVAL;
> +
>  #ifdef CONFIG_RT_GROUP_SCHED
>        if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
>                return -EINVAL;
>
>
>

[patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2011-10-18 at 21:57 -0700, Paul Menage wrote:
> On Tue, Oct 18, 2011 at 1:10 AM, Mike Galbraith <efault@gmx.de> wrote:
> >
> > Save the user some unexpected trouble, just say no.
> >
> > Signed-off-by: Mike Galbraith <efault@gmx.de>
> 
> Acked-by: Paul Menage <paul@paulmenage.org>

Thanks.  Ok, seems it is decided that this is the best way to deal with
annoying corner-case.  For convenience, patch with acquired acks below.


cpusets, cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Paul Menage <paul@paulmenage.org>

---
 kernel/cpuset.c |    6 ++++--
 kernel/sched.c  |   10 ++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

Index: linux-3.0-tip/kernel/cpuset.c
===================================================================
--- linux-3.0-tip.orig/kernel/cpuset.c
+++ linux-3.0-tip/kernel/cpuset.c
@@ -59,6 +59,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/cgroup.h>
+#include <linux/kthread.h>
 
 /*
  * Workqueue for cpuset related tasks.
@@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgro
 	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
 	 * applicable for such threads.  This prevents checking for success of
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
-	 * be changed.
+	 * be changed.  We also disallow attaching kthreadd, to prevent it's
+	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
 		return -EINVAL;
 
 	return 0;
Index: linux-3.0-tip/kernel/sched.c
===================================================================
--- linux-3.0-tip.orig/kernel/sched.c
+++ linux-3.0-tip/kernel/sched.c
@@ -71,6 +71,7 @@
 #include <linux/ctype.h>
 #include <linux/ftrace.h>
 #include <linux/slab.h>
+#include <linux/kthread.h>
 
 #include <asm/tlb.h>
 #include <asm/irq_regs.h>
@@ -9141,6 +9142,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
 static int
 cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 {
+	/*
+	 * kthreadd can fork workers for an RT workqueue in a cgroup
+	 * which may or may not have rt_runtime allocated.  Just say no,
+	 * as attaching a global resource to a non-root group  doesn't
+	 * make any sense anyway.
+	 */
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 #ifdef CONFIG_RT_GROUP_SCHED
 	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
 		return -EINVAL;

Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Peter Zijlstra]

On Wed, 2011-10-19 at 07:24 +0200, Mike Galbraith wrote:
> Thanks.  Ok, seems it is decided that this is the best way to deal with
> annoying corner-case.  For convenience, patch with acquired acks below.
> 

Ah, but I'm not rolling over that easily ;-)

I really think that if we want to restrain userspace from doing
something stupid we might as well do something that makes sense, and
that is mandate kthreadd stays in the root group at all times for
everybody.

Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Paul Menage]

On Wed, Oct 19, 2011 at 12:54 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> I really think that if we want to restrain userspace from doing
> something stupid we might as well do something that makes sense,

Totally agreed. There are two solutions proposed, both of which make
sense as a method of solving the problem, and which have different
levels of effect on the existing user-visible API. So which do we
prefer?

If you just want to make sure people think about it before there's a
problem rather than after, then an alternative would be to have a flag
in cgroup_subsys such as allow_kthreadd, which defaults to false and
can be explicitly set to true for subsystems that are kthreadd-safe.

Paul

Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Wed, 2011-10-19 at 09:54 +0200, Peter Zijlstra wrote:
> On Wed, 2011-10-19 at 07:24 +0200, Mike Galbraith wrote:
> > Thanks.  Ok, seems it is decided that this is the best way to deal with
> > annoying corner-case.  For convenience, patch with acquired acks below.
> > 
> 
> Ah, but I'm not rolling over that easily ;-)

I'm easy.  I committed the global "thou shalt not fiddle with kthreadd"
variant yesterday, and shall wait, quiet like little mouse, for which
ever variant flies.  I bet a nickle I never hear from bugzilla again
even if I forget to replace it with which ever one takes wing :)

	-Mike

Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Wed, 19 Oct 2011, Mike Galbraith wrote:

> cpusets, cgroups: disallow attaching kthreadd
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> Acked-by: David Rientjes <rientjes@google.com>
> Acked-by: Paul Menage <paul@paulmenage.org>
> 

Let's add Andrew to the cc so we can get it in -mm, I haven't seen it hit 
linux-next yet.

> ---
>  kernel/cpuset.c |    6 ++++--
>  kernel/sched.c  |   10 ++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> Index: linux-3.0-tip/kernel/cpuset.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/cpuset.c
> +++ linux-3.0-tip/kernel/cpuset.c
> @@ -59,6 +59,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/cgroup.h>
> +#include <linux/kthread.h>
>  
>  /*
>   * Workqueue for cpuset related tasks.
> @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgro
>  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
>  	 * applicable for such threads.  This prevents checking for success of
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> -	 * be changed.
> +	 * be changed.  We also disallow attaching kthreadd, to prevent it's
> +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
>  		return -EINVAL;
>  
>  	return 0;
> Index: linux-3.0-tip/kernel/sched.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/sched.c
> +++ linux-3.0-tip/kernel/sched.c
> @@ -71,6 +71,7 @@
>  #include <linux/ctype.h>
>  #include <linux/ftrace.h>
>  #include <linux/slab.h>
> +#include <linux/kthread.h>
>  
>  #include <asm/tlb.h>
>  #include <asm/irq_regs.h>
> @@ -9141,6 +9142,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
>  static int
>  cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  {
> +	/*
> +	 * kthreadd can fork workers for an RT workqueue in a cgroup
> +	 * which may or may not have rt_runtime allocated.  Just say no,
> +	 * as attaching a global resource to a non-root group  doesn't
> +	 * make any sense anyway.
> +	 */
> +	if (tsk == kthreadd_task)
> +		return -EINVAL;
> +
>  #ifdef CONFIG_RT_GROUP_SCHED
>  	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
>  		return -EINVAL;

Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Wed, 26 Oct 2011, David Rientjes wrote:

> On Wed, 19 Oct 2011, Mike Galbraith wrote:
> 
> > cpusets, cgroups: disallow attaching kthreadd
> > 
> > Allowing kthreadd to be moved to a non-root group makes no sense, it being
> > a global resource, and needlessly leads unsuspecting users toward trouble.
> > 
> > 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> > allocated is not schedulable.  Simple user error, but harmful to the box.
> > 
> > 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> > rendering the cpuset immortal.
> > 
> > Save the user some unexpected trouble, just say no.
> > 
> > Signed-off-by: Mike Galbraith <efault@gmx.de>
> > Acked-by: David Rientjes <rientjes@google.com>
> > Acked-by: Paul Menage <paul@paulmenage.org>
> > 
> 
> Let's add Andrew to the cc so we can get it in -mm, I haven't seen it hit 
> linux-next yet.
> 

Ping?  Still missing from -mm and linux-next.

> > ---
> >  kernel/cpuset.c |    6 ++++--
> >  kernel/sched.c  |   10 ++++++++++
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > Index: linux-3.0-tip/kernel/cpuset.c
> > ===================================================================
> > --- linux-3.0-tip.orig/kernel/cpuset.c
> > +++ linux-3.0-tip/kernel/cpuset.c
> > @@ -59,6 +59,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/workqueue.h>
> >  #include <linux/cgroup.h>
> > +#include <linux/kthread.h>
> >  
> >  /*
> >   * Workqueue for cpuset related tasks.
> > @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgro
> >  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
> >  	 * applicable for such threads.  This prevents checking for success of
> >  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> > -	 * be changed.
> > +	 * be changed.  We also disallow attaching kthreadd, to prevent it's
> > +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
> >  	 */
> > -	if (tsk->flags & PF_THREAD_BOUND)
> > +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
> >  		return -EINVAL;
> >  
> >  	return 0;
> > Index: linux-3.0-tip/kernel/sched.c
> > ===================================================================
> > --- linux-3.0-tip.orig/kernel/sched.c
> > +++ linux-3.0-tip/kernel/sched.c
> > @@ -71,6 +71,7 @@
> >  #include <linux/ctype.h>
> >  #include <linux/ftrace.h>
> >  #include <linux/slab.h>
> > +#include <linux/kthread.h>
> >  
> >  #include <asm/tlb.h>
> >  #include <asm/irq_regs.h>
> > @@ -9141,6 +9142,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
> >  static int
> >  cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> >  {
> > +	/*
> > +	 * kthreadd can fork workers for an RT workqueue in a cgroup
> > +	 * which may or may not have rt_runtime allocated.  Just say no,
> > +	 * as attaching a global resource to a non-root group  doesn't
> > +	 * make any sense anyway.
> > +	 */
> > +	if (tsk == kthreadd_task)
> > +		return -EINVAL;
> > +
> >  #ifdef CONFIG_RT_GROUP_SCHED
> >  	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
> >  		return -EINVAL;

Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Thu, 10 Nov 2011, David Rientjes wrote:

> On Wed, 26 Oct 2011, David Rientjes wrote:
> 
> > On Wed, 19 Oct 2011, Mike Galbraith wrote:
> > 
> > > cpusets, cgroups: disallow attaching kthreadd
> > > 
> > > Allowing kthreadd to be moved to a non-root group makes no sense, it being
> > > a global resource, and needlessly leads unsuspecting users toward trouble.
> > > 
> > > 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> > > allocated is not schedulable.  Simple user error, but harmful to the box.
> > > 
> > > 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> > > rendering the cpuset immortal.
> > > 
> > > Save the user some unexpected trouble, just say no.
> > > 
> > > Signed-off-by: Mike Galbraith <efault@gmx.de>
> > > Acked-by: David Rientjes <rientjes@google.com>
> > > Acked-by: Paul Menage <paul@paulmenage.org>
> > > 
> > 
> > Let's add Andrew to the cc so we can get it in -mm, I haven't seen it hit 
> > linux-next yet.
> > 
> 
> Ping?  Still missing from -mm and linux-next.
> 

Ping #2?

> > > ---
> > >  kernel/cpuset.c |    6 ++++--
> > >  kernel/sched.c  |   10 ++++++++++
> > >  2 files changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > Index: linux-3.0-tip/kernel/cpuset.c
> > > ===================================================================
> > > --- linux-3.0-tip.orig/kernel/cpuset.c
> > > +++ linux-3.0-tip/kernel/cpuset.c
> > > @@ -59,6 +59,7 @@
> > >  #include <linux/mutex.h>
> > >  #include <linux/workqueue.h>
> > >  #include <linux/cgroup.h>
> > > +#include <linux/kthread.h>
> > >  
> > >  /*
> > >   * Workqueue for cpuset related tasks.
> > > @@ -1382,9 +1383,10 @@ static int cpuset_can_attach(struct cgro
> > >  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
> > >  	 * applicable for such threads.  This prevents checking for success of
> > >  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> > > -	 * be changed.
> > > +	 * be changed.  We also disallow attaching kthreadd, to prevent it's
> > > +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
> > >  	 */
> > > -	if (tsk->flags & PF_THREAD_BOUND)
> > > +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
> > >  		return -EINVAL;
> > >  
> > >  	return 0;
> > > Index: linux-3.0-tip/kernel/sched.c
> > > ===================================================================
> > > --- linux-3.0-tip.orig/kernel/sched.c
> > > +++ linux-3.0-tip/kernel/sched.c
> > > @@ -71,6 +71,7 @@
> > >  #include <linux/ctype.h>
> > >  #include <linux/ftrace.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/kthread.h>
> > >  
> > >  #include <asm/tlb.h>
> > >  #include <asm/irq_regs.h>
> > > @@ -9141,6 +9142,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
> > >  static int
> > >  cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> > >  {
> > > +	/*
> > > +	 * kthreadd can fork workers for an RT workqueue in a cgroup
> > > +	 * which may or may not have rt_runtime allocated.  Just say no,
> > > +	 * as attaching a global resource to a non-root group  doesn't
> > > +	 * make any sense anyway.
> > > +	 */
> > > +	if (tsk == kthreadd_task)
> > > +		return -EINVAL;
> > > +
> > >  #ifdef CONFIG_RT_GROUP_SCHED
> > >  	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
> > >  		return -EINVAL;
> 

Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Andrew Morton]

On Tue, 6 Dec 2011 12:13:10 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:

> On Thu, 10 Nov 2011, David Rientjes wrote:
> 
> > On Wed, 26 Oct 2011, David Rientjes wrote:
> > 
> > > On Wed, 19 Oct 2011, Mike Galbraith wrote:
> > > 
> > > > cpusets, cgroups: disallow attaching kthreadd
> > > > 
> > > > Allowing kthreadd to be moved to a non-root group makes no sense, it being
> > > > a global resource, and needlessly leads unsuspecting users toward trouble.
> > > > 
> > > > 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> > > > allocated is not schedulable.  Simple user error, but harmful to the box.
> > > > 
> > > > 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> > > > rendering the cpuset immortal.
> > > > 
> > > > Save the user some unexpected trouble, just say no.
> > > > 
> > > > Signed-off-by: Mike Galbraith <efault@gmx.de>
> > > > Acked-by: David Rientjes <rientjes@google.com>
> > > > Acked-by: Paul Menage <paul@paulmenage.org>
> > > > 
> > > 
> > > Let's add Andrew to the cc so we can get it in -mm, I haven't seen it hit 
> > > linux-next yet.
> > > 
> > 
> > Ping?  Still missing from -mm and linux-next.
> > 
> 
> Ping #2?
> 

Why am I being pinged about scheduler patches?  My sole contribution
to this one is to point out that "its"->possessive and "it's"->"it is".

Also, Peter has said

: I really think that if we want to restrain userspace from doing
: something stupid we might as well do something that makes sense, and
: that is mandate kthreadd stays in the root group at all times for
: everybody.

which appears to be what the patch already did, so I'm confused again.

It's time for a fresh resend, IMO.

Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Tue, 6 Dec 2011, Andrew Morton wrote:

> > Ping #2?
> > 
> 
> Why am I being pinged about scheduler patches?  My sole contribution
> to this one is to point out that "its"->possessive and "it's"->"it is".
> 

It touches two areas and there's probably a counter argument on the 
otherside about wanting to apply cpuset patches, which typically go 
through -mm.  It's a patch that, as the changelog indicates, fixes cpusets 
from becoming immortal if their set of tasks can never be emptied.  Would 
you like the change separated into kernel/cpuset.c and kernel/sched.c 
portions each referring to each other?

> Also, Peter has said
> 
> : I really think that if we want to restrain userspace from doing
> : something stupid we might as well do something that makes sense, and
> : that is mandate kthreadd stays in the root group at all times for
> : everybody.
> 
> which appears to be what the patch already did, so I'm confused again.
> 

Yes, that's what the patch does.  I'm not sure that Peter's explanation 
above is any better than what Mike wrote in the changelog, though, the 
changelog states why allowing kthreadd to move to a cpuset is actually 
troublesome.

> It's time for a fresh resend, IMO.
> 

Mike, would you mind resending the patch for the fourth or fifth time?  If 
not, I'll rebase it.

Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Andrew Morton]

On Tue, 6 Dec 2011 15:53:47 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:

> Would 
> you like the change separated into kernel/cpuset.c and kernel/sched.c 
> portions each referring to each other?

Nope.

> Mike, would you mind resending the patch for the fourth or fifth time?  If 
> not, I'll rebase it.

Oh goody.

[resubmit] Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2011-12-06 at 15:53 -0800, David Rientjes wrote:

> Mike, would you mind resending the patch for the fourth or fifth time?  If 
> not, I'll rebase it.

Sure, below are the two candidates.  I like Peter's idea better (door
#2), "Keep yer grubby mitts offa _my_ Mom" is a natural reaction ;-)

Door #1

From: Mike Galbraith <efault@gmx.de>

cpusets, cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Paul Menage <paul@paulmenage.org>

---
 kernel/cpuset.c     |    6 ++++--
 kernel/sched/core.c |   10 ++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

Index: linux-3.0-tip/kernel/cpuset.c
===================================================================
--- linux-3.0-tip.orig/kernel/cpuset.c
+++ linux-3.0-tip/kernel/cpuset.c
@@ -59,6 +59,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/cgroup.h>
+#include <linux/kthread.h>
 
 /*
  * Workqueue for cpuset related tasks.
@@ -1385,9 +1386,10 @@ static int cpuset_can_attach(struct cgro
 	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
 	 * applicable for such threads.  This prevents checking for success of
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
-	 * be changed.
+	 * be changed.  We also disallow attaching kthreadd, to prevent it's
+	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
 		return -EINVAL;
 
 	return 0;
Index: linux-3.0-tip/kernel/sched/core.c
===================================================================
--- linux-3.0-tip.orig/kernel/sched/core.c
+++ linux-3.0-tip/kernel/sched/core.c
@@ -71,6 +71,7 @@
 #include <linux/ftrace.h>
 #include <linux/slab.h>
 #include <linux/init_task.h>
+#include <linux/kthread.h>
 
 #include <asm/tlb.h>
 #include <asm/irq_regs.h>
@@ -7461,6 +7462,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
 static int
 cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 {
+	/*
+	 * kthreadd can fork workers for an RT workqueue in a cgroup
+	 * which may or may not have rt_runtime allocated.  Just say no,
+	 * as attaching a global resource to a non-root group  doesn't
+	 * make any sense anyway.
+	 */
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 #ifdef CONFIG_RT_GROUP_SCHED
 	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
 		return -EINVAL;


Door #2


From: Mike Galbraith <efault@gmx.de>

cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <efault@gmx.de>

---
 kernel/cgroup.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-3.0-tip/kernel/cgroup.c
===================================================================
--- linux-3.0-tip.orig/kernel/cgroup.c
+++ linux-3.0-tip/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/kthread.h>
 
 #include <linux/atomic.h>
 
@@ -1824,6 +1825,14 @@ int cgroup_attach_task(struct cgroup *cg
 	struct cgroup *oldcgrp;
 	struct cgroupfs_root *root = cgrp->root;
 
+	/*
+	 * Workqueue threads may acquire PF_THREAD_BOUND and become
+	 * trapped in a cpuset, or RT worker may be born in a cgroup
+	 * with no rt_runtime allocated.  Just say no.
+	 */
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 	/* Nothing to do if the task is already in that cgroup */
 	oldcgrp = task_cgroup_from_root(tsk, root);
 	if (cgrp == oldcgrp)


Door #3

Re: [resubmit] Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

Door #1 with speeling correction.

From: Mike Galbraith <efault@gmx.de>

cpusets, cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Paul Menage <paul@paulmenage.org>

---
 kernel/cpuset.c     |    6 ++++--
 kernel/sched/core.c |   10 ++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

Index: linux-3.0-tip/kernel/cpuset.c
===================================================================
--- linux-3.0-tip.orig/kernel/cpuset.c
+++ linux-3.0-tip/kernel/cpuset.c
@@ -59,6 +59,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/cgroup.h>
+#include <linux/kthread.h>
 
 /*
  * Workqueue for cpuset related tasks.
@@ -1385,9 +1386,10 @@ static int cpuset_can_attach(struct cgro
 	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
 	 * applicable for such threads.  This prevents checking for success of
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
-	 * be changed.
+	 * be changed.  We also disallow attaching kthreadd, to prevent its
+	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
 		return -EINVAL;
 
 	return 0;
Index: linux-3.0-tip/kernel/sched/core.c
===================================================================
--- linux-3.0-tip.orig/kernel/sched/core.c
+++ linux-3.0-tip/kernel/sched/core.c
@@ -71,6 +71,7 @@
 #include <linux/ftrace.h>
 #include <linux/slab.h>
 #include <linux/init_task.h>
+#include <linux/kthread.h>
 
 #include <asm/tlb.h>
 #include <asm/irq_regs.h>
@@ -7461,6 +7462,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
 static int
 cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 {
+	/*
+	 * kthreadd can fork workers for an RT workqueue in a cgroup
+	 * which may or may not have rt_runtime allocated.  Just say no,
+	 * as attaching a global resource to a non-root group  doesn't
+	 * make any sense anyway.
+	 */
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 #ifdef CONFIG_RT_GROUP_SCHED
 	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
 		return -EINVAL;

Re: [resubmit] Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Wed, 7 Dec 2011, Mike Galbraith wrote:

> Door #1 with speeling correction.
> 
> From: Mike Galbraith <efault@gmx.de>
> 
> cpusets, cgroups: disallow attaching kthreadd
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> Acked-by: David Rientjes <rientjes@google.com>
> Acked-by: Paul Menage <paul@paulmenage.org>

Yes, this looks good.  Andrew could you apply this?

> ---
>  kernel/cpuset.c     |    6 ++++--
>  kernel/sched/core.c |   10 ++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> Index: linux-3.0-tip/kernel/cpuset.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/cpuset.c
> +++ linux-3.0-tip/kernel/cpuset.c
> @@ -59,6 +59,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/cgroup.h>
> +#include <linux/kthread.h>
>  
>  /*
>   * Workqueue for cpuset related tasks.
> @@ -1385,9 +1386,10 @@ static int cpuset_can_attach(struct cgro
>  	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
>  	 * applicable for such threads.  This prevents checking for success of
>  	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
> -	 * be changed.
> +	 * be changed.  We also disallow attaching kthreadd, to prevent its
> +	 * child from becoming trapped should it then acquire PF_THREAD_BOUND.
>  	 */
> -	if (tsk->flags & PF_THREAD_BOUND)
> +	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
>  		return -EINVAL;
>  
>  	return 0;
> Index: linux-3.0-tip/kernel/sched/core.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/sched/core.c
> +++ linux-3.0-tip/kernel/sched/core.c
> @@ -71,6 +71,7 @@
>  #include <linux/ftrace.h>
>  #include <linux/slab.h>
>  #include <linux/init_task.h>
> +#include <linux/kthread.h>
>  
>  #include <asm/tlb.h>
>  #include <asm/irq_regs.h>
> @@ -7461,6 +7462,15 @@ cpu_cgroup_destroy(struct cgroup_subsys
>  static int
>  cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  {
> +	/*
> +	 * kthreadd can fork workers for an RT workqueue in a cgroup
> +	 * which may or may not have rt_runtime allocated.  Just say no,
> +	 * as attaching a global resource to a non-root group  doesn't
> +	 * make any sense anyway.
> +	 */
> +	if (tsk == kthreadd_task)
> +		return -EINVAL;
> +
>  #ifdef CONFIG_RT_GROUP_SCHED
>  	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
>  		return -EINVAL;

Re: [resubmit] Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Andrew Morton]

On Wed, 7 Dec 2011 14:03:56 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:

> On Wed, 7 Dec 2011, Mike Galbraith wrote:
> 
> > Door #1 with speeling correction.
> > 
> > From: Mike Galbraith <efault@gmx.de>
> > 
> > cpusets, cgroups: disallow attaching kthreadd
> > 
> > Allowing kthreadd to be moved to a non-root group makes no sense, it being
> > a global resource, and needlessly leads unsuspecting users toward trouble.
> > 
> > 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> > allocated is not schedulable.  Simple user error, but harmful to the box.
> > 
> > 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> > rendering the cpuset immortal.
> > 
> > Save the user some unexpected trouble, just say no.
> > 
> > Signed-off-by: Mike Galbraith <efault@gmx.de>
> > Acked-by: David Rientjes <rientjes@google.com>
> > Acked-by: Paul Menage <paul@paulmenage.org>
> 
> Yes, this looks good.  Andrew could you apply this?

This patch needed considerable rework resulting from ongoing churn in
linux-next cgroups code.  It needs checking and retesting, please.


From: Mike Galbraith <efault@gmx.de>
Subject: cpusets, cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no
   rt_runtime allocated is not schedulable.  Simple user error, but
   harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a
   cpuset, rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Paul Menage <paul@paulmenage.org>
Cc: Tejun Heo <htejun@gmail.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/cpuset.c     |    7 +++++--
 kernel/sched/core.c |    9 +++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff -puN kernel/cpuset.c~cpusets-cgroups-disallow-attaching-kthreadd kernel/cpuset.c
--- a/kernel/cpuset.c~cpusets-cgroups-disallow-attaching-kthreadd
+++ a/kernel/cpuset.c
@@ -59,6 +59,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/cgroup.h>
+#include <linux/kthread.h>
 
 /*
  * Workqueue for cpuset related tasks.
@@ -1417,9 +1418,11 @@ static int cpuset_can_attach(struct cgro
 		 * unnecessary.  Thus, cpusets are not applicable for such
 		 * threads.  This prevents checking for success of
 		 * set_cpus_allowed_ptr() on all attached tasks before
-		 * cpus_allowed may be changed.
+		 * cpus_allowed may be changed.  We also disallow attaching
+		 * kthreadd, to prevent its child from becoming trapped should
+		 * it then acquire PF_THREAD_BOUND.
 		 */
-		if (task->flags & PF_THREAD_BOUND)
+		if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
 			return -EINVAL;
 		if ((ret = security_task_setscheduler(task)))
 			return ret;
diff -puN kernel/sched/core.c~cpusets-cgroups-disallow-attaching-kthreadd kernel/sched/core.c
--- a/kernel/sched/core.c~cpusets-cgroups-disallow-attaching-kthreadd
+++ a/kernel/sched/core.c
@@ -71,6 +71,7 @@
 #include <linux/ftrace.h>
 #include <linux/slab.h>
 #include <linux/init_task.h>
+#include <linux/kthread.h>
 
 #include <asm/tlb.h>
 #include <asm/irq_regs.h>
@@ -7535,6 +7536,14 @@ static int cpu_cgroup_can_attach(struct 
 	struct task_struct *task;
 
 	cgroup_taskset_for_each(task, cgrp, tset) {
+		/*
+		 * kthreadd can fork workers for an RT workqueue in a cgroup
+		 * which may or may not have rt_runtime allocated.  Just say no,
+		 * as attaching a global resource to a non-root group  doesn't
+		 * make any sense anyway.
+		 */
+		if (tsk == kthreadd_task)
+			return -EINVAL;
 #ifdef CONFIG_RT_GROUP_SCHED
 		if (!sched_rt_can_attach(cgroup_tg(cgrp), task))
 			return -EINVAL;
_

Re: [resubmit] Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Wed, 14 Dec 2011, Andrew Morton wrote:

> diff -puN kernel/cpuset.c~cpusets-cgroups-disallow-attaching-kthreadd kernel/cpuset.c
> --- a/kernel/cpuset.c~cpusets-cgroups-disallow-attaching-kthreadd
> +++ a/kernel/cpuset.c
> @@ -59,6 +59,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/cgroup.h>
> +#include <linux/kthread.h>
>  
>  /*
>   * Workqueue for cpuset related tasks.
> @@ -1417,9 +1418,11 @@ static int cpuset_can_attach(struct cgro
>  		 * unnecessary.  Thus, cpusets are not applicable for such
>  		 * threads.  This prevents checking for success of
>  		 * set_cpus_allowed_ptr() on all attached tasks before
> -		 * cpus_allowed may be changed.
> +		 * cpus_allowed may be changed.  We also disallow attaching
> +		 * kthreadd, to prevent its child from becoming trapped should
> +		 * it then acquire PF_THREAD_BOUND.
>  		 */
> -		if (task->flags & PF_THREAD_BOUND)
> +		if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
>  			return -EINVAL;
>  		if ((ret = security_task_setscheduler(task)))
>  			return ret;
> diff -puN kernel/sched/core.c~cpusets-cgroups-disallow-attaching-kthreadd kernel/sched/core.c
> --- a/kernel/sched/core.c~cpusets-cgroups-disallow-attaching-kthreadd
> +++ a/kernel/sched/core.c
> @@ -71,6 +71,7 @@
>  #include <linux/ftrace.h>
>  #include <linux/slab.h>
>  #include <linux/init_task.h>
> +#include <linux/kthread.h>
>  
>  #include <asm/tlb.h>
>  #include <asm/irq_regs.h>
> @@ -7535,6 +7536,14 @@ static int cpu_cgroup_can_attach(struct 
>  	struct task_struct *task;
>  
>  	cgroup_taskset_for_each(task, cgrp, tset) {
> +		/*
> +		 * kthreadd can fork workers for an RT workqueue in a cgroup
> +		 * which may or may not have rt_runtime allocated.  Just say no,
> +		 * as attaching a global resource to a non-root group  doesn't
> +		 * make any sense anyway.
> +		 */
> +		if (tsk == kthreadd_task)
> +			return -EINVAL;
>  #ifdef CONFIG_RT_GROUP_SCHED
>  		if (!sched_rt_can_attach(cgroup_tg(cgrp), task))
>  			return -EINVAL;

Ah, yes, this requires knowledge of the cgroups' proc file trigger to 
move all threads into the cgroup.  The ->can_attach() functions then 
have all-or-nothing behavior; either all the threads can be moved or 
none of them can.  So the above are the correct fixups for both 
->can_attach() functions.

The above diff references non-existant symbols (tsk instead of task) in 
both functions, but it appears (and compiles correctly) in linux-next.  So 
please preserve my acked-by.

Thanks for fixing this up!

Re: [resubmit] Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Andrew Morton]

On Wed, 07 Dec 2011 05:36:57 +0100
Mike Galbraith <efault@gmx.de> wrote:

> cpusets, cgroups: disallow attaching kthreadd
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.

Someone's been screwing around in linux-next during the merge window. 
afacit some patch which was previously there has magically disappeared.
That killed your patch - the original version of the
kernel/sched/core.c chagne applies to the new linux-next but the
kernel/cpuset.c part is all wrecked.

There's been a lot of screwing around this time.  There were a large
number of rejects merging current linux-next onto current mainline this
morning due to conflicting changes in tools/perf/.

Re: [resubmit] Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Fri, 2012-01-06 at 14:06 -0800, Andrew Morton wrote:
> On Wed, 07 Dec 2011 05:36:57 +0100
> Mike Galbraith <efault@gmx.de> wrote:
> 
> > cpusets, cgroups: disallow attaching kthreadd
> > 
> > Allowing kthreadd to be moved to a non-root group makes no sense, it being
> > a global resource, and needlessly leads unsuspecting users toward trouble.
> > 
> > 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> > allocated is not schedulable.  Simple user error, but harmful to the box.
> > 
> > 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> > rendering the cpuset immortal.
> > 
> > Save the user some unexpected trouble, just say no.
> 
> Someone's been screwing around in linux-next during the merge window. 
> afacit some patch which was previously there has magically disappeared.
> That killed your patch - the original version of the
> kernel/sched/core.c chagne applies to the new linux-next but the
> kernel/cpuset.c part is all wrecked.

Hm, what to do...

I can't get to mmotm to see what's there, but fresh pulled linux-next
has it as 904655b9, and looked ok. (?)  The only thing I can think to do
is post it as just plugged into fresh pulled master and hope that helps
turn little floundering guppy right side up again.

From: Mike Galbraith <efault@gmx.de>

cpusets, cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no
   rt_runtime allocated is not schedulable.  Simple user error, but
   harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a
   cpuset, rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Paul Menage <paul@paulmenage.org>
Cc: Tejun Heo <htejun@gmail.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 0b1712d..cf438f3 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -59,6 +59,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/cgroup.h>
+#include <linux/kthread.h>
 
 /*
  * Workqueue for cpuset related tasks.
@@ -1404,9 +1405,10 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
 	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
 	 * applicable for such threads.  This prevents checking for success of
 	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
-	 * be changed.
+	 * be changed.  We also disallow attaching kthreadd, to prevent a child
+	 * from becoming trapped should it later acquire PF_THREAD_BOUND.
 	 */
-	if (tsk->flags & PF_THREAD_BOUND)
+	if (tsk->flags & PF_THREAD_BOUND || tsk == kthreadd_task)
 		return -EINVAL;
 
 	return 0;
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4dbfd04..165d610 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -71,6 +71,7 @@
 #include <linux/ftrace.h>
 #include <linux/slab.h>
 #include <linux/init_task.h>
+#include <linux/kthread.h>
 
 #include <asm/tlb.h>
 #include <asm/irq_regs.h>
@@ -7576,6 +7577,15 @@ cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 	if (tsk->sched_class != &fair_sched_class)
 		return -EINVAL;
 #endif
+	/*
+	 * kthreadd can fork workers for an RT workqueue in a cgroup
+	 * which may or may not have rt_runtime allocated.  Just say no,
+	 * as attaching a global resource to a non-root group  doesn't
+	 * make any sense anyway.
+	 */
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 	return 0;
 }
 

Re: [resubmit] Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[Andrew Morton]

On Sat, 07 Jan 2012 07:34:17 +0100 Mike Galbraith <efault@gmx.de> wrote:

> I can't get to mmotm to see what's there, but fresh pulled linux-next
> has it as 904655b9, and looked ok. (?)  The only thing I can think to do
> is post it as just plugged into fresh pulled master and hope that helps
> turn little floundering guppy right side up again.

master is not where the problems lies.  It's recent vandalism in
linux-next which is breaking things.  Let's leave it a few days, see if
things get sorted out.

Re: [resubmit] Re: [patch-final] Re: patch] cpusets, cgroups: disallow attaching kthreadd

[David Rientjes]

On Wed, 7 Dec 2011, Mike Galbraith wrote:

> > Mike, would you mind resending the patch for the fourth or fifth time?  If 
> > not, I'll rebase it.
> 
> Sure, below are the two candidates.  I like Peter's idea better (door
> #2), "Keep yer grubby mitts offa _my_ Mom" is a natural reaction ;-)
> 

The problem with door #2 is that it may be perfectly legitimate to attach 
kthreadd to a non-root cgroup if that cgroup allows it to move without any 
special PF_THREAD_BOUND exception.  The cpusets case is special because we 
have this in cpuset_can_attach():

        /*
         * Kthreads bound to specific cpus cannot be moved to a new cpuset; we
         * cannot change their cpu affinity and isolating such threads by their
         * set of allowed nodes is unnecessary.  Thus, cpusets are not
         * applicable for such threads.  This prevents checking for success of
         * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
         * be changed.
         */
        if (tsk->flags & PF_THREAD_BOUND)
                return -EINVAL;

that precludes PF_THREAD_BOUND threads from moving to any other cpuset.  
This doesn't impact any other cgroups if cpusets are not used.  The 
exception exists for cpusets since we don't want to be able to attach a 
PF_THREAD_BOUND thread to a cpuset and then subsequently change 
cpuset.mems which would cause a discrepency between threads attached to a 
cpuset and their set of allowed nodes since we cannot touch the affinity 
of these threads: set_cpus_allowed_ptr() returns -EINVAL.

So door #2 would definitely be too heavy of a hammer for something that is 
_only_ a cpusets issue.  If new cgroups come along that need special 
behavior for PF_THREAD_BOUND threads, they need to implement that in their 
own ->can_attach() function just like you have here for cpusets in door 
#1.

[snip to door #2 for completeness]

> Door #2
> 
> 
> From: Mike Galbraith <efault@gmx.de>
> 
> cgroups: disallow attaching kthreadd
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> 
> ---
>  kernel/cgroup.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> Index: linux-3.0-tip/kernel/cgroup.c
> ===================================================================
> --- linux-3.0-tip.orig/kernel/cgroup.c
> +++ linux-3.0-tip/kernel/cgroup.c
> @@ -60,6 +60,7 @@
>  #include <linux/eventfd.h>
>  #include <linux/poll.h>
>  #include <linux/flex_array.h> /* used in cgroup_attach_proc */
> +#include <linux/kthread.h>
>  
>  #include <linux/atomic.h>
>  
> @@ -1824,6 +1825,14 @@ int cgroup_attach_task(struct cgroup *cg
>  	struct cgroup *oldcgrp;
>  	struct cgroupfs_root *root = cgrp->root;
>  
> +	/*
> +	 * Workqueue threads may acquire PF_THREAD_BOUND and become
> +	 * trapped in a cpuset, or RT worker may be born in a cgroup
> +	 * with no rt_runtime allocated.  Just say no.
> +	 */
> +	if (tsk == kthreadd_task)
> +		return -EINVAL;
> +
>  	/* Nothing to do if the task is already in that cgroup */
>  	oldcgrp = task_cgroup_from_root(tsk, root);
>  	if (cgrp == oldcgrp)