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

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

[Nicholas Piggin]

This warns and prevents tracing attempted in a real-mode context.

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;
+
 	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)
+
 	/* Get the _mcount() call site out of LR */
 	mflr	r7
 	/* Save it as pt_regs->nip */
@@ -141,6 +146,12 @@ _GLOBAL(ftrace_graph_stub)
 _GLOBAL(ftrace_stub)
 	blr
 
+ftrace_bad_realmode:
+	REST_4GPRS(8, r1)
+#ifdef CONFIG_BUG
+1:	trap
+	EMIT_BUG_ENTRY 1b,__FILE__,__LINE__,(BUGFLAG_WARNING | BUGFLAG_ONCE)
+#endif
 ftrace_no_trace:
 	mflr	r3
 	mtctr	r3
-- 
2.23.0

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

[Naveen N. Rao]

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.

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.

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


- Naveen

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

[Nicholas Piggin]

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

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

[Naveen N. Rao]

Nicholas Piggin wrote:
> 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.

Ok, makes sense.

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

I'm not too familiar with MC/HMI, but I suppose those aren't re-entrant?  
If those have access to an emergency stack, can we save/restore 
ftrace_enabled state across the handlers?

We're primarily disabling ftrace across idle/offline/KVM right now. I'm 
not sure if nesting is useful there.

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

That's great!


Thanks,
Naveen

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

[Nicholas Piggin]

Naveen N. Rao's on March 23, 2020 2:17 am:
> Nicholas Piggin wrote:
>> 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.
> 
> Ok, makes sense.
> 
>> 
>> 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.
> 
> I'm not too familiar with MC/HMI, but I suppose those aren't re-entrant?  
> If those have access to an emergency stack, can we save/restore 
> ftrace_enabled state across the handlers?

Yeah that's true. They're not highly reentrant though, we could just 
make it an 8 bit counter, it's nicer than saving / restoring from stack
(but I guess I could do that too).

> 
> We're primarily disabling ftrace across idle/offline/KVM right now. I'm 
> not sure if nesting is useful there.
> 
>> 
>>> 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. 
> 
> That's great!

Thanks,
Nick