lkml.org 
[lkml]   [2022]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] x86/unwind/orc: unwind ftrace trampolines with correct orc
From
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


\
 
 \ /
  Last update: 2022-08-18 05:44    [W:2.111 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site