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

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