[RFC 1/2] cgroup, kthread: do not allow moving kthreads out of the root cgroup

[RFC 1/2] cgroup, kthread: do not allow moving kthreads out of the root cgroup

[Roman Gushchin]

Attaching kernel threads to a non-root cgroup is generally a bad
idea. Kernel threads are generally performing the work required
to keep the system working and healthy, and applying various
resource limits may affect system stability and performance.

Some examples of dangerous behavior are limiting CPU time available
to rcu stuff, memory limits applied to almost all kthreads, etc.

To prevent this dangerous behavior, let's deny all kthread
movements between cgroups. Right now only kthreads bounded
to CPUs are not allowed to move, which is not sufficient.

If there are examples of kthreads which can be limited,
and it's guaranteed to be safe, we can allow explicit
exceptions further.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Chris Mason <clm@fb.com>
Cc: kernel-team@fb.com
Cc: linux-kernel@vger.kernel.org
---
 kernel/cgroup/cgroup.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index c7086c8835da..a3d7a1b5720d 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2629,12 +2629,12 @@ struct task_struct *cgroup_procs_write_start(char *buf, bool threadgroup)
 		tsk = tsk->group_leader;
 
 	/*
-	 * kthreads may acquire PF_NO_SETAFFINITY during initialization.
-	 * If userland migrates such a kthread to a non-root cgroup, it can
+	 * If userland migrates a kthread to a non-root cgroup, it can
 	 * become trapped in a cpuset, or RT kthread may be born in a
-	 * cgroup with no rt_runtime allocated.  Just say no.
+	 * cgroup with no rt_runtime allocated, or it can suffer from
+	 * memory shortage, etc. Just say no.
 	 */
