ftrace/x86_32: Ftrace cleanup and add support for -mfentry

From: "Steven Rostedt (VMware)" <[email protected]>

The function hook ftrace_caller does not create its own stack frame, and this causes the ftrace stack trace to miss the first function when doing stack traces.

 # echo schedule:stacktrace > /sys/kernel/tracing/set_ftrace_filter

Before:

         <idle>-0     [002] .N..    29.865807: <stack trace>
 => cpu_startup_entry
 => start_secondary
 => startup_32_smp
           <...>-7     [001] ....    29.866509: <stack trace>
 => kthread
 => ret_from_fork
           <...>-1     [000] ....    29.865377: <stack trace>

=> poll_schedule_timeout
=> do_select
=> core_sys_select
=> SyS_select
=> do_fast_syscall_32
=> entry_SYSENTER_32

After:

          <idle>-0     [002] .N..    31.234853: <stack trace>
 => do_idle
 => cpu_startup_entry
 => start_secondary
 => startup_32_smp
           <...>-7     [003] ....    31.235140: <stack trace>
 => rcu_gp_kthread
 => kthread
 => ret_from_fork
           <...>-1819  [000] ....    31.264172: <stack trace>

=> schedule_hrtimeout_range
=> poll_schedule_timeout
=> do_sys_poll
=> SyS_ppoll
=> do_fast_syscall_32
=> entry_SYSENTER_32

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 arch/x86/kernel/ftrace_32.S | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/x86/kernel/ftrace_32.S +0 -0

index 4ff30f31f0a5..73d61a62649d 100644

From: "Steven Rostedt (VMware)" <[email protected]>

When ftrace_regs_caller was created, it was designed to preserve flags as much as possible as it needed to act just like a breakpoint triggered on the same location. But the design is over complicated as it treated all operations as modifying flags. But push, mov and lea do not modify flags. This means the code can become more simplified by allowing flags to be stored further down.

Making ftrace_regs_caller simpler will also be useful in implementing fentry logic.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 arch/x86/kernel/ftrace_32.S | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

arch/x86/kernel/ftrace_32.S +7 -5

index 73d61a62649d..8ca33d9806ac 100644
@@ -55,23 +55,30 @@ WEAK(ftrace_stub)
 END(ftrace_caller)
 
 ENTRY(ftrace_regs_caller)
-	pushf	/* push flags before compare (in cs location) */
-
 	/*
 	 * i386 does not save SS and ESP when coming from kernel.
 	 * Instead, to get sp, &regs->sp is used (see ptrace.h).
 	 * Unfortunately, that means eflags must be at the same location
 	 * as the current return ip is. We move the return ip into the
-	 * ip location, and move flags into the return ip location.
-	 */
-	pushl	4(%esp)				/* save return ip into ip slot */
+	 * regs->ip location, and move flags into the return ip location.
+         */
+	pushl	$__KERNEL_CS
+	pushl	4(%esp)				/* Save the return ip */
+
+	/* temporarily save flags in the orig_ax location */
+	pushf
 

Quoted 43 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

When ftrace_regs_caller was created, it was designed to preserve flags as much as possible as it needed to act just like a breakpoint triggered on the same location. But the design is over complicated as it treated all operations as modifying flags. But push, mov and lea do not modify flags. This means the code can become more simplified by allowing flags to be stored further down.

Making ftrace_regs_caller simpler will also be useful in implementing fentry logic.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 arch/x86/kernel/ftrace_32.S | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

arch/x86/kernel/ftrace_32.S +7 -5

index 73d61a62649d..8ca33d9806ac 100644
@@ -55,23 +55,30 @@ WEAK(ftrace_stub)
 END(ftrace_caller)
 
 ENTRY(ftrace_regs_caller)
-	pushf	/* push flags before compare (in cs location) */
-
 	/*
 	 * i386 does not save SS and ESP when coming from kernel.
 	 * Instead, to get sp, &regs->sp is used (see ptrace.h).
 	 * Unfortunately, that means eflags must be at the same location
 	 * as the current return ip is. We move the return ip into the
-	 * ip location, and move flags into the return ip location.
-	 */
-	pushl	4(%esp)				/* save return ip into ip slot */
+	 * regs->ip location, and move flags into the return ip location.
+         */
+	pushl	$__KERNEL_CS
+	pushl	4(%esp)				/* Save the return ip */
+
+	/* temporarily save flags in the orig_ax location */
+	pushf
 

