[RFC][PATCH 0/2] ftrace/jprobes/x86: Graph trace jprobes if fentry is used

[RFC][PATCH 0/2] ftrace/jprobes/x86: Graph trace jprobes if fentry is used

[Steven Rostedt]

This is built on top of the patches that disable function graph tracer
for jprobes:

  https://lkml.org/lkml/2015/1/14/631

This is actually the first solution I had to deal with jprobes and
function graph tracing, but as I thought the problem only happened
when kprobes used the ftrace infrastructure (fentry), I had a bit
more control of what was happening. But testing showed that the
issue also happens when kprobes does not use fentry, but uses the
breakpoint at the start of the function. Dealing with breakpoints
and simulating commands the breakpoint is on and trying to get
all that to work with function graph tracing was a bit too much,
so I punted (sorry for the American Football reference). I just
made a patch set that disabled function graph tracing on jprobes
all together and I'm pushing that to mainline and stable.

That said, in most cases today, people use x86_64 over i386 and have
newer gcc's that support fentry. That means for the majority of
use cases jprobes can still be traced and we do not need to limit
function graph tracing against them. I took my first solution
and placed it on top of the final solution where if fentry is
supported, we do not disable function graph tracing on jprobes
but instead use the added trampoline tricks to cover it.

The first patch allows for tracing of jprobes and the second patch
lets function graph tracing still trace the function that is being
probed.

Steven Rostedt (Red Hat) (2):
      ftrace/jprobes/x86: Allow jprobes to be graph traced if using fentry
      ftrace/jprobes/x86: Have function being probed be graph traced

----
 arch/x86/include/asm/ftrace.h    |  4 +++
 arch/x86/include/asm/kprobes.h   |  9 +++++
 arch/x86/kernel/kprobes/core.c   | 72 +++++++++++++++++++++++++++++++++++++++-
 arch/x86/kernel/kprobes/ftrace.c | 14 ++++++++
 arch/x86/kernel/mcount_64.S      | 36 +++++++++++++++++++-
 5 files changed, 133 insertions(+), 2 deletions(-)

