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

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

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.

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.

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

Naveen N. Rao's on March 21, 2020 4:39 am:

Hi Nick,

Nicholas Piggin wrote:

quoted

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. 

quoted

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.

quoted

 	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

Nicholas Piggin wrote:

Naveen N. Rao's on March 21, 2020 4:39 am:

quoted

Hi Nick,

Nicholas Piggin wrote:

quoted

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.

quoted

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

Naveen N. Rao's on March 23, 2020 2:17 am:

Nicholas Piggin wrote:

quoted

Naveen N. Rao's on March 21, 2020 4:39 am:

quoted

Hi Nick,

Nicholas Piggin wrote:

quoted

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.

quoted

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.

quoted

quoted

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