arch/x86/kernel/ftrace_32.S WEAK(ftrace_stub) · line 79

Quoted 10 lines

arch/x86/kernel/ftrace_32.S +4 -1

-	pushl	$0				/* Load 0 into orig_ax */
 	pushl	%gs
 	pushl	%fs
 	pushl	%es
 	pushl	%ds
 	pushl	%eax
+
+	/* move flags into the location of where the return ip was */
+	movl	5*4(%esp), %eax
+	movl	$0, (%esp)			/* Load 0 into orig_ax */  

This writes a 0 into regs->ax instead of regs->orig_ax.

Quoted 44 lines · On Wed, 15 Mar 2017 16:13:43 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 43 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

When ftrace_regs_caller was created, it was designed to preserve flags as much as possible as it needed to act just like a breakpoint triggered on the same location. But the design is over complicated as it treated all operations as modifying flags. But push, mov and lea do not modify flags. This means the code can become more simplified by allowing flags to be stored further down.

Making ftrace_regs_caller simpler will also be useful in implementing fentry logic.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 arch/x86/kernel/ftrace_32.S | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

arch/x86/kernel/ftrace_32.S +7 -5

index 73d61a62649d..8ca33d9806ac 100644
@@ -55,23 +55,30 @@ WEAK(ftrace_stub)
 END(ftrace_caller)
 
 ENTRY(ftrace_regs_caller)
-	pushf	/* push flags before compare (in cs location) */
-
 	/*
 	 * i386 does not save SS and ESP when coming from kernel.
 	 * Instead, to get sp, &regs->sp is used (see ptrace.h).
 	 * Unfortunately, that means eflags must be at the same location
 	 * as the current return ip is. We move the return ip into the
-	 * ip location, and move flags into the return ip location.
-	 */
-	pushl	4(%esp)				/* save return ip into ip slot */
+	 * regs->ip location, and move flags into the return ip location.
+         */
+	pushl	$__KERNEL_CS
+	pushl	4(%esp)				/* Save the return ip */
+
+	/* temporarily save flags in the orig_ax location */
+	pushf
 

Quoted 13 lines

arch/x86/kernel/ftrace_32.S WEAK(ftrace_stub) · line 79

Quoted 10 lines

arch/x86/kernel/ftrace_32.S +4 -1

-	pushl	$0				/* Load 0 into orig_ax */
 	pushl	%gs
 	pushl	%fs
 	pushl	%es
 	pushl	%ds
 	pushl	%eax
+
+	/* move flags into the location of where the return ip was */
+	movl	5*4(%esp), %eax
+	movl	$0, (%esp)			/* Load 0 into orig_ax */  

This writes a 0 into regs->ax instead of regs->orig_ax.

Ouch, I'm surprised this didn't crash my box. Thanks for catching it.

arch/x86/kernel/ftrace_32.S +8 -0

+#ifdef USING_FRAME_POINTER
+# ifdef CC_USING_FENTRY
+	/*
+	 * Frame pointers are of ip followed by bp.
+	 * Since fentry is an immediate jump, we are left with
+	 * parent-ip, function-ip. We need to add a frame with
+	 * parent-ip followed by ebp.
+	 */

Quoted 8 lines · Steven Rostedt

arch/x86/kernel/ftrace_32.S +8 -0

+#ifdef USING_FRAME_POINTER
+# ifdef CC_USING_FENTRY
+	/*
+	 * Frame pointers are of ip followed by bp.
+	 * Since fentry is an immediate jump, we are left with
+	 * parent-ip, function-ip. We need to add a frame with
+	 * parent-ip followed by ebp.
+	 */

arch/x86/kernel/ftrace_32.S line 63

Quoted 10 lines

arch/x86/kernel/ftrace_32.S +7 -1

+	pushl 4(%esp)				/* parent ip */
 	pushl %ebp
 	movl %esp, %ebp
