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

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

[Chen Zhongjin]

When meeting ftrace trampolines in orc unwinding, unwinder uses address
of ftrace_{regs_}call address to find the orc, which gets next frame at
sp+176.

If there is an irq hitting at sub $0xa8,%rsp, the next frame should be
sp+8 instead of 176. It makes unwinder skip correct frame and throw
warnings such as "wrong direction" or "can't access registers", etc,
depending on the content of the wrong frame address.

By adding the base address ftrace_{regs_}caller with the offset
*ip - ops->trampoline*,
we can get the correct address to find orc.

Also change "caller" to "tramp_addr" to make variable name conform to
its content.

Fixes: 6be7fa3c74d1 ("ftrace, orc, x86: Handle ftrace dynamically allocated trampolines")
Cc: <stable@vger.kernel.org>
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 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;
 
 	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;
+
+	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)
-- 
2.17.1

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

[Steven Rostedt]

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;


Let's add some comments.

	/* 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)

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

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

[Steven Rostedt]

On Thu, 18 Aug 2022 11:42:17 +0800
Chen Zhongjin <chenzhongjin@huawei.com> wrote:

> 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.
> 

Duh, you're correct. I wasn't paying attention to how we acquired ops. Yes,
if ops->trampoline is NULL, then it will never be returned by
ftrace_ops_trampoline().
 
> >
> > Let's add some comments.  
> 
> Makes sense.
> 
> If the above explanation logic is fine, I'll add this comment and send v2.
> 

Yes, just add the comments for v2.

Thanks,

-- Steve