[RFC][PATCH 1/2] ftrace/jprobes/x86: Allow jprobes to be graph traced if using fentry

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-jprobes-x86-Allow-jprobes-to-be-graph-traced-.patch --]
[-- Type: text/plain, Size: 9427 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Currently when a jprobe handler is called it disables function graph
tracing until the jprobe handler is complete. This means that jprobe
handlers can not be traced, nor can the functions they probe be traced
if fentry is used (the disabling happens around the time the function
graph code is called on the probed function).

If fentry is enabled, then kprobes is used on top of the ftrace
infrastructure when a probe is attached to the beginning of a function
(like jprobes are). In this special case, ftrace has more control of
what happens with the jprobe, and the jprobe can detect that it was
traced.

If the jprobe detects that it was traced by the function graph tracer
(it sees that the return address on th stack was changed to return to
"return_to_handler"), it can return to a fixup function in ftrace that
would allow it to fix the skipped return that it did by calling
jprobe_return().

The solution is two fold. One is to simply add notrace to the
jprobe_return(), as there's no reason to trace it as it is only
a hack for jprobes. The other is to have jprobes detect this in
the breakpoint handler itself. If it sees that the return address
was replaced by the function graph tracer's return_to_handler
hook, it will then call an ftrace helper function that updates
the ftrace return stack data, and will have the regs->ip return
to a fixup_jprobe() handler. When it returns, the handler
will perform the final fixes and return back to the function call.

Note, the function attached to the jprobe may still not be traced
if it is filtered by the function graph tracer, as the regs->ip
is still modified by the jprobe code before it reaches the function
graph code, and it may not match the graph filtered functions.

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/include/asm/ftrace.h    |  3 ++
 arch/x86/include/asm/kprobes.h   |  9 ++++++
 arch/x86/kernel/kprobes/core.c   | 65 +++++++++++++++++++++++++++++++++++++++-
 arch/x86/kernel/kprobes/ftrace.c | 12 ++++++++
 arch/x86/kernel/mcount_64.S      | 24 ++++++++++++++-
 5 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index f45acad3c4b6..bfabbb44797f 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -35,6 +35,9 @@ struct dyn_arch_ftrace {
 
 int ftrace_int3_handler(struct pt_regs *regs);
 
+/* Used to keep jprobes from messing with function graph tracing */
+void fixup_jprobe(void);
+
 #define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR
 
 #endif /*  CONFIG_DYNAMIC_FTRACE */
diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index 4421b5da409d..d4e3d221b896 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -117,4 +117,13 @@ extern int kprobe_exceptions_notify(struct notifier_block *self,
 				    unsigned long val, void *data);
 extern int kprobe_int3_handler(struct pt_regs *regs);
 extern int kprobe_debug_handler(struct pt_regs *regs);
+
+/*
+ * If fentry is being used, then it's OK to function graph trace
+ * jprobes, as it is implented on top of the ftrace infrastructure.
+ */
+#if defined(CC_USING_FENTRY) && defined(CONFIG_FUNCTION_GRAPH_TRACER)
+# define ALLOW_JPROBE_GRAPH_TRACER
+#endif
+
 #endif /* _ASM_X86_KPROBES_H */
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 98f654d466e5..971c3803f283 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -1021,19 +1021,22 @@ int setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 	trace_hardirqs_off();
 	regs->ip = (unsigned long)(jp->entry);
 
+#ifndef ALLOW_JPROBE_GRAPH_TRACER
 	/*
 	 * jprobes use jprobe_return() which skips the normal return
 	 * path of the function, and this messes up the accounting of the
 	 * function graph tracer to get messed up.
 	 *
 	 * Pause function graph tracing while performing the jprobe function.
+	 * Unless we allow for jprobes to be traced.
 	 */
 	pause_graph_tracing();
+#endif
 	return 1;
 }
 NOKPROBE_SYMBOL(setjmp_pre_handler);
 
-void jprobe_return(void)
+void notrace jprobe_return(void)
 {
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
@@ -1072,8 +1075,68 @@ int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 			show_regs(regs);
 			BUG();
 		}
+/*
+ * When using fentry (which is not yet used by i386), the jprobe handlers
+ * may use the ftrace infrastructure to implement the calling of the kprobe
+ * (and jprobe) handlers. The jprobe pre handler copies the stack frame and
+ * changes the ip address to call the jprobe callback. The callback calls
+ * jprobe_return() which does an int3 breakpoint to call the jprobe breakpiont
+ * handler which will put back the stack frame and set the regs->ip back to the
+ * originally traced function.
+ *
+ * The problem is, the stack frame can be changed by the function graph tracer
+ * to have it call return_to_handler() that does the tracing of the function
+ * when it returns. The jprobe would swap back the original stack frame
+ * and lose the update. That would in turn, cause the accounting of the
+ * stack frames in ret_stack to get messed up, which is used to put back the
+ * stack frames that were replaced. The result is that the stack frame
+ * would get the wrong value and cause the function to return to the wrong
+ * place which is most definitely followed by a function oops.
+ *
+ * To make things worse, the jprobe callback return value never gets
+ * called due to it using jprobe_return() instead of return.
+ */
+#ifdef ALLOW_JPROBE_GRAPH_TRACER
+		/*
+		 * If function graph tracing traced the function that the
+		 * jprobe attached to, then the function graph tracing
+		 * would have changed the stack return address to point to
+		 * "return_to_handler". If this is the case, then we need to
+		 * do a bit more work in order to not mess up the function
+		 * graph tracer.
+		 */
+		if (*(void **)saved_sp == return_to_handler) {
+			/*
+			 * The current->ret_stack still has the jprobe callback
+			 * in its list. It can not be removed until we are
+			 * at the function frame that set it (when going back
+			 * to regs->ip). Set the ip to the fixup_jprobe
+			 * trampoline, and the stack to the address that was
+			 * saved by the function graph trace. The fixup_jprobe
+			 * will be able to pop the ret_stack for the jprobe
+			 * handler.
+			 */
+			kcb->jprobe_saved_regs.ip = (unsigned long)fixup_jprobe;
+			/*
+			 * Since we are not returning back to the function
+			 * that was probed, the fixup_jprobe needs a way
+			 * to know what to jump back to. Store that in the
+			 * r10 register which callee functions are allowed
+			 * to clobber. Since r10 can be clobbered by the callee,
+			 * the caller must save it if necessary. As the callee
+			 * (probed function) has not been executed yet, the
+			 * value for r10 currently is not important.
+			 *
+			 * Note, as this only happens with fentry which is
+			 * not supported (yet) by i386, we can use the r10
+			 * field directly here.
+			 */
+			kcb->jprobe_saved_regs.r10 = (unsigned long)p->addr;
+		}
+#else
 		/* It's OK to start function graph tracing again */
 		unpause_graph_tracing();
+#endif
 		*regs = kcb->jprobe_saved_regs;
 		memcpy(saved_sp, kcb->jprobes_stack, MIN_STACK_SIZE(saved_sp));
 		preempt_enable_no_resched();
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 5f8f0b3cc674..0fd23d19ed66 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -29,6 +29,18 @@ static nokprobe_inline
 int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
 		      struct kprobe_ctlblk *kcb, unsigned long orig_ip)
 {
+#ifdef ALLOW_JPROBE_GRAPH_TRACER
+	/*
+	 * If the function graph tracer traced the jprobe entry then
+	 * the ip address would be set to fixup_jprobe, and we do
+	 * not want to modify that (and we expect that orig_ip to be
+	 * zero in this case as well). Make sure that the regs->ip
+	 * is set back to fixup_jprobe on exit.
+	 */
+	if (!orig_ip && regs->ip == (unsigned long)fixup_jprobe)
+		orig_ip = regs->ip;
+#endif
+
 	/*
 	 * Emulate singlestep (and also recover regs->ip)
 	 * as if there is a 5byte nop
diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
index 94ea120fa21f..fe3ccc99530d 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -323,4 +323,26 @@ GLOBAL(return_to_handler)
 	movq (%rsp), %rax
 	addq $24, %rsp
 	jmp *%rdi
-#endif
+
+/*
+ * A jprobe that is traced or the function it traces is traced, then
+ * it needs to have a special verson of return_to_handler that it
+ * gets set from longjmp_break_handler(). This is because the jprobe
+ * callback is not returning to the function that called it, but instead
+ * it is returning to the function it probed.
+ */
+ENTRY(fixup_jprobe)
+	/* longjmp_break_handler() placed the probed function into r10 */
+	addq $MCOUNT_INSN_SIZE, %r10
+	pushq %r10
+	save_mcount_regs
+	/* No need to check frames here */
+	movq $0, %rdi
+	call ftrace_return_to_handler
+	/* Put the return address back to its proper place */
+	movq %rax, MCOUNT_REG_SIZE+8(%rsp)
+	restore_mcount_regs
+	retq
+END(fixup_jprobe)
+
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-- 
2.1.4

[RFC][PATCH 2/2] ftrace/jprobes/x86: Have function being probed be graph traced

[Steven Rostedt]

[-- Attachment #1: 0002-ftrace-jprobes-x86-Have-function-being-probed-be-gra.patch --]
[-- Type: text/plain, Size: 5840 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Because jprobes replaces the stack frame to call the jprobe handler with
the function arguments, if kprobes/jprobes uses ftrace (fentry)
infrastructure for its implementation, it messes with the tracing of
the function graph tracer.

The jprobe gets set up from the ftrace fentry trampoline and it
changes the regs->ip from the called function to call the jprobe
handler. The function graph tracing happens after the function
tracing and the function graph will see the jprobe ip address
instead of the function that was called. If the functions were
filtered, the jprobe ip address would not match what it expected
to see, and the graph tracer will not trace the function.

Add a new trampoline called ftrace_trace_addr that jprobes always
calls if the jprobe itself was not traced, and kprobes uses the
ftrace (fentry) infrastructure. The ftrace_trace_addr will reset
the ip address with the function that was probed and recall the
ftrace_graph_caller.

In case the jprobe itself was traced, the fixup_jprobe will call
ftrace_graph_caller.

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/include/asm/ftrace.h    |  1 +
 arch/x86/kernel/kprobes/core.c   | 33 ++++++++++++++++++++-------------
 arch/x86/kernel/kprobes/ftrace.c |  4 +++-
 arch/x86/kernel/mcount_64.S      | 14 +++++++++++++-
 4 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index bfabbb44797f..d725c816ea05 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -37,6 +37,7 @@ int ftrace_int3_handler(struct pt_regs *regs);
 
 /* Used to keep jprobes from messing with function graph tracing */
 void fixup_jprobe(void);
+void ftrace_trace_addr(void);
 
 #define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR
 
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 971c3803f283..fd6d91a85e3b 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -1098,6 +1098,22 @@ int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
  */
 #ifdef ALLOW_JPROBE_GRAPH_TRACER
 		/*
+		 * Since we are not returning back to the function
+		 * that was probed, the fixup_jprobe and ftrace_trace_addr
+		 * needs a way to know what to jump back to. Store that in the
+		 * r10 register, which callee functions are allowed
+		 * to clobber. Since r10 can be clobbered by the callee,
+		 * the caller must save it if necessary. As the callee
+		 * (probed function) has not been executed yet, the
+		 * value for r10 currently is not important.
+		 *
+		 * Note, as this only happens with fentry which is
+		 * not supported (yet) by i386, we can use the r10
+		 * field directly here.
+		 */
+		kcb->jprobe_saved_regs.r10 = (unsigned long)p->addr;
+
+		/*
 		 * If function graph tracing traced the function that the
 		 * jprobe attached to, then the function graph tracing
 		 * would have changed the stack return address to point to
@@ -1117,21 +1133,12 @@ int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 			 * handler.
 			 */
 			kcb->jprobe_saved_regs.ip = (unsigned long)fixup_jprobe;
+		} else {
 			/*
-			 * Since we are not returning back to the function
-			 * that was probed, the fixup_jprobe needs a way
-			 * to know what to jump back to. Store that in the
-			 * r10 register which callee functions are allowed
-			 * to clobber. Since r10 can be clobbered by the callee,
-			 * the caller must save it if necessary. As the callee
-			 * (probed function) has not been executed yet, the
-			 * value for r10 currently is not important.
-			 *
-			 * Note, as this only happens with fentry which is
-			 * not supported (yet) by i386, we can use the r10
-			 * field directly here.
+			 * See if function graph tracing is enabled and
+			 * trace this function if necessary.
 			 */
-			kcb->jprobe_saved_regs.r10 = (unsigned long)p->addr;
+			kcb->jprobe_saved_regs.ip = ftrace_trace_addr;
 		}
 #else
 		/* It's OK to start function graph tracing again */
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 0fd23d19ed66..317377f1df26 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -37,7 +37,9 @@ int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
 	 * zero in this case as well). Make sure that the regs->ip
 	 * is set back to fixup_jprobe on exit.
 	 */
-	if (!orig_ip && regs->ip == (unsigned long)fixup_jprobe)
+	if (!orig_ip &&
+	    (regs->ip == (unsigned long)fixup_jprobe ||
+	     regs->ip == (unsigned long)ftrace_trace_addr))
 		orig_ip = regs->ip;
 #endif
 
diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
index fe3ccc99530d..b6caf6d1e10e 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -333,7 +333,6 @@ GLOBAL(return_to_handler)
  */
 ENTRY(fixup_jprobe)
 	/* longjmp_break_handler() placed the probed function into r10 */
-	addq $MCOUNT_INSN_SIZE, %r10
 	pushq %r10
 	save_mcount_regs
 	/* No need to check frames here */
@@ -341,8 +340,21 @@ ENTRY(fixup_jprobe)
 	call ftrace_return_to_handler
 	/* Put the return address back to its proper place */
 	movq %rax, MCOUNT_REG_SIZE+8(%rsp)
+	movq MCOUNT_REG_SIZE(%rsp), %rdi
+	leaq MCOUNT_REG_SIZE+8(%rsp), %rsi
+	movq $0, %rdx	/* No framepointers needed */
+	call prepare_ftrace_return
 	restore_mcount_regs
+	/* Skip over the fentry call */
+	addq $MCOUNT_INSN_SIZE, 0(%rsp)
 	retq
 END(fixup_jprobe)
 
+ENTRY(ftrace_trace_addr)
+	/* longjmp_break_handler() saved our return address in r10 */
+	pushq %r10
+	addq $MCOUNT_INSN_SIZE, 0(%rsp)
+	jmp ftrace_graph_caller
+END(ftrace_trace_addr)
+
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-- 
2.1.4