Always try and CC people who wrote the code..
On Fri, 2012-10-19 at 16:36 +0800, Xiaotian Feng wrote:
> There's a regression from commit 800d4d30, in autogroup_move_group()
>
> p->signal->autogroup = autogroup_kref_get(ag);
>
> if (!ACCESS_ONCE(sysctl_sched_autogroup_enabled))
> goto out;
> ...
> out:
> autogroup_kref_put(prev);
>
> So kernel changed p's autogroup to ag, but never sched_move_task(p).
> Then previous autogroup of p is released, which may release task_group
> related with p. After commit 8323f26ce, p->sched_task_group might point
> to this stale value, and thus caused kernel crashes.
>
> This is very easy to reproduce, add "kernel.sched_autogroup_enabled = 0"
> to your /etc/sysctl.conf, your system will never boot up. It is not reasonable
> to put the sysctl enabled check in autogroup_move_group(), kernel should check
> it before autogroup_create in sched_autogroup_create_attach().
>
> Reported-by: cwillu <cwi...@cwillu.com>
> Reported-by: Luis Henriques <luis.henriq...@canonical.com>
> Signed-off-by: Xiaotian Feng <dannyf...@tencent.com>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: Peter Zijlstra <pet...@infradead.org>
> ---
> kernel/sched/auto_group.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/sched/auto_group.c b/kernel/sched/auto_group.c
> index 0984a21..ac62415 100644
> --- a/kernel/sched/auto_group.c
> +++ b/kernel/sched/auto_group.c
> @@ -143,15 +143,11 @@ autogroup_move_group(struct task_struct *p, struct
> autogroup *ag)
>
> p->signal->autogroup = autogroup_kref_get(ag);
>
> - if (!ACCESS_ONCE(sysctl_sched_autogroup_enabled))
> - goto out;
> -
> t = p;
> do {
> sched_move_task(t);
> } while_each_thread(p, t);
>
> -out:
> unlock_task_sighand(p, &flags);
> autogroup_kref_put(prev);
> }
So I've looked at this for all of 1 minute, but why isn't moving that
check up one line to be above the p->signal->autogroup assignment
enough?
> @@ -159,8 +155,12 @@ out:
> /* Allocates GFP_KERNEL, cannot be called under any spinlock */
> void sched_autogroup_create_attach(struct task_struct *p)
> {
> - struct autogroup *ag = autogroup_create();
> + struct autogroup *ag;
> +
> + if (!ACCESS_ONCE(sysctl_sched_autogroup_enabled))
> + return;
>
> + ag = autogroup_create();
> autogroup_move_group(p, ag);
> /* drop extra reference added by autogroup_create() */
> autogroup_kref_put(ag);
Man,.. so on memory allocation fail we'll put the group in
autogroup_default, which I think ends up being the root cgroup.
But what happens when sysctl_sched_autogroup_enabled is false?
It looks like sched_autogroup_fork() is effective in that case, which
would mean we'll stay in whatever group our parent is in, which is not
the same as being disabled.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
