Commit 8323f26c authored by Peter Zijlstra's avatar Peter Zijlstra Committed by Ingo Molnar
Browse files

sched: Fix race in task_group() 



Stefan reported a crash on a kernel before a3e5d109 ("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: default avatarStefan Bader <stefan.bader@canonical.com>
Signed-off-by: default avatarPeter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1340364965.18025.71.camel@twins


Signed-off-by: default avatarIngo Molnar <mingo@kernel.org>
parent 88b8dac0

include/linux/init_task.h

+11 −1
Original line number Diff line number Diff line
@@ -123,6 +123,15 @@ 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)						\

	.perf_event_mutex = 						\
@@ -161,6 +170,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
Original line number Diff line number Diff line
@@ -1245,6 +1245,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_PREEMPT_NOTIFIERS

	/* list of struct preempt_notifier: */
@@ -2724,7 +2727,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

+8 −1
Original line number Diff line number Diff line
@@ -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().
@@ -7658,6 +7658,7 @@ void sched_destroy_group(struct task_group *tg) 
 */

void sched_move_task(struct task_struct *tsk)

{

	struct task_group *tg;

	int on_rq, running;

	unsigned long flags;

	struct rq *rq;
@@ -7672,6 +7673,12 @@ 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_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

+10 −13
Original line number Diff line number Diff line
@@ -538,22 +538,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;



	css = task_subsys_state_check(p, cpu_cgroup_subsys_id,

			lockdep_is_held(&p->pi_lock) ||

			lockdep_is_held(&task_rq(p)->lock));

	tg = container_of(css, struct task_group, css);



	return autogroup_task_group(p, tg);

	return p->sched_task_group;

}



/* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */

CRA Git | Maintained and supported by SUSTech CRA and CCSE