Re: [PATCH] x86/unwind/orc: unwind ftrace trampolines with correct orc

[Chen Zhongjin <chenzhongjin@huawei.com>]

Thanks for review!


On 2022/8/18 10:28, Steven Rostedt wrote:
> On Thu, 18 Aug 2022 09:55:25 +0800
> Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>
>
>>   arch/x86/kernel/unwind_orc.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
>> index 38185aedf7d1..a938c5d0ed6f 100644
>> --- a/arch/x86/kernel/unwind_orc.c
>> +++ b/arch/x86/kernel/unwind_orc.c
>> @@ -93,22 +93,25 @@ static struct orc_entry *orc_find(unsigned long ip);
>>   static struct orc_entry *orc_ftrace_find(unsigned long ip)
>>   {
>>   	struct ftrace_ops *ops;
>> -	unsigned long caller;
>> +	unsigned long tramp_addr, offset;
>>   
>>   	ops = ftrace_ops_trampoline(ip);
>>   	if (!ops)
>>   		return NULL;
>>   
> Now if this is that unlikely recursion mentioned below then ops->trampoline
> will be NULL, and if we do that offset addition, it will be incorrect.
>
> Perhaps we should add here:
>
> 	if (!ops->trampoline)
> 		return NULL;

I think when this will return NULL and then stop at orc_find:`if (ip == 
0)` and return null_orc_entry.

And in ftrace_ops_trampoline: `if (op->trampoline && 
op->trampoline_size)` which promise !ops->trampoline when !ops.


IIUC the In unlikely recursion below means if orc_find(ftrace_call) 
can't find any orc it will enter orc_ftrace_find(ftrace_call).

If we dont check ip==caller then,

ftrace_ops_trampoline(ftrace_call) causes orc_find(ftrace_call) again 
(I'm not 100% sure it will)

and it will be trapped in recursion


When here is an offset we can still protect this scenario when 
orc_find(ftrace_caller + offset) and check ip == ftrace_caller + offset.

>
> Let's add some comments.

Makes sense.

If the above explanation logic is fine, I'll add this comment and send v2.

>
> 	/* Set tramp_addr to the start of the code copied by the trampoline */
>
>>   	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS)
>> -		caller = (unsigned long)ftrace_regs_call;
>> +		tramp_addr = (unsigned long)ftrace_regs_caller;
>>   	else
>> -		caller = (unsigned long)ftrace_call;
>> +		tramp_addr = (unsigned long)ftrace_caller;
>> +
> 	/* Now place tramp_addr to the location within the trampoline ip is at */
>
>> +	offset = ip - ops->trampoline;
>> +	tramp_addr += offset;
>>   
>>   	/* Prevent unlikely recursion */
>> -	if (ip == caller)
>> +	if (ip == tramp_addr)
>>   		return NULL;
>>   
>> -	return orc_find(caller);
>> +	return orc_find(tramp_addr);
>>   }
>>   #else
>>   static struct orc_entry *orc_ftrace_find(unsigned long ip)

Best,

Chen