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

[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@xxxxxxxxxx> 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