* [PATCH] powerpc/64: Fix stacktrace on BE when function_graph is enabled
@ 2019-08-23 12:29 Michael Ellerman
2019-08-24 8:59 ` Naveen N. Rao
0 siblings, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2019-08-23 12:29 UTC (permalink / raw)
To: linuxppc-dev
Currently if we oops or warn while function_graph is active the stack
trace looks like:
.trace_graph_return+0xac/0x100
.ftrace_return_to_handler+0x98/0x140
.return_to_handler+0x20/0x40
.return_to_handler+0x0/0x40
.return_to_handler+0x0/0x40
.return_to_handler+0x0/0x40
.return_to_handler+0x0/0x40
.return_to_handler+0x0/0x40
.return_to_handler+0x0/0x40
.cpu_startup_entry+0x34/0x40
.start_secondary+0x680/0x6f0
start_secondary_prolog+0x10/0x14
Notice the multiple entries that just show .return_to_handler.
There is logic in show_stack() to detect this case and print the
traced function, but we inadvertently broke it in commit
7d56c65a6ff9 ("powerpc/ftrace: Remove mod_return_to_handler") (2014),
because that commit accidentally removed the dereference of rth which
gets the text address from the function descriptor. Hence this is only
broken on big endian (or technically ELFv1).
Fix it by using the proper accessor, which is ppc_function_entry().
Result is we get a stack trace such as:
.trace_graph_return+0x134/0x160
.ftrace_return_to_handler+0x94/0x140
.return_to_handler+0x20/0x40
.return_to_handler+0x0/0x40 (.shared_cede_loop+0x48/0x130)
.return_to_handler+0x0/0x40 (.cpuidle_enter_state+0xa0/0x690)
.return_to_handler+0x0/0x40 (.cpuidle_enter+0x44/0x70)
.return_to_handler+0x0/0x40 (.call_cpuidle+0x68/0xc0)
.return_to_handler+0x0/0x40 (.do_idle+0x37c/0x400)
.return_to_handler+0x0/0x40 (.cpu_startup_entry+0x30/0x50)
.rest_init+0x224/0x348
Fixes: 7d56c65a6ff9 ("powerpc/ftrace: Remove mod_return_to_handler")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 8fc4de0d22b4..1601d7cfe45e 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -2048,7 +2048,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
struct ftrace_ret_stack *ret_stack;
extern void return_to_handler(void);
- unsigned long rth = (unsigned long)return_to_handler;
+ unsigned long rth = ppc_function_entry(return_to_handler);
int curr_frame = 0;
#endif
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/64: Fix stacktrace on BE when function_graph is enabled
2019-08-23 12:29 [PATCH] powerpc/64: Fix stacktrace on BE when function_graph is enabled Michael Ellerman
@ 2019-08-24 8:59 ` Naveen N. Rao
2019-09-05 9:53 ` Michael Ellerman
0 siblings, 1 reply; 4+ messages in thread
From: Naveen N. Rao @ 2019-08-24 8:59 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman
Michael Ellerman wrote:
> Currently if we oops or warn while function_graph is active the stack
> trace looks like:
> .trace_graph_return+0xac/0x100
> .ftrace_return_to_handler+0x98/0x140
> .return_to_handler+0x20/0x40
> .return_to_handler+0x0/0x40
> .return_to_handler+0x0/0x40
> .return_to_handler+0x0/0x40
> .return_to_handler+0x0/0x40
> .return_to_handler+0x0/0x40
> .return_to_handler+0x0/0x40
> .cpu_startup_entry+0x34/0x40
> .start_secondary+0x680/0x6f0
> start_secondary_prolog+0x10/0x14
>
> Notice the multiple entries that just show .return_to_handler.
>
> There is logic in show_stack() to detect this case and print the
> traced function, but we inadvertently broke it in commit
> 7d56c65a6ff9 ("powerpc/ftrace: Remove mod_return_to_handler") (2014),
> because that commit accidentally removed the dereference of rth which
> gets the text address from the function descriptor. Hence this is only
> broken on big endian (or technically ELFv1).
>
> Fix it by using the proper accessor, which is ppc_function_entry().
> Result is we get a stack trace such as:
>
> .trace_graph_return+0x134/0x160
> .ftrace_return_to_handler+0x94/0x140
> .return_to_handler+0x20/0x40
> .return_to_handler+0x0/0x40 (.shared_cede_loop+0x48/0x130)
> .return_to_handler+0x0/0x40 (.cpuidle_enter_state+0xa0/0x690)
> .return_to_handler+0x0/0x40 (.cpuidle_enter+0x44/0x70)
> .return_to_handler+0x0/0x40 (.call_cpuidle+0x68/0xc0)
> .return_to_handler+0x0/0x40 (.do_idle+0x37c/0x400)
> .return_to_handler+0x0/0x40 (.cpu_startup_entry+0x30/0x50)
> .rest_init+0x224/0x348
>
> Fixes: 7d56c65a6ff9 ("powerpc/ftrace: Remove mod_return_to_handler")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/kernel/process.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 8fc4de0d22b4..1601d7cfe45e 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -2048,7 +2048,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> struct ftrace_ret_stack *ret_stack;
> extern void return_to_handler(void);
> - unsigned long rth = (unsigned long)return_to_handler;
> + unsigned long rth = ppc_function_entry(return_to_handler);
Thanks! This looks good to me. A small suggestion though -- can we use
dereference_kernel_function_descriptor() instead? It will be a nop for
ABIv2, which would be nice, but not really a major deal.
In either case:
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
- Naveen
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/64: Fix stacktrace on BE when function_graph is enabled
2019-08-24 8:59 ` Naveen N. Rao
@ 2019-09-05 9:53 ` Michael Ellerman
2019-09-05 11:35 ` Naveen N. Rao
0 siblings, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2019-09-05 9:53 UTC (permalink / raw)
To: Naveen N. Rao, linuxppc-dev
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> Michael Ellerman wrote:
>> Currently if we oops or warn while function_graph is active the stack
>> trace looks like:
>> .trace_graph_return+0xac/0x100
>> .ftrace_return_to_handler+0x98/0x140
>> .return_to_handler+0x20/0x40
>> .return_to_handler+0x0/0x40
>> .return_to_handler+0x0/0x40
>> .return_to_handler+0x0/0x40
>> .return_to_handler+0x0/0x40
>> .return_to_handler+0x0/0x40
>> .return_to_handler+0x0/0x40
>> .cpu_startup_entry+0x34/0x40
>> .start_secondary+0x680/0x6f0
>> start_secondary_prolog+0x10/0x14
>>
>> Notice the multiple entries that just show .return_to_handler.
>>
>> There is logic in show_stack() to detect this case and print the
>> traced function, but we inadvertently broke it in commit
>> 7d56c65a6ff9 ("powerpc/ftrace: Remove mod_return_to_handler") (2014),
>> because that commit accidentally removed the dereference of rth which
>> gets the text address from the function descriptor. Hence this is only
>> broken on big endian (or technically ELFv1).
>>
>> Fix it by using the proper accessor, which is ppc_function_entry().
>> Result is we get a stack trace such as:
>>
>> .trace_graph_return+0x134/0x160
>> .ftrace_return_to_handler+0x94/0x140
>> .return_to_handler+0x20/0x40
>> .return_to_handler+0x0/0x40 (.shared_cede_loop+0x48/0x130)
>> .return_to_handler+0x0/0x40 (.cpuidle_enter_state+0xa0/0x690)
>> .return_to_handler+0x0/0x40 (.cpuidle_enter+0x44/0x70)
>> .return_to_handler+0x0/0x40 (.call_cpuidle+0x68/0xc0)
>> .return_to_handler+0x0/0x40 (.do_idle+0x37c/0x400)
>> .return_to_handler+0x0/0x40 (.cpu_startup_entry+0x30/0x50)
>> .rest_init+0x224/0x348
>>
>> Fixes: 7d56c65a6ff9 ("powerpc/ftrace: Remove mod_return_to_handler")
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> arch/powerpc/kernel/process.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index 8fc4de0d22b4..1601d7cfe45e 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -2048,7 +2048,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
>> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> struct ftrace_ret_stack *ret_stack;
>> extern void return_to_handler(void);
>> - unsigned long rth = (unsigned long)return_to_handler;
>> + unsigned long rth = ppc_function_entry(return_to_handler);
>
> Thanks! This looks good to me. A small suggestion though -- can we use
> dereference_kernel_function_descriptor() instead? It will be a nop for
> ABIv2, which would be nice, but not really a major deal.
ppc_function_entry() isn't a nop on ABIv2, *if* the function has a local
entry point.
As it happens return_to_handler doesn't have a local entry point, so it
is currently a nop.
But if return_to_handler did have a local entry then
ppc_function_entry() would do the right thing here because we use
ppc_function_entry() in prepare_ftrace_return().
At least I think that's true :)
> In either case:
> Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/64: Fix stacktrace on BE when function_graph is enabled
2019-09-05 9:53 ` Michael Ellerman
@ 2019-09-05 11:35 ` Naveen N. Rao
0 siblings, 0 replies; 4+ messages in thread
From: Naveen N. Rao @ 2019-09-05 11:35 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman
Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>> Michael Ellerman wrote:
>>> Currently if we oops or warn while function_graph is active the stack
>>> trace looks like:
>>> .trace_graph_return+0xac/0x100
>>> .ftrace_return_to_handler+0x98/0x140
>>> .return_to_handler+0x20/0x40
>>> .return_to_handler+0x0/0x40
>>> .return_to_handler+0x0/0x40
>>> .return_to_handler+0x0/0x40
>>> .return_to_handler+0x0/0x40
>>> .return_to_handler+0x0/0x40
>>> .return_to_handler+0x0/0x40
>>> .cpu_startup_entry+0x34/0x40
>>> .start_secondary+0x680/0x6f0
>>> start_secondary_prolog+0x10/0x14
>>>
>>> Notice the multiple entries that just show .return_to_handler.
>>>
>>> There is logic in show_stack() to detect this case and print the
>>> traced function, but we inadvertently broke it in commit
>>> 7d56c65a6ff9 ("powerpc/ftrace: Remove mod_return_to_handler") (2014),
>>> because that commit accidentally removed the dereference of rth which
>>> gets the text address from the function descriptor. Hence this is only
>>> broken on big endian (or technically ELFv1).
>>>
>>> Fix it by using the proper accessor, which is ppc_function_entry().
>>> Result is we get a stack trace such as:
>>>
>>> .trace_graph_return+0x134/0x160
>>> .ftrace_return_to_handler+0x94/0x140
>>> .return_to_handler+0x20/0x40
>>> .return_to_handler+0x0/0x40 (.shared_cede_loop+0x48/0x130)
>>> .return_to_handler+0x0/0x40 (.cpuidle_enter_state+0xa0/0x690)
>>> .return_to_handler+0x0/0x40 (.cpuidle_enter+0x44/0x70)
>>> .return_to_handler+0x0/0x40 (.call_cpuidle+0x68/0xc0)
>>> .return_to_handler+0x0/0x40 (.do_idle+0x37c/0x400)
>>> .return_to_handler+0x0/0x40 (.cpu_startup_entry+0x30/0x50)
>>> .rest_init+0x224/0x348
>>>
>>> Fixes: 7d56c65a6ff9 ("powerpc/ftrace: Remove mod_return_to_handler")
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>> arch/powerpc/kernel/process.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>>> index 8fc4de0d22b4..1601d7cfe45e 100644
>>> --- a/arch/powerpc/kernel/process.c
>>> +++ b/arch/powerpc/kernel/process.c
>>> @@ -2048,7 +2048,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
>>> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>> struct ftrace_ret_stack *ret_stack;
>>> extern void return_to_handler(void);
>>> - unsigned long rth = (unsigned long)return_to_handler;
>>> + unsigned long rth = ppc_function_entry(return_to_handler);
>>
>> Thanks! This looks good to me. A small suggestion though -- can we use
>> dereference_kernel_function_descriptor() instead? It will be a nop for
>> ABIv2, which would be nice, but not really a major deal.
>
> ppc_function_entry() isn't a nop on ABIv2, *if* the function has a local
> entry point.
>
> As it happens return_to_handler doesn't have a local entry point, so it
> is currently a nop.
What I meant was that we still go read the first two instructions to
identify if there is a GEP with ppc_function_entry(). But,
dereference_kernel_function_descriptor() would be compiled out.
>
> But if return_to_handler did have a local entry then
> ppc_function_entry() would do the right thing here because we use
> ppc_function_entry() in prepare_ftrace_return().
>
> At least I think that's true :)
That's a good point :)
However, I think we should never have return_to_handler() with a GEP/LEP
since it is not a regular function.
We should switch use of ppc_function_entry() in prepare_ftrace_return()
to dereference_kernel_function_descriptor(). I will send a patch for
that.
- Naveen
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-05 11:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 12:29 [PATCH] powerpc/64: Fix stacktrace on BE when function_graph is enabled Michael Ellerman
2019-08-24 8:59 ` Naveen N. Rao
2019-09-05 9:53 ` Michael Ellerman
2019-09-05 11:35 ` Naveen N. Rao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).