sched: Fix race in task_group()

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched: Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

Anybody else got a better idea?

Reported-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 include/linux/init_task.h |   12 +++++++++++-
 include/linux/sched.h     |    5 ++++-
 kernel/sched/core.c       |    9 ++++++++-
 kernel/sched/sched.h      |   23 ++++++++++-------------
 4 files changed, 33 insertions(+), 16 deletions(-)

include/linux/init_task.h +11 -1

index 4e4bc1a..53be033 100644
@@ -123,8 +123,17 @@ extern struct group_info init_groups;
 
 extern struct cred init_cred;
 
+extern struct task_group root_task_group;
+
+#ifdef CONFIG_CGROUP_SCHED
+# define INIT_CGROUP_SCHED(tsk)						\
+	.sched_task_group = &root_task_group,
+#else
+# define INIT_CGROUP_SCHED(tsk)
+#endif
+
 #ifdef CONFIG_PERF_EVENTS
-# define INIT_PERF_EVENTS(tsk)					\
+# define INIT_PERF_EVENTS(tsk)						\
 	.perf_event_mutex = 						\
 		 __MUTEX_INITIALIZER(tsk.perf_event_mutex),		\
 	.perf_event_list = LIST_HEAD_INIT(tsk.perf_event_list),

@@ -168,6 +177,7 @@ extern struct cred init_cred;
 	},								\
 	.tasks		= LIST_HEAD_INIT(tsk.tasks),			\
 	INIT_PUSHABLE_TASKS(tsk)					\
+	INIT_CGROUP_SCHED(tsk)						\
 	.ptraced	= LIST_HEAD_INIT(tsk.ptraced),			\
 	.ptrace_entry	= LIST_HEAD_INIT(tsk.ptrace_entry),		\
 	.real_parent	= &tsk,						\

include/linux/sched.h +4 -1

index 32157b9..77437d4 100644
@@ -1246,6 +1246,9 @@ struct task_struct {
 	const struct sched_class *sched_class;
 	struct sched_entity se;
 	struct sched_rt_entity rt;
+#ifdef CONFIG_CGROUP_SCHED
+	struct task_struct *sched_task_group;
+#endif
 
 #ifdef CONFIG_NUMA
 	unsigned long	 numa_contrib;

@@ -2741,7 +2744,7 @@ extern int sched_group_set_rt_period(struct task_group *tg,
 extern long sched_group_rt_period(struct task_group *tg);
 extern int sched_rt_can_attach(struct task_group *tg, struct task_struct *tsk);
 #endif
-#endif
+#endif /* CONFIG_CGROUP_SCHED */
 
 extern int task_can_switch_user(struct user_struct *up,
 					struct task_struct *tsk);

kernel/sched/core.c +3 -1

index 9bb7d28..9adb9a0 100644
@@ -1096,7 +1096,7 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 	 * a task's CPU. ->pi_lock for waking tasks, rq->lock for runnable tasks.
 	 *
 	 * sched_move_task() holds both and thus holding either pins the cgroup,
-	 * see set_task_rq().
+	 * see task_group().
 	 *
 	 * Furthermore, all task_rq users should acquire both locks, see
 	 * task_rq_lock().

@@ -7581,6 +7581,8 @@ void sched_destroy_group(struct task_group *tg)
  */
 void sched_move_task(struct task_struct *tsk)
 {
+	struct cgroup_subsys_state *css;
+	struct task_group *tg;
 	int on_rq, running;
 	unsigned long flags;
 	struct rq *rq;

kernel/sched/core.c +2 -0

+	tsk->sched_task_group = tg;
+
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	if (tsk->sched_class->task_move_group)
 		tsk->sched_class->task_move_group(tsk, on_rq);

kernel/sched/sched.h +9 -7

index 4134d37..c26378c 100644
@@ -554,22 +554,19 @@ extern int group_balance_cpu(struct sched_group *sg);
 /*
  * Return the group to which this tasks belongs.
  *
- * We use task_subsys_state_check() and extend the RCU verification with
- * pi->lock and rq->lock because cpu_cgroup_attach() holds those locks for each
- * task it moves into the cgroup. Therefore by holding either of those locks,
- * we pin the task to the current cgroup.
+ * We cannot use task_subsys_state() and friends because the cgroup
+ * subsystem changes that value before the cgroup_subsys::attach() method
+ * is called, therefore we cannot pin it and might observe the wrong value.
+ *
+ * The same is true for autogroup's p->signal->autogroup->tg, the autogroup
+ * core changes this before calling sched_move_task().
+ *
+ * Instead we use a 'copy' which is updated from sched_move_task() while
+ * holding both task_struct::pi_lock and rq::lock.
  */
 static inline struct task_group *task_group(struct task_struct *p)
 {
-	struct task_group *tg;
-	struct cgroup_subsys_state *css;
-

kernel/sched/core.c void sched_move_task(struct task_struct *tsk) · line 7600

Quoted 5 lines

kernel/sched/core.c +1 -0

@@ -7595,6 +7597,11 @@ void sched_move_task(struct task_struct *tsk)
      if (unlikely(running))
              tsk->sched_class->put_prev_task(rq, tsk);
 
+     tg = container_of(task_subsys_state(p, cpu_cgroup_subsys_id),

s/p/tsk/

kernel/sched/core.c void sched_move_task(struct task_struct *tsk) · line 7602

Quoted 2 lines

kernel/sched/core.c +2 -0

+                       struct task_group, css);
+     tg = autogroup_task_group(p, tg);

s/p/tsk/

Quoted 2 lines

kernel/sched/core.c +2 -0

+     tsk->sched_task_group = tg;
+ 

Here's one that's actually compile tested (with the right CONFIG_foo enabled) and I fixed the autogroup lockdep splat.

---
Subject: sched: Fix race in task_group()
From: Peter Zijlstra <[email protected]>
Date: Fri, 22 Jun 2012 13:36:05 +0200

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched: Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

Reported-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 include/linux/init_task.h |   12 +++++++++++-
 include/linux/sched.h     |    5 ++++-
 kernel/sched/core.c       |    9 ++++++++-
 kernel/sched/sched.h      |   23 ++++++++++-------------
 4 files changed, 33 insertions(+), 16 deletions(-)

include/linux/init_task.h +11 -1

@@ -123,8 +123,17 @@ extern struct group_info init_groups;
 
 extern struct cred init_cred;
 
+extern struct task_group root_task_group;
+
+#ifdef CONFIG_CGROUP_SCHED
+# define INIT_CGROUP_SCHED(tsk)						\
+	.sched_task_group = &root_task_group,
+#else
+# define INIT_CGROUP_SCHED(tsk)
+#endif
+
 #ifdef CONFIG_PERF_EVENTS
-# define INIT_PERF_EVENTS(tsk)					\
+# define INIT_PERF_EVENTS(tsk)						\
 	.perf_event_mutex = 						\
 		 __MUTEX_INITIALIZER(tsk.perf_event_mutex),		\
 	.perf_event_list = LIST_HEAD_INIT(tsk.perf_event_list),

@@ -168,6 +177,7 @@ extern struct cred init_cred;
 	},								\
 	.tasks		= LIST_HEAD_INIT(tsk.tasks),			\
 	INIT_PUSHABLE_TASKS(tsk)					\
+	INIT_CGROUP_SCHED(tsk)						\
 	.ptraced	= LIST_HEAD_INIT(tsk.ptraced),			\
 	.ptrace_entry	= LIST_HEAD_INIT(tsk.ptrace_entry),		\
 	.real_parent	= &tsk,						\

include/linux/sched.h +4 -1

@@ -1246,6 +1246,9 @@ struct task_struct {
 	const struct sched_class *sched_class;
 	struct sched_entity se;
 	struct sched_rt_entity rt;
+#ifdef CONFIG_CGROUP_SCHED
+	struct task_group *sched_task_group;
+#endif
 
 #ifdef CONFIG_NUMA
 	unsigned long	 numa_contrib;

@@ -2749,7 +2752,7 @@ extern int sched_group_set_rt_period(str
 extern long sched_group_rt_period(struct task_group *tg);
 extern int sched_rt_can_attach(struct task_group *tg, struct task_struct *tsk);
 #endif
-#endif
+#endif /* CONFIG_CGROUP_SCHED */
 
 extern int task_can_switch_user(struct user_struct *up,
 					struct task_struct *tsk);

kernel/sched/core.c +8 -1

@@ -1096,7 +1096,7 @@ void set_task_cpu(struct task_struct *p,
 	 * a task's CPU. ->pi_lock for waking tasks, rq->lock for runnable tasks.
 	 *
 	 * sched_move_task() holds both and thus holding either pins the cgroup,
-	 * see set_task_rq().
+	 * see task_group().
 	 *
 	 * Furthermore, all task_rq users should acquire both locks, see
 	 * task_rq_lock().

@@ -7712,6 +7712,7 @@ void sched_destroy_group(struct task_gro
  */
 void sched_move_task(struct task_struct *tsk)
 {
+	struct task_group *tg;
 	int on_rq, running;
 	unsigned long flags;
 	struct rq *rq;

@@ -7726,6 +7727,12 @@ void sched_move_task(struct task_struct
 	if (unlikely(running))
 		tsk->sched_class->put_prev_task(rq, tsk);
 
+	tg = container_of(task_subsys_state_check(tsk, cpu_cgroup_subsys_id,
+				lockdep_is_held(&tsk->sighand->siglock)),
+			  struct task_group, css);
+	tg = autogroup_task_group(tsk, tg);
+	tsk->sched_task_group = tg;
+
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	if (tsk->sched_class->task_move_group)
 		tsk->sched_class->task_move_group(tsk, on_rq);

kernel/sched/sched.h +9 -7

@@ -554,22 +554,19 @@ extern int group_balance_cpu(struct sche
 /*
  * Return the group to which this tasks belongs.
  *
- * We use task_subsys_state_check() and extend the RCU verification with
- * pi->lock and rq->lock because cpu_cgroup_attach() holds those locks for each
- * task it moves into the cgroup. Therefore by holding either of those locks,
- * we pin the task to the current cgroup.
+ * We cannot use task_subsys_state() and friends because the cgroup
+ * subsystem changes that value before the cgroup_subsys::attach() method
+ * is called, therefore we cannot pin it and might observe the wrong value.
+ *
+ * The same is true for autogroup's p->signal->autogroup->tg, the autogroup
+ * core changes this before calling sched_move_task().
+ *
+ * Instead we use a 'copy' which is updated from sched_move_task() while
+ * holding both task_struct::pi_lock and rq::lock.
  */
 static inline struct task_group *task_group(struct task_struct *p)
 {
-	struct task_group *tg;
-	struct cgroup_subsys_state *css;
-

Reported-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---

include/linux/sched.h +0 -0

index 32157b9..77437d4 100644

Quoted 9 lines · Peter Zijlstra

Here's one that's actually compile tested (with the right CONFIG_foo enabled) and I fixed the autogroup lockdep splat.

---
Subject: sched: Fix race in task_group()
From: Peter Zijlstra <[email protected]>
Date: Fri, 22 Jun 2012 13:36:05 +0200

include/linux/sched.h struct task_struct { · line 1252

Quoted 13 lines

Reported-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---

include/linux/sched.h +3 -0

@@ -1246,6 +1246,9 @@ struct task_struct {
       const struct sched_class *sched_class;
       struct sched_entity se;
       struct sched_rt_entity rt;
+#ifdef CONFIG_CGROUP_SCHED
+     struct task_group *sched_task_group;
+#endif
 

From d751ab1f1e532f32412d99b71a1bfea3e5282d07 Mon Sep 17 00:00:00 2001

The patch went three versions, the first,

Quoted 8 lines · Peter Zijlstra

Reported-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---

include/linux/sched.h +0 -0

index 32157b9..77437d4 100644

include/linux/sched.h struct task_struct { · line 1252

Quoted 7 lines

include/linux/sched.h +3 -0

@@ -1246,6 +1246,9 @@ struct task_struct {
 	const struct sched_class *sched_class;
 	struct sched_entity se;
 	struct sched_rt_entity rt;
+#ifdef CONFIG_CGROUP_SCHED
+	struct task_struct *sched_task_group;
+#endif
 

The second,

Quoted 9 lines

Quoted 9 lines · Peter Zijlstra

Here's one that's actually compile tested (with the right CONFIG_foo enabled) and I fixed the autogroup lockdep splat.

---
Subject: sched: Fix race in task_group()
From: Peter Zijlstra <[email protected]>
Date: Fri, 22 Jun 2012 13:36:05 +0200

Quoted 13 lines

include/linux/sched.h struct task_struct { · line 1252

Quoted 13 lines

Reported-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---

include/linux/sched.h +3 -0

@@ -1246,6 +1246,9 @@ struct task_struct {
      const struct sched_class *sched_class;
      struct sched_entity se;
      struct sched_rt_entity rt;
+#ifdef CONFIG_CGROUP_SCHED
+     struct task_group *sched_task_group;
+#endif
 

And the third, https://lkml.org/lkml/2012/6/26/331

From d751ab1f1e532f32412d99b71a1bfea3e5282d07 Mon Sep 17 00:00:00 2001
From: Peter Zijlstra <[email protected]>
Date: Fri, 22 Jun 2012 13:36:00 +0200
Subject: [PATCH] sched: Fix race in task_group()

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched: Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

Reported-and-tested-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

[backported to apply to 3.0 and 3.2]

Signed-off-by Stefan Bader <[email protected]>

---
 include/linux/init_task.h |   12 +++++++++++-
 include/linux/sched.h     |    5 ++++-
 kernel/sched.c            |   32 ++++++++++++++++++--------------
 3 files changed, 33 insertions(+), 16 deletions(-)

include/linux/sched.h +3 -0

index 56de5c1..1fd9884 100644
@@ -1242,6 +1242,9 @@ struct task_struct {
 	const struct sched_class *sched_class;
 	struct sched_entity se;
 	struct sched_rt_entity rt;
+#ifdef CONFIG_CGROUP_SCHED
+	struct task_struct *sched_task_group;
+#endif
 

where sched_task_group was defined to be task_struct twice(in the first and the third versions) and to be task_group once.

Before backport, feel free to respin with the final define determined.

Here's one that's actually compile tested (with the right CONFIG_foo enabled) and I fixed the autogroup lockdep splat.

---
Subject: sched: Fix race in task_group()
From: Peter Zijlstra <[email protected]>
Date: Fri, 22 Jun 2012 13:36:05 +0200

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched: Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

BTW your patch is whitespace broken. Seems like QP encoded.

Here's one that's actually compile tested (with the right CONFIG_foo enabled) and I fixed the autogroup lockdep splat.

---
Subject: sched: Fix race in task_group()
From: Peter Zijlstra <[email protected]>
Date: Fri, 22 Jun 2012 13:36:05 +0200

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched: Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

Reported-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 include/linux/init_task.h |   12 +++++++++++-
 include/linux/sched.h     |    5 ++++-
 kernel/sched/core.c       |    9 ++++++++-
 kernel/sched/sched.h      |   23 ++++++++++-------------
 4 files changed, 33 insertions(+), 16 deletions(-)

include/linux/init_task.h +11 -1

@@ -123,8 +123,17 @@ extern struct group_info init_groups;
 
 extern struct cred init_cred;
 
+extern struct task_group root_task_group;
+
+#ifdef CONFIG_CGROUP_SCHED
+# define INIT_CGROUP_SCHED(tsk)						\
+	.sched_task_group = &root_task_group,
+#else
+# define INIT_CGROUP_SCHED(tsk)
+#endif
+
 #ifdef CONFIG_PERF_EVENTS
-# define INIT_PERF_EVENTS(tsk)					\
+# define INIT_PERF_EVENTS(tsk)						\
 	.perf_event_mutex = 						\
 		 __MUTEX_INITIALIZER(tsk.perf_event_mutex),		\
 	.perf_event_list = LIST_HEAD_INIT(tsk.perf_event_list),

@@ -168,6 +177,7 @@ extern struct cred init_cred;
 	},								\
 	.tasks		= LIST_HEAD_INIT(tsk.tasks),			\
 	INIT_PUSHABLE_TASKS(tsk)					\
+	INIT_CGROUP_SCHED(tsk)						\
 	.ptraced	= LIST_HEAD_INIT(tsk.ptraced),			\
 	.ptrace_entry	= LIST_HEAD_INIT(tsk.ptrace_entry),		\
 	.real_parent	= &tsk,						\

include/linux/sched.h +4 -1

@@ -1246,6 +1246,9 @@ struct task_struct {
 	const struct sched_class *sched_class;
 	struct sched_entity se;
 	struct sched_rt_entity rt;
+#ifdef CONFIG_CGROUP_SCHED
+	struct task_group *sched_task_group;
+#endif
 
 #ifdef CONFIG_NUMA
 	unsigned long	 numa_contrib;

@@ -2749,7 +2752,7 @@ extern int sched_group_set_rt_period(str
 extern long sched_group_rt_period(struct task_group *tg);
 extern int sched_rt_can_attach(struct task_group *tg, struct task_struct *tsk);
 #endif
-#endif
+#endif /* CONFIG_CGROUP_SCHED */
 
 extern int task_can_switch_user(struct user_struct *up,
 					struct task_struct *tsk);

kernel/sched/core.c +8 -1

@@ -1096,7 +1096,7 @@ void set_task_cpu(struct task_struct *p,
 	 * a task's CPU. ->pi_lock for waking tasks, rq->lock for runnable tasks.
 	 *
 	 * sched_move_task() holds both and thus holding either pins the cgroup,
-	 * see set_task_rq().
+	 * see task_group().
 	 *
 	 * Furthermore, all task_rq users should acquire both locks, see
 	 * task_rq_lock().

@@ -7712,6 +7712,7 @@ void sched_destroy_group(struct task_gro
  */
 void sched_move_task(struct task_struct *tsk)
 {
+	struct task_group *tg;
 	int on_rq, running;
 	unsigned long flags;
 	struct rq *rq;

@@ -7726,6 +7727,12 @@ void sched_move_task(struct task_struct
 	if (unlikely(running))
 		tsk->sched_class->put_prev_task(rq, tsk);
 
+	tg = container_of(task_subsys_state_check(tsk, cpu_cgroup_subsys_id,
+				lockdep_is_held(&tsk->sighand->siglock)),
+			  struct task_group, css);
+	tg = autogroup_task_group(tsk, tg);
+	tsk->sched_task_group = tg;
+
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	if (tsk->sched_class->task_move_group)
 		tsk->sched_class->task_move_group(tsk, on_rq);

kernel/sched/sched.h +9 -7

@@ -554,22 +554,19 @@ extern int group_balance_cpu(struct sche
 /*
  * Return the group to which this tasks belongs.
  *
- * We use task_subsys_state_check() and extend the RCU verification with
- * pi->lock and rq->lock because cpu_cgroup_attach() holds those locks for each
- * task it moves into the cgroup. Therefore by holding either of those locks,
- * we pin the task to the current cgroup.
+ * We cannot use task_subsys_state() and friends because the cgroup
+ * subsystem changes that value before the cgroup_subsys::attach() method
+ * is called, therefore we cannot pin it and might observe the wrong value.
+ *
+ * The same is true for autogroup's p->signal->autogroup->tg, the autogroup
+ * core changes this before calling sched_move_task().
+ *
+ * Instead we use a 'copy' which is updated from sched_move_task() while
+ * holding both task_struct::pi_lock and rq::lock.
  */
 static inline struct task_group *task_group(struct task_struct *p)
 {
-	struct task_group *tg;
-	struct cgroup_subsys_state *css;
-

Commit-ID:  8323f26ce3425460769605a6aece7a174edaa7d1
Gitweb:     http://git.kernel.org/tip/8323f26ce3425460769605a6aece7a174edaa7d1
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 22 Jun 2012 13:36:05 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 24 Jul 2012 13:58:20 +0200

sched: Fix race in task_group()

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched:
Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

Reported-and-tested-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/1340364965.18025.71.camel@twins open here

Signed-off-by Ingo Molnar <[email protected]>

Quoted 22 lines · tip-bot for Peter Zijlstra

Commit-ID:  8323f26ce3425460769605a6aece7a174edaa7d1
Gitweb:     http://git.kernel.org/tip/8323f26ce3425460769605a6aece7a174edaa7d1
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 22 Jun 2012 13:36:05 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 24 Jul 2012 13:58:20 +0200

sched: Fix race in task_group()

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched:
Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

Quoted 4 lines

Reported-and-tested-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/1340364965.18025.71.camel@twins open here

Signed-off-by Ingo Molnar <[email protected]>

I just finished bisecting a crash on boot to this commit; booting with "noautogroup" brings it back.

3.5.4 is the latest -stable that still boots, and none of the 3.6 rc's boot at all.

Photo of the bug (3.6.0next is 3.6 + btrfs's for-linus):
https://lh5.googleusercontent.com/-0DY-YYhgvzs/UHdB-BQdzMI/AAAAAAAAAEg/QhY9rgxnv98/s811/2012-10-11

Quoted 35 lines · cwillu

Quoted 22 lines · tip-bot for Peter Zijlstra

Commit-ID:  8323f26ce3425460769605a6aece7a174edaa7d1
Gitweb:     http://git.kernel.org/tip/8323f26ce3425460769605a6aece7a174edaa7d1
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 22 Jun 2012 13:36:05 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 24 Jul 2012 13:58:20 +0200

sched: Fix race in task_group()

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched:
Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

Quoted 4 lines

Reported-and-tested-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/1340364965.18025.71.camel@twins open here

Signed-off-by Ingo Molnar <[email protected]>

I just finished bisecting a crash on boot to this commit; booting with "noautogroup" brings it back.

3.5.4 is the latest -stable that still boots, and none of the 3.6 rc's boot at all.

Quoted 3 lines

Photo of the bug (3.6.0next is 3.6 + btrfs's for-linus):
https://lh5.googleusercontent.com/-0DY-YYhgvzs/UHdB-BQdzMI/AAAAAAAAAEg/QhY9rgxnv98/s811/2012-10-11

On a very quick glance I wonder whether there might be a case where sched_fork goes into set_task_cpu with a different cpu than the current but has not yet task_group.sched_task_group set to something valid...

Quoted 36 lines · Stefan Bader

Quoted 35 lines · cwillu

Quoted 22 lines · tip-bot for Peter Zijlstra

Commit-ID:  8323f26ce3425460769605a6aece7a174edaa7d1
Gitweb:     http://git.kernel.org/tip/8323f26ce3425460769605a6aece7a174edaa7d1
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 22 Jun 2012 13:36:05 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 24 Jul 2012 13:58:20 +0200

sched: Fix race in task_group()

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched:
Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

Quoted 4 lines

Reported-and-tested-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/1340364965.18025.71.camel@twins open here

Signed-off-by Ingo Molnar <[email protected]>

I just finished bisecting a crash on boot to this commit; booting with "noautogroup" brings it back.

3.5.4 is the latest -stable that still boots, and none of the 3.6 rc's boot at all.

Quoted 9 lines

Quoted 3 lines

Photo of the bug (3.6.0next is 3.6 + btrfs's for-linus):
https://lh5.googleusercontent.com/-0DY-YYhgvzs/UHdB-BQdzMI/AAAAAAAAAEg/QhY9rgxnv98/s811/2012-10-11

On a very quick glance I wonder whether there might be a case where sched_fork goes into set_task_cpu with a different cpu than the current but has not yet task_group.sched_task_group set to something valid...

I was looking at another bug report [1] which may be related with this issue. Basically, it looks like there is a race window where resetting sched_autogroup_enabled will cause a crash on shutdown/reboot. In the bug report, the user has added:

echo 0 > /proc/sys/kernel/sched_autogroup_enabled

to /etc/rc.local. This will cause a NULL pointer dereference during shutdown (and it is reproducible with mainline kernel 3.7.0-rc1).

By using the kernel parameter noautogroup I *wasn't* able to reproduce this issue.

Quoted 36 lines · Stefan Bader

Quoted 35 lines · cwillu

Quoted 22 lines · tip-bot for Peter Zijlstra

Commit-ID:  8323f26ce3425460769605a6aece7a174edaa7d1
Gitweb:     http://git.kernel.org/tip/8323f26ce3425460769605a6aece7a174edaa7d1
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 22 Jun 2012 13:36:05 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 24 Jul 2012 13:58:20 +0200

sched: Fix race in task_group()

Stefan reported a crash on a kernel before a3e5d1091c1 ("sched:
Don't call task_group() too many times in set_task_rq()"), he found the reason to be that the multiple task_group() invocations in set_task_rq() returned different values.

Looking at all that I found a lack of serialization and plain wrong comments.

The below tries to fix it using an extra pointer which is updated under the appropriate scheduler locks. Its not pretty, but I can't really see another way given how all the cgroup stuff works.

Quoted 4 lines

Reported-and-tested-by Stefan Bader <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/1340364965.18025.71.camel@twins open here

Signed-off-by Ingo Molnar <[email protected]>

I just finished bisecting a crash on boot to this commit; booting with "noautogroup" brings it back.

3.5.4 is the latest -stable that still boots, and none of the 3.6 rc's boot at all.

Quoted 9 lines

Quoted 3 lines

Photo of the bug (3.6.0next is 3.6 + btrfs's for-linus):
https://lh5.googleusercontent.com/-0DY-YYhgvzs/UHdB-BQdzMI/AAAAAAAAAEg/QhY9rgxnv98/s811/2012-10-11

On a very quick glance I wonder whether there might be a case where sched_fork goes into set_task_cpu with a different cpu than the current but has not yet task_group.sched_task_group set to something valid...

I was looking at another bug report [1] which may be related with this issue. Basically, it looks like there is a race window where resetting sched_autogroup_enabled will cause a crash on shutdown/reboot. In the bug report, the user has added:

echo 0 > /proc/sys/kernel/sched_autogroup_enabled

to /etc/rc.local. This will cause a NULL pointer dereference during shutdown (and it is reproducible with mainline kernel 3.7.0-rc1).

By using the kernel parameter noautogroup I *wasn't* able to reproduce this issue.

After a little bit of digging, commit 800d4d30c8f20bd728e5741a3b77c4859a613f7c ("sched, autogroup: Stop going ahead if autogroup is disabled") caught my attention as it changes the following code path when sched_autogroup_enabled is disabled:

sched_autogroup_create_attach()
autogroup_move_group()
sched_move_task() <<-- conditionally invoked
task_move_group_fair()
set_task_rq()
task_group()
autogroup_task_group()

And commit 8323f26ce3425460769605a6aece7a174edaa7d1 ("sched: Fix race in task_group()") actually adds code to this conditional path (in sched_move_task()).

A quick test shows that reverting 800d4d30c8f20bd728e5741a3b77c4859a613f7c (i.e., always going through the whole call tree) seems to fix it or, at least, doesn't trigger the NULL pointer. But again, I may just be doing something foolish, hiding something else. It is also possible that this is a completely different issue.

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1055222

Cheers,
--
Luis