* stack traces and zombie tasks
@ 2017-12-15 12:54 Miroslav Benes
2017-12-15 15:51 ` Andy Lutomirski
0 siblings, 1 reply; 6+ messages in thread
From: Miroslav Benes @ 2017-12-15 12:54 UTC (permalink / raw)
To: luto, jpoimboe; +Cc: x86, linux-kernel, live-patching
Hi,
commit 1959a60182f4 ("x86/dumpstack: Pin the target stack when dumping
it") slightly changed the behaviour of stack traces dumping for zombie
tasks.
Before the commit (well, this is older SLE12 kernel, but that should not
matter), if one called 'cat /proc/<zombie pid>/stack', they would get
something like this
[<ffffffff8105b877>] do_exit+0x6f7/0xa80
[<ffffffff8105bc79>] do_group_exit+0x39/0xa0
[<ffffffff8105bcf0>] __wake_up_parent+0x0/0x30
[<ffffffff8152dd09>] system_call_fastpath+0x16/0x1b
[<00007fd128f9c4f9>] 0x7fd128f9c4f9
[<ffffffffffffffff>] 0xffffffffffffffff
After, one gets nothing. The trace is empty. try_get_task_stack() contains
atomic_inc_not_zero() (CONFIG_THREAD_INFO_IN_TASK is now default on
x86_64) and because stack_refcount is 0 for a zombie task, it returns
NULL. Therefore, all save_stack_trace_*() functions return immediately.
I guess that no one has cared about it so far. There is a problem for
live patching though. save_stack_trace_tsk_reliable() returns -EINVAL for
the zombie task and its stack is deemed unreliable. It could block our
transition for quite a long time.
We can skip those tasks in kernel/livepatch/ with a simple test we have in
kGraft. Skip the task if (task->state == TASK_DEAD && task->on_cpu == 0).
But you may want to change it generally, so better to ask first.
Regards,
Miroslav
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: stack traces and zombie tasks
2017-12-15 12:54 stack traces and zombie tasks Miroslav Benes
@ 2017-12-15 15:51 ` Andy Lutomirski
2017-12-18 3:34 ` Josh Poimboeuf
0 siblings, 1 reply; 6+ messages in thread
From: Andy Lutomirski @ 2017-12-15 15:51 UTC (permalink / raw)
To: Miroslav Benes
Cc: Andrew Lutomirski, Josh Poimboeuf, X86 ML, linux-kernel, live-patching
On Fri, Dec 15, 2017 at 4:54 AM, Miroslav Benes <mbenes@suse.cz> wrote:
> Hi,
>
> commit 1959a60182f4 ("x86/dumpstack: Pin the target stack when dumping
> it") slightly changed the behaviour of stack traces dumping for zombie
> tasks.
>
> Before the commit (well, this is older SLE12 kernel, but that should not
> matter), if one called 'cat /proc/<zombie pid>/stack', they would get
> something like this
>
> [<ffffffff8105b877>] do_exit+0x6f7/0xa80
> [<ffffffff8105bc79>] do_group_exit+0x39/0xa0
> [<ffffffff8105bcf0>] __wake_up_parent+0x0/0x30
> [<ffffffff8152dd09>] system_call_fastpath+0x16/0x1b
> [<00007fd128f9c4f9>] 0x7fd128f9c4f9
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> After, one gets nothing. The trace is empty. try_get_task_stack() contains
> atomic_inc_not_zero() (CONFIG_THREAD_INFO_IN_TASK is now default on
> x86_64) and because stack_refcount is 0 for a zombie task, it returns
> NULL. Therefore, all save_stack_trace_*() functions return immediately.
>
> I guess that no one has cared about it so far. There is a problem for
> live patching though. save_stack_trace_tsk_reliable() returns -EINVAL for
> the zombie task and its stack is deemed unreliable. It could block our
> transition for quite a long time.
>
> We can skip those tasks in kernel/livepatch/ with a simple test we have in
> kGraft. Skip the task if (task->state == TASK_DEAD && task->on_cpu == 0).
> But you may want to change it generally, so better to ask first.
>
Sounds like a bug in save_stack_trace_tsk_reliable() to me: if the
task has no stack, then the trace is 100% definitely empty :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: stack traces and zombie tasks
2017-12-15 15:51 ` Andy Lutomirski
@ 2017-12-18 3:34 ` Josh Poimboeuf
2017-12-18 13:07 ` Miroslav Benes
0 siblings, 1 reply; 6+ messages in thread
From: Josh Poimboeuf @ 2017-12-18 3:34 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Miroslav Benes, X86 ML, linux-kernel, live-patching
On Fri, Dec 15, 2017 at 07:51:45AM -0800, Andy Lutomirski wrote:
> On Fri, Dec 15, 2017 at 4:54 AM, Miroslav Benes <mbenes@suse.cz> wrote:
> > Hi,
> >
> > commit 1959a60182f4 ("x86/dumpstack: Pin the target stack when dumping
> > it") slightly changed the behaviour of stack traces dumping for zombie
> > tasks.
> >
> > Before the commit (well, this is older SLE12 kernel, but that should not
> > matter), if one called 'cat /proc/<zombie pid>/stack', they would get
> > something like this
> >
> > [<ffffffff8105b877>] do_exit+0x6f7/0xa80
> > [<ffffffff8105bc79>] do_group_exit+0x39/0xa0
> > [<ffffffff8105bcf0>] __wake_up_parent+0x0/0x30
> > [<ffffffff8152dd09>] system_call_fastpath+0x16/0x1b
> > [<00007fd128f9c4f9>] 0x7fd128f9c4f9
> > [<ffffffffffffffff>] 0xffffffffffffffff
> >
> > After, one gets nothing. The trace is empty. try_get_task_stack() contains
> > atomic_inc_not_zero() (CONFIG_THREAD_INFO_IN_TASK is now default on
> > x86_64) and because stack_refcount is 0 for a zombie task, it returns
> > NULL. Therefore, all save_stack_trace_*() functions return immediately.
> >
> > I guess that no one has cared about it so far. There is a problem for
> > live patching though. save_stack_trace_tsk_reliable() returns -EINVAL for
> > the zombie task and its stack is deemed unreliable. It could block our
> > transition for quite a long time.
> >
> > We can skip those tasks in kernel/livepatch/ with a simple test we have in
> > kGraft. Skip the task if (task->state == TASK_DEAD && task->on_cpu == 0).
> > But you may want to change it generally, so better to ask first.
> >
>
> Sounds like a bug in save_stack_trace_tsk_reliable() to me: if the
> task has no stack, then the trace is 100% definitely empty :)
I would agree with that, something like the following should fix it?
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 77835bc021c7..20161ef53537 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -164,8 +164,12 @@ int save_stack_trace_tsk_reliable(struct task_struct *tsk,
{
int ret;
+ /*
+ * If the task doesn't have a stack (e.g., a zombie), the stack is
+ * "reliably" empty.
+ */
if (!try_get_task_stack(tsk))
- return -EINVAL;
+ return 0;
ret = __save_stack_trace_reliable(trace, tsk);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: stack traces and zombie tasks
2017-12-18 3:34 ` Josh Poimboeuf
@ 2017-12-18 13:07 ` Miroslav Benes
2017-12-19 7:50 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Miroslav Benes @ 2017-12-18 13:07 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: Andy Lutomirski, X86 ML, linux-kernel, live-patching
On Sun, 17 Dec 2017, Josh Poimboeuf wrote:
> On Fri, Dec 15, 2017 at 07:51:45AM -0800, Andy Lutomirski wrote:
> > On Fri, Dec 15, 2017 at 4:54 AM, Miroslav Benes <mbenes@suse.cz> wrote:
> > > Hi,
> > >
> > > commit 1959a60182f4 ("x86/dumpstack: Pin the target stack when dumping
> > > it") slightly changed the behaviour of stack traces dumping for zombie
> > > tasks.
> > >
> > > Before the commit (well, this is older SLE12 kernel, but that should not
> > > matter), if one called 'cat /proc/<zombie pid>/stack', they would get
> > > something like this
> > >
> > > [<ffffffff8105b877>] do_exit+0x6f7/0xa80
> > > [<ffffffff8105bc79>] do_group_exit+0x39/0xa0
> > > [<ffffffff8105bcf0>] __wake_up_parent+0x0/0x30
> > > [<ffffffff8152dd09>] system_call_fastpath+0x16/0x1b
> > > [<00007fd128f9c4f9>] 0x7fd128f9c4f9
> > > [<ffffffffffffffff>] 0xffffffffffffffff
> > >
> > > After, one gets nothing. The trace is empty. try_get_task_stack() contains
> > > atomic_inc_not_zero() (CONFIG_THREAD_INFO_IN_TASK is now default on
> > > x86_64) and because stack_refcount is 0 for a zombie task, it returns
> > > NULL. Therefore, all save_stack_trace_*() functions return immediately.
> > >
> > > I guess that no one has cared about it so far. There is a problem for
> > > live patching though. save_stack_trace_tsk_reliable() returns -EINVAL for
> > > the zombie task and its stack is deemed unreliable. It could block our
> > > transition for quite a long time.
> > >
> > > We can skip those tasks in kernel/livepatch/ with a simple test we have in
> > > kGraft. Skip the task if (task->state == TASK_DEAD && task->on_cpu == 0).
> > > But you may want to change it generally, so better to ask first.
> > >
> >
> > Sounds like a bug in save_stack_trace_tsk_reliable() to me: if the
> > task has no stack, then the trace is 100% definitely empty :)
>
> I would agree with that, something like the following should fix it?
>
> diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
> index 77835bc021c7..20161ef53537 100644
> --- a/arch/x86/kernel/stacktrace.c
> +++ b/arch/x86/kernel/stacktrace.c
> @@ -164,8 +164,12 @@ int save_stack_trace_tsk_reliable(struct task_struct *tsk,
> {
> int ret;
>
> + /*
> + * If the task doesn't have a stack (e.g., a zombie), the stack is
> + * "reliably" empty.
> + */
> if (!try_get_task_stack(tsk))
> - return -EINVAL;
> + return 0;
>
> ret = __save_stack_trace_reliable(trace, tsk);
This obviously fixes the problem, so you can add
Reported-and-tested-by: Miroslav Benes <mbenes@suse.cz>
Thanks
Miroslav
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: stack traces and zombie tasks
2017-12-18 13:07 ` Miroslav Benes
@ 2017-12-19 7:50 ` Ingo Molnar
2017-12-19 8:00 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2017-12-19 7:50 UTC (permalink / raw)
To: Miroslav Benes
Cc: Josh Poimboeuf, Andy Lutomirski, X86 ML, linux-kernel, live-patching
* Miroslav Benes <mbenes@suse.cz> wrote:
> On Sun, 17 Dec 2017, Josh Poimboeuf wrote:
>
> > On Fri, Dec 15, 2017 at 07:51:45AM -0800, Andy Lutomirski wrote:
> > > On Fri, Dec 15, 2017 at 4:54 AM, Miroslav Benes <mbenes@suse.cz> wrote:
> > > > Hi,
> > > >
> > > > commit 1959a60182f4 ("x86/dumpstack: Pin the target stack when dumping
> > > > it") slightly changed the behaviour of stack traces dumping for zombie
> > > > tasks.
> > > >
> > > > Before the commit (well, this is older SLE12 kernel, but that should not
> > > > matter), if one called 'cat /proc/<zombie pid>/stack', they would get
> > > > something like this
> > > >
> > > > [<ffffffff8105b877>] do_exit+0x6f7/0xa80
> > > > [<ffffffff8105bc79>] do_group_exit+0x39/0xa0
> > > > [<ffffffff8105bcf0>] __wake_up_parent+0x0/0x30
> > > > [<ffffffff8152dd09>] system_call_fastpath+0x16/0x1b
> > > > [<00007fd128f9c4f9>] 0x7fd128f9c4f9
> > > > [<ffffffffffffffff>] 0xffffffffffffffff
> > > >
> > > > After, one gets nothing. The trace is empty. try_get_task_stack() contains
> > > > atomic_inc_not_zero() (CONFIG_THREAD_INFO_IN_TASK is now default on
> > > > x86_64) and because stack_refcount is 0 for a zombie task, it returns
> > > > NULL. Therefore, all save_stack_trace_*() functions return immediately.
> > > >
> > > > I guess that no one has cared about it so far. There is a problem for
> > > > live patching though. save_stack_trace_tsk_reliable() returns -EINVAL for
> > > > the zombie task and its stack is deemed unreliable. It could block our
> > > > transition for quite a long time.
> > > >
> > > > We can skip those tasks in kernel/livepatch/ with a simple test we have in
> > > > kGraft. Skip the task if (task->state == TASK_DEAD && task->on_cpu == 0).
> > > > But you may want to change it generally, so better to ask first.
> > > >
> > >
> > > Sounds like a bug in save_stack_trace_tsk_reliable() to me: if the
> > > task has no stack, then the trace is 100% definitely empty :)
> >
> > I would agree with that, something like the following should fix it?
> >
> > diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
> > index 77835bc021c7..20161ef53537 100644
> > --- a/arch/x86/kernel/stacktrace.c
> > +++ b/arch/x86/kernel/stacktrace.c
> > @@ -164,8 +164,12 @@ int save_stack_trace_tsk_reliable(struct task_struct *tsk,
> > {
> > int ret;
> >
> > + /*
> > + * If the task doesn't have a stack (e.g., a zombie), the stack is
> > + * "reliably" empty.
> > + */
> > if (!try_get_task_stack(tsk))
> > - return -EINVAL;
> > + return 0;
> >
> > ret = __save_stack_trace_reliable(trace, tsk);
>
> This obviously fixes the problem, so you can add
>
> Reported-and-tested-by: Miroslav Benes <mbenes@suse.cz>
Great.
Josh, mind sending a changelogged version, or should I distill a commit out of
this discussion, for tip:x86/urgent?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: stack traces and zombie tasks
2017-12-19 7:50 ` Ingo Molnar
@ 2017-12-19 8:00 ` Ingo Molnar
0 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2017-12-19 8:00 UTC (permalink / raw)
To: Miroslav Benes
Cc: Josh Poimboeuf, Andy Lutomirski, X86 ML, linux-kernel, live-patching
* Ingo Molnar <mingo@kernel.org> wrote:
> > This obviously fixes the problem, so you can add
> >
> > Reported-and-tested-by: Miroslav Benes <mbenes@suse.cz>
>
> Great.
>
> Josh, mind sending a changelogged version, or should I distill a commit out of
> this discussion, for tip:x86/urgent?
... never mind, later in my mbox I found the patch that Josh already submitted!
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-19 8:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-15 12:54 stack traces and zombie tasks Miroslav Benes
2017-12-15 15:51 ` Andy Lutomirski
2017-12-18 3:34 ` Josh Poimboeuf
2017-12-18 13:07 ` Miroslav Benes
2017-12-19 7:50 ` Ingo Molnar
2017-12-19 8:00 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome