[Natty, Oneiric, Precise SRU] Fix race in task_group()
Stefan Bader
stefan.bader at canonical.com
Tue Jul 17 17:22:55 UTC 2012
On 07/03/2012 08:17 AM, Herton Ronaldo Krzesinski wrote:
> On Tue, Jul 03, 2012 at 03:01:58PM +0200, Stefan Bader wrote:
>> Though reproducing this somehow required (or at least made much
>> more likely) a certain setup (two CPUs and the described loop of
>> a certain command), the result (crash) is probably bad enough to
>> warrant applying it before it appears upstream. Peter queued it
>> up but needs Ingo to push it onwards.
>>
>> Also this may cause other subtle to find issues as the current
>> code calls task_group() four times to assign values to cfs and
>> rt schdeduler elements. So while the observed crash happened when
>> cfs elements were inconsistent, there could be other problems when
>> the inconsistency hits cfs/rt or rt only.
>>
>> Quantal still has the same race but at least the code was changed
>> to call task_group only once. So the value could be old but at least
>> it is always old (or the new one).
>>
>> This backport should apply to Natty till Precise (maybe a bit of
>> fuzz in Natty). But later that 3.3 all the files moved around.
>>
>> -Stefan
The patch at least made it into linux-next as
commit 7fac251a97f36c9aef31b08ce7ad6ef8f06e54d1
Author: Peter Zijlstra <peterz at infradead.org>
Date: Fri Jun 22 13:36:05 2012 +0200
sched: Fix race in task_group()
I would like to see the backport I submitted to go into our kernels as soon as
possible. Upstream unlikely is in a hurry (so I would think it gets there not
before 3.6) because the issue is made covered up there.
-Stefan
>
> Ack, verified to fix the issue, etc. Though I would lean to wait this
> lands up on Linus' tree first.
>
>>
>> From 97dc9f26a508f140cc97eadedddab493619eeee4 Mon Sep 17 00:00:00 2001
>> From: Peter Zijlstra <peterz at infradead.org>
>> Date: Fri, 22 Jun 2012 13:36:00 +0200
>> Subject: [PATCH] UBUNTU: (pre-stable) 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-by: Stefan Bader <stefan.bader at canonical.com>
>> Signed-off-by: Peter Zijlstra <a.p.zijlstra at chello.nl>
>>
>> BugLink: http://bugs.launchpad.net/bugs/999755
>>
>> [backported from patch posted on mailing list]
>> Signed-off-by: Stefan Bader <stefan.bader at canonical.com>
>> ---
>> include/linux/init_task.h | 12 +++++++++++-
>> include/linux/sched.h | 5 ++++-
>> kernel/sched.c | 32 ++++++++++++++++++--------------
>> 3 files changed, 33 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
>> index 32574ee..13b2684 100644
>> --- a/include/linux/init_task.h
>> +++ b/include/linux/init_task.h
>> @@ -117,8 +117,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),
>> @@ -155,6 +164,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, \
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 56de5c1..ac51641 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -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_group *sched_task_group;
>> +#endif
>>
>> #ifdef CONFIG_PREEMPT_NOTIFIERS
>> /* list of struct preempt_notifier: */
>> @@ -2646,7 +2649,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);
>> diff --git a/kernel/sched.c b/kernel/sched.c
>> index aae0c1d..b99a61e 100644
>> --- a/kernel/sched.c
>> +++ b/kernel/sched.c
>> @@ -746,22 +746,19 @@ static inline int cpu_of(struct rq *rq)
>> /*
>> * 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 */
>> @@ -2373,7 +2370,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().
>> @@ -8765,6 +8762,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;
>> @@ -8779,6 +8777,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);
>> --
>> 1.7.9.5
>>
>>
>> --
>> kernel-team mailing list
>> kernel-team at lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>>
>
More information about the kernel-team
mailing list