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

[David Rientjes <rientjes@google.com>]

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)