-	if (tsk->no_cgroup_migration || (tsk->flags & PF_NO_SETAFFINITY)) {
+	if (tsk->flags & (PF_KTHREAD | PF_NO_SETAFFINITY)) {
 		tsk = ERR_PTR(-EINVAL);
 		goto out_unlock_threadgroup;
 	}
-- 
2.13.6

[RFC 2/2] cgroup, kthread: cleanup after sticking kthreads to the root cgroup

[Roman Gushchin]

Since we're not allowing to move kernel threads out of the root
cgroup anymore, we don't need some infrastructure built to
introduce partial moving ban by
commit 77f88796cee8 ("cgroup, kthread: close race window where
new kthreads can be migrated to non-root cgroups").

This patch is just a cleanup, no functional change
is performed.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Chris Mason <clm@fb.com>
Cc: kernel-team@fb.com
Cc: linux-kernel@vger.kernel.org
---
 include/linux/cgroup.h | 21 ---------------------
 include/linux/sched.h  |  4 ----
 kernel/kthread.c       |  2 --
 3 files changed, 27 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 328a70ce0e23..2b2b838beaed 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -626,25 +626,6 @@ static inline void pr_cont_cgroup_path(struct cgroup *cgrp)
 	pr_cont_kernfs_path(cgrp->kn);
 }
 
-static inline void cgroup_init_kthreadd(void)
-{
-	/*
-	 * kthreadd is inherited by all kthreads, keep it in the root so
-	 * that the new kthreads are guaranteed to stay in the root until
-	 * initialization is finished.
-	 */
-	current->no_cgroup_migration = 1;
-}
-
-static inline void cgroup_kthread_ready(void)
-{
-	/*
-	 * This kthread finished initialization.  The creator should have
-	 * set PF_NO_SETAFFINITY if this kthread should stay in the root.
-	 */
-	current->no_cgroup_migration = 0;
-}
-
 static inline union kernfs_node_id *cgroup_get_kernfs_id(struct cgroup *cgrp)
 {
 	return &cgrp->kn->id;
@@ -672,8 +653,6 @@ static inline void cgroup_free(struct task_struct *p) {}
 
 static inline int cgroup_init_early(void) { return 0; }
 static inline int cgroup_init(void) { return 0; }
-static inline void cgroup_init_kthreadd(void) {}
-static inline void cgroup_kthread_ready(void) {}
 static inline union kernfs_node_id *cgroup_get_kernfs_id(struct cgroup *cgrp)
 {
 	return NULL;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index bdd6ad6fcce1..7ee7bad521ff 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -650,10 +650,6 @@ struct task_struct {
 #ifdef CONFIG_COMPAT_BRK
 	unsigned			brk_randomized:1;
 #endif
-#ifdef CONFIG_CGROUPS
-	/* disallow userland-initiated cgroup migration */
-	unsigned			no_cgroup_migration:1;
-#endif
 
 	unsigned long			atomic_flags; /* Flags requiring atomic access. */
 
diff --git a/kernel/kthread.c b/kernel/kthread.c
index e92070878ce5..0ba173e9b5b6 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -237,7 +237,6 @@ static int kthread(void *_create)
 
 	ret = -EINTR;
 	if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) {
-		cgroup_kthread_ready();
 		__kthread_parkme(self);
 		ret = threadfn(data);
 	}
@@ -551,7 +550,6 @@ int kthreadd(void *unused)
 	set_mems_allowed(node_states[N_MEMORY]);
 
 	current->flags |= PF_NOFREEZE;
-	cgroup_init_kthreadd();
 
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);
-- 
2.13.6

Re: [RFC 1/2] cgroup, kthread: do not allow moving kthreads out of the root cgroup

[Peter Zijlstra]

On Thu, Oct 12, 2017 at 06:37:22PM +0100, Roman Gushchin wrote:
> Attaching kernel threads to a non-root cgroup is generally a bad
> idea. Kernel threads are generally performing the work required
> to keep the system working and healthy, and applying various
> resource limits may affect system stability and performance.
> 
> Some examples of dangerous behavior are limiting CPU time available
> to rcu stuff, memory limits applied to almost all kthreads, etc.
> 
> To prevent this dangerous behavior, let's deny all kthread
> movements between cgroups. Right now only kthreads bounded
> to CPUs are not allowed to move, which is not sufficient.
> 
> If there are examples of kthreads which can be limited,
> and it's guaranteed to be safe, we can allow explicit
> exceptions further.

The traditional use-case is stuffing all the unbound kthreads into a
system cpuset in order to limit 'crap' on the rest of the CPUs.
This setup is typically found in HPC and RT environments.

So NAK. This needs to stay working in as far as it still works.

Re: [RFC 1/2] cgroup, kthread: do not allow moving kthreads out of the root cgroup

[Roman Gushchin]

On Thu, Oct 12, 2017 at 09:24:45PM +0200, Peter Zijlstra wrote:
> On Thu, Oct 12, 2017 at 06:37:22PM +0100, Roman Gushchin wrote:
> > Attaching kernel threads to a non-root cgroup is generally a bad
> > idea. Kernel threads are generally performing the work required
> > to keep the system working and healthy, and applying various
> > resource limits may affect system stability and performance.
> > 
> > Some examples of dangerous behavior are limiting CPU time available
> > to rcu stuff, memory limits applied to almost all kthreads, etc.
> > 
> > To prevent this dangerous behavior, let's deny all kthread
> > movements between cgroups. Right now only kthreads bounded
> > to CPUs are not allowed to move, which is not sufficient.
> > 
> > If there are examples of kthreads which can be limited,
> > and it's guaranteed to be safe, we can allow explicit
> > exceptions further.
> 
> The traditional use-case is stuffing all the unbound kthreads into a
> system cpuset in order to limit 'crap' on the rest of the CPUs.
> This setup is typically found in HPC and RT environments.
> 
> So NAK. This needs to stay working in as far as it still works.

Ok, thanks!

Re: [RFC 1/2] cgroup, kthread: do not allow moving kthreads out of the root cgroup

[David Rientjes]

On Thu, 12 Oct 2017, Peter Zijlstra wrote:

> > Attaching kernel threads to a non-root cgroup is generally a bad
> > idea. Kernel threads are generally performing the work required
> > to keep the system working and healthy, and applying various
> > resource limits may affect system stability and performance.
> > 
> > Some examples of dangerous behavior are limiting CPU time available
> > to rcu stuff, memory limits applied to almost all kthreads, etc.
> > 
> > To prevent this dangerous behavior, let's deny all kthread
> > movements between cgroups. Right now only kthreads bounded
> > to CPUs are not allowed to move, which is not sufficient.
> > 
> > If there are examples of kthreads which can be limited,
> > and it's guaranteed to be safe, we can allow explicit
> > exceptions further.
> 
> The traditional use-case is stuffing all the unbound kthreads into a
> system cpuset in order to limit 'crap' on the rest of the CPUs.
> This setup is typically found in HPC and RT environments.
> 
> So NAK. This needs to stay working in as far as it still works.
> 

+1, I originally introduced PF_THREAD_BOUND to cpusets in 2008 to prevent 
migration threads and software watchdog threads from being able to access 
the cpus they work on.  We also move unbound kthreads to a system cpuset.