Re: [PATCH] powerpc/64: Fix stacktrace on BE when function_graph is enabled

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

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