Messages in this thread | | | Date | Thu, 18 Aug 2022 11:42:17 +0800 | Subject | Re: [PATCH] x86/unwind/orc: unwind ftrace trampolines with correct orc | From | Chen Zhongjin <> |
| |
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
| |