[Natty, Oneiric, Precise SRU] Fix race in task_group()
Herton Ronaldo Krzesinski
herton.krzesinski at canonical.com
Tue Jul 3 15:17:51 UTC 2012
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
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
>
--
[]'s
Herton
More information about the kernel-team
mailing list