Re: [PATCH] powerpc/64: ftrace don't trace real mode

[Nicholas Piggin <npiggin@gmail.com>]

Naveen N. Rao's on March 21, 2020 4:39 am:
> Hi Nick,
> 
> Nicholas Piggin wrote:
>> This warns and prevents tracing attempted in a real-mode context.
> 
> Is this something you're seeing often? Last time we looked at this, KVM 
> was the biggest offender and we introduced paca->ftrace_enabled as a way 
> to disable ftrace while in KVM code.

Not often but it has a tendancy to blow up the least tested code at the
worst times :)

Machine check is bad, I'm sure HMI too but I haven't tested that yet.

I've fixed up most of it with annotations, this is obviously an extra
safety not something to rely on like ftrace_enabled. Probably even the
WARN_ON here is dangerous here, but I don't want to leave these bugs
in there.

Although the machine check and hmi code touch a fair bit of stuff and
annotating is a bit fragile. It might actually be better if the
paca->ftrace_enabled could be a nesting counter, then we could use it
in machine checks too and avoid a lot of annotations.

> While this is cheap when handling ftrace_regs_caller() as done in this 
> patch, for simple function tracing (see below), we will have to grab the 
> MSR which will slow things down slightly.

mfmsr is not too bad these days. 


> 
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  arch/powerpc/kernel/trace/ftrace.c            |  3 +++
>>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 19 +++++++++++++++----
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
>> index 7ea0ca044b65..ef965815fcb9 100644
>> --- a/arch/powerpc/kernel/trace/ftrace.c
>> +++ b/arch/powerpc/kernel/trace/ftrace.c
>> @@ -949,6 +949,9 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
>>  {
>>  	unsigned long return_hooker;
>> 
>> +	if (WARN_ON_ONCE((mfmsr() & (MSR_IR|MSR_DR)) != (MSR_IR|MSR_DR)))
>> +		goto out;
>> +
> 
> This is called on function entry to redirect function return to a 
> trampoline if needed. I am not sure if we have (or will have) too many C 
> functions that disable MSR_IR|MSR_DR. Unless the number of such 
> functions is large, it might be preferable to mark specific functions as 
> notrace.
> 
>>  	if (unlikely(ftrace_graph_is_dead()))
>>  		goto out;
>> 
>> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
>> index f9fd5f743eba..6205f15cb603 100644
>> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
>> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
>> @@ -51,16 +51,21 @@ _GLOBAL(ftrace_regs_caller)
>>  	SAVE_10GPRS(12, r1)
>>  	SAVE_10GPRS(22, r1)
>> 
>> -	/* Save previous stack pointer (r1) */
>> -	addi	r8, r1, SWITCH_FRAME_SIZE
>> -	std	r8, GPR1(r1)
>> -
>>  	/* Load special regs for save below */
>>  	mfmsr   r8
>>  	mfctr   r9
>>  	mfxer   r10
>>  	mfcr	r11
>> 
>> +	/* Shouldn't be called in real mode */
>> +	andi.	r3,r8,(MSR_IR|MSR_DR)
>> +	cmpdi	r3,(MSR_IR|MSR_DR)
>> +	bne	ftrace_bad_realmode
>> +
>> +	/* Save previous stack pointer (r1) */
>> +	addi	r8, r1, SWITCH_FRAME_SIZE
>> +	std	r8, GPR1(r1)
>> +
> 
> This stomps on the MSR value in r8, which is saved into pt_regs further 
> below.
> 
> You'll also have to handle ftrace_caller() which is used for simple 
> function tracing. We don't read the MSR there today, but that will be 
> needed if we want to suppress tracing.

Oops, thanks good catch.

Thanks,
Nick