-
+	pushl 2*4(%esp)				/* function ip */
+# endif
+	/* For mcount, the function ip is directly above */
+	pushl %ebp
+	movl %esp, %ebp
+#endif  

More vertical operand alignment issues.

arch/x86/kernel/ftrace_32.S ENTRY(ftrace_regs_caller) · line 146

Quoted 8 lines

arch/x86/kernel/ftrace_32.S +4 -0

@@ -85,6 +140,10 @@ ENTRY(ftrace_regs_caller)
 	pushl	%edx
 	pushl	%ecx
 	pushl	%ebx
+#ifndef USING_FRAME_POINTER
+	/* Load 4 off of the parent ip addr into ebp */
+	lea	14*4(%esp), %ebp
+#endif  

Instead of:

	#ifndef USING_FRAME_POINTER

Shouldn't it be:

	#ifdef CC_USING_FENTRY

?

Otherwise, if I'm reading it right, with fentry and CONFIG_FRAME_POINTER=y, ebp will lead to the grandparent's ip instead of the parent's ip.

From: "Steven Rostedt (VMware)" <[email protected]>

x86_64 has had fentry support for some time. I did not add support to x86_32 as I was unsure if it will be used much in the future. It is still very much used, and there's issues with function graph tracing with gcc playing around with the mcount frames, causing function graph to panic. The fentry code does not have this issue, and is able to cope as there is no frame to mess up.

Note, this only add support for fentry when DYNAMIC_FTRACE is set. There's really no reason to not have that set, because the performance of the machine drops significantly when it's not enabled. I only keep !DYNAMIC_FTRACE around to test it off, as there's still some archs that have FTRACE but not DYNAMIC_FTRACE.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 arch/x86/Kconfig            |  2 +-
 arch/x86/kernel/ftrace_32.S | 88 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 79 insertions(+), 11 deletions(-)

arch/x86/Kconfig +1 -1

index cc98d5a294ee..8c17146427ca 100644
@@ -127,7 +127,7 @@ config X86
 	select HAVE_EBPF_JIT			if X86_64
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_EXIT_THREAD
-	select HAVE_FENTRY			if X86_64
+	select HAVE_FENTRY			if X86_64 || DYNAMIC_FTRACE
 	select HAVE_FTRACE_MCOUNT_RECORD
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_FUNCTION_TRACER

arch/x86/kernel/ftrace_32.S +15 -0

index 8ca33d9806ac..4bf8223555cd 100644
@@ -9,27 +9,75 @@
 #include <asm/export.h>
 #include <asm/ftrace.h>
 
+
+#ifdef CC_USING_FENTRY
+# define function_hook	__fentry__
+EXPORT_SYMBOL(__fentry__)
+#else
+# define function_hook	mcount
+EXPORT_SYMBOL(mcount)
+#endif
+
+/* mcount uses a frame pointer even if CONFIG_FRAME_POINTER is not set */
+#if !defined(CC_USING_FENTRY) || defined(CONFIG_FRAME_POINTER)
+# define USING_FRAME_POINTER
+#endif
+
+#ifdef USING_FRAME_POINTER

Quoted 10 lines

arch/x86/kernel/ftrace_32.S +10 -0

+/* mcount uses a frame pointer even if CONFIG_FRAME_POINTER is not set */
+#if !defined(CC_USING_FENTRY) || defined(CONFIG_FRAME_POINTER)
+# define USING_FRAME_POINTER
+#endif
+
+#ifdef USING_FRAME_POINTER
+# ifdef CC_USING_FENTRY
+#  define MCOUNT_FRAME_SIZE		(4*4)	/* bp,ip and parent's  */
+# else
+#  define MCOUNT_FRAME_SIZE		4	/* just the bp         */

arch/x86/kernel/ftrace_32.S line 36

Quoted 6 lines

arch/x86/kernel/ftrace_32.S +6 -0

+# endif
+# define MCOUNT_FRAME			1	/* using frame = true  */
+#else
+# define MCOUNT_FRAME_SIZE		0	/* no stack frame      */
+# define MCOUNT_FRAME			0	/* using frame = false */
+#endif  

It seems that there is no use of MCOUNT_FRAME_SIZE below. Do we really need it?