Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <24556707-99b3-ec3c-c176-cb73e1d030d8@huawei.com>
Date:   Thu, 18 Aug 2022 11:42:17 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.0
Subject: Re: [PATCH] x86/unwind/orc: unwind ftrace trampolines with correct
 orc
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     <linux-kernel@vger.kernel.org>, <jpoimboe@kernel.org>,
        <peterz@infradead.org>, <tglx@linutronix.de>, <mingo@redhat.com>,
        <bp@alien8.de>, <dave.hansen@linux.intel.com>, <x86@kernel.org>,
        <hpa@zytor.com>
References: <20220818015525.222053-1-chenzhongjin@huawei.com>
 <20220817222836.72aa77bd@gandalf.local.home>
Content-Language: en-US
From:   Chen Zhongjin <chenzhongjin@huawei.com>
In-Reply-To: <20220817222836.72aa77bd@gandalf.local.home>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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