[GIT PULL] ftrace/x86: Add frames pointers to trampoline as necessary

[GIT PULL] ftrace/x86: Add frames pointers to trampoline as necessary

[Steven Rostedt]

Linus,

While testing function triggers, I noticed that the stack trace trigger
for functions was missing the function that caused the trigger as
well as the parent function that called the triggered function. I use
this feature a lot and never noticed this before. Then I realized that
the difference was that I had CONFIG_FRAME_POINTERS enabled, which I
don't on the production machines I'm debugging.

When frame pointers are enabled, the stack trace code will ignore anything
that isn't found via the frame pointer. As the function trace trampoline
does not set up frame pointers, it is skipped. When fentry is used,
(gcc -pg --mfentry) it is called before the function stack frame is set
up, thus we also lose the parent pointer. These are quite useful for
debugging live kernels.

This goes back to 3.7 where the stacktrace trigger was introduced.
Before that, the function stack trace option had this issue, but since
it was before fentry was introduced, it only lost the traced function.
But that wasn't an issue as the option required the function itself
to be traced, and that showed the missing function.

This is cherry-picked from my 3.19 queue. After some thought, I figured
it should go into 3.18 and stable.

Please pull the latest trace-fixes-v3.18-rc5 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-fixes-v3.18-rc5

Tag SHA1: 7c58faf742b3dba437a84f40e994ac490cdad86a
Head SHA1: ec86b2aedbcb69ea4f8eae0d5e1af9321810b2f8


Steven Rostedt (Red Hat) (1):
      ftrace/x86: Add frames pointers to trampoline as necessary

----
 arch/x86/kernel/mcount_64.S | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
---------------------------
commit ec86b2aedbcb69ea4f8eae0d5e1af9321810b2f8
Author: Steven Rostedt (Red Hat) <rostedt@goodmis.org>
Date:   Tue Nov 18 19:13:25 2014 -0500

    ftrace/x86: Add frames pointers to trampoline as necessary
    
    When CONFIG_FRAME_POINTERS are enabled, it is required that the
    ftrace_caller and ftrace_regs_caller trampolines set up frame pointers
    otherwise a stack trace from a function call wont print the functions
    that called the trampoline. This is due to a check in
    __save_stack_address():
    
     #ifdef CONFIG_FRAME_POINTER
    	if (!reliable)
    		return;
     #endif
    
    The "reliable" variable is only set if the function address is equal to
    contents of the address before the address the frame pointer register
    points to. If the frame pointer is not set up for the ftrace caller
    then this will fail the reliable test. It will miss the function that
    called the trampoline. Worse yet, if fentry is used (gcc 4.6 and
    beyond), it will also miss the parent, as the fentry is called before
    the stack frame is set up. That means the bp frame pointer points
    to the stack of just before the parent function was called.
    
    Link: http://lkml.kernel.org/r/20141119034829.355440340@goodmis.org
    
    Cc: Ingo Molnar <mingo@redhat.com>
    Cc: "H. Peter Anvin" <hpa@zytor.com>
    Cc: x86@kernel.org
    Cc: stable@vger.kernel.org # 3.7+
    Acked-by: Thomas Gleixner <tglx@linutronix.de>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
index c73aecf10d34..d23132a9ef1c 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -45,14 +45,51 @@ END(function_hook)
 #endif
 .endm
 
+#ifdef CONFIG_FRAME_POINTER
+/*
+ * Stack traces will stop at the ftrace trampoline if the frame pointer
+ * is not set up properly. If fentry is used, we need to save a frame
+ * pointer for the parent as well as the function traced, because the
+ * fentry is called before the stack frame is set up, where as mcount
+ * is called afterward.
+ */
+.macro create_frame parent rip
+#ifdef CC_USING_FENTRY
+	pushq \parent
+	pushq %rbp
+	movq %rsp, %rbp
+#endif
+	pushq \rip
+	pushq %rbp
+	movq %rsp, %rbp
+.endm
+
+.macro restore_frame
+#ifdef CC_USING_FENTRY
+	addq $16, %rsp
+#endif
+	popq %rbp
+	addq $8, %rsp
+.endm
+#else
+.macro create_frame parent rip
+.endm
+.macro restore_frame
+.endm
+#endif /* CONFIG_FRAME_POINTER */
+
 ENTRY(ftrace_caller)
 	ftrace_caller_setup
 	/* regs go into 4th parameter (but make it NULL) */
 	movq $0, %rcx
 
+	create_frame %rsi, %rdi
+
 GLOBAL(ftrace_call)
 	call ftrace_stub
 
+	restore_frame
+
 	MCOUNT_RESTORE_FRAME
 ftrace_return:
 
@@ -96,9 +133,13 @@ ENTRY(ftrace_regs_caller)
 	/* regs go into 4th parameter */
 	leaq (%rsp), %rcx
 
+	create_frame %rsi, %rdi
+
 GLOBAL(ftrace_regs_call)
 	call ftrace_stub
 
+	restore_frame
+
 	/* Copy flags back to SS, to restore them */
 	movq EFLAGS(%rsp), %rax
 	movq %rax, SS(%rsp)

Re: [GIT PULL] ftrace/x86: Add frames pointers to trampoline as necessary

[Linus Torvalds]

On Fri, Nov 21, 2014 at 6:50 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> +.macro create_frame parent rip
> +#ifdef CC_USING_FENTRY
> +       pushq \parent
> +       pushq %rbp
> +       movq %rsp, %rbp

This is a very strange frame.

Why do you do the "pushq \parent" at all? Why isn't this just a *real*
frame and add it to MCOUNT_SAVE_FRAME.

You seem to create this fake frame-within-a-frame thing. It's not clear why.

                  Linus

Re: [GIT PULL] ftrace/x86: Add frames pointers to trampoline as necessary

[Steven Rostedt]

On Fri, 21 Nov 2014 17:35:10 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Nov 21, 2014 at 6:50 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > +.macro create_frame parent rip
> > +#ifdef CC_USING_FENTRY
> > +       pushq \parent
> > +       pushq %rbp
> > +       movq %rsp, %rbp
> 
> This is a very strange frame.
> 
> Why do you do the "pushq \parent" at all? Why isn't this just a *real*
> frame and add it to MCOUNT_SAVE_FRAME.
> 
> You seem to create this fake frame-within-a-frame thing. It's not clear why.
> 

Honestly, I did it this way because it was the simplest way to do it.

There's three different setups this covers:

1) frame pointers and mcount

2) frame pointers and fentry

3) no frame pointers and fentry

(mcount forces frame pointers, so there's no "no frame pointers and
mcount)

You probably know all this but I'm writing this for anyone else that
might be reading this. And also let you know what I understand, which
might be wrong too ;-)

With #3 we don't need to do anything. But when frame pointers
are enabled, we have in the main code:

1) mcount

func:
		push %rbp
		mov %rsp %rbp
		[...]
		call mcount

2) fentry

func:
		call fentry
		push %rbp
		mov %rsp %rbp


On mcount, the frame pointer of the parent is pushed to the stack, with
fentry it is not.

Now when fentry is called, the stack is:

	address of return-to-parent-addr
	address of return-to-func-addr

 If fentry does just:

fentry:
		push %rbp
		mov  %rsp %rbp

Then the stack trace will see after %rbp the return to func, but it
will never see the return to parent, and the parent will be missing
from the stack trace.

At a minimum, fentry needs:

fentry:
		push 8(%rsp)    // Return to parent, not func
		push %rbp
		mov  %rsp %rbp
		push 16(%rbp)	// Return to func:
		push %rbp
		mov  %rsp %rbp

With mcount, the call is done after the parent frame pointer is setup,
so we only really need to

mcount:
		push %rbp
		mov  %rsp %rbp

My version adds an extra push %rip in the mcount case but is the same
in the fentry case which is now the default.

The ftrace_caller_setup took a little bit to get right. I'm nervous
about modifying that and breaking something. Better yet, after that is
called, the parent rip is placed in %rsi and the func rip is placed in
%rdi. By doing it the way I did it, it made it nice and convenient that
I could just make a couple of macros that handle all three cases.

If you want, I can add more comments to explain all this.

-- Steve

Re: [GIT PULL] ftrace/x86: Add frames pointers to trampoline as necessary

[Linus Torvalds]

On Sat, Nov 22, 2014 at 5:05 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Honestly, I did it this way because it was the simplest way to do it.

So the thing is, I already dislike the whole setup, and your patch
just makes it worse.

And part of the problem is that there is *already* too many stupid
layers of indirection, and "setup" macros.

And yes, they really look pretty stupid.

There's MCOUNT_SAVE_FRAME, which is supposed to create a frame, but,
as you found out, doesn't actually do a good job of creating a real
frame, despite its name.

It's also in a really stupid place, excuse my french. Why is it in
that header file?

Then there's "ftrace_caller_setup", which uses MCOUNT_SAVE_FRAME, and
sets up %rsi/rdi to have the parent/rip stuff.

And that one does a particularly bad job *too*, since it's used in
only two of the four places that want this, with the other two doing
the whole thing by hand (look for things like

        movq RIP(%rsp), %rsi
        subq $MCOUNT_INSN_SIZE, %rsi

being duplicated..)

And it's also being particularly stupid about this all, because of the
horrible MCOUNT_SAVE_FRAME placement somewhere else. That
MCOUNT_SAVE_FRAME macro actually loads that %rip value into %rdx, but
because of the extra indirection and odd extra macro in a header file
(even though that one *.S file is the only apparent user),
"ftrace_caller_setup" doesn't actually realize that, and instead just
reloads it from where the previous macro just saved it. So the code it
generates just looks bad too.

And now your patch adds a *third* macro, to make up for the fact that
the first macro did a bad job.

You already have too many of these macros, and they are already ugly.
Why not just *fix* the existing macro, instead of adding yet another
layer of confusion and horror?

> If you want, I can add more comments to explain all this.

I'd really prefer the underlying problem to be fixed, not more
explanations for odd code choices.

Why can't MCOUNT_SAVE_FRAME just save the frame right? Maybe even do
it unconditionally - the cost of setting up a frame is not that huge,
and it might be better to have more understandable code and avoid yet
another ifdef for a config option.

And while at it, why not move it to where it's used, and maybe make it
not scream?

And maybe "ftrace_caller_setup" could be fixed so that the two cases
that now do that magic MCOUNT_INSN_SIZE stuff could use it?

Looking at that file, I just get the heebie-jeebies. It's like a
Halloween house of horrors of macros and #ifdefs. I'd really prefer
for a fix to not just be band-aid that makes it even worse.

                   Linus

Re: [GIT PULL] ftrace/x86: Add frames pointers to trampoline as necessary

[Steven Rostedt]

On Sun, 23 Nov 2014 14:51:57 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Nov 22, 2014 at 5:05 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Honestly, I did it this way because it was the simplest way to do it.
> 
> So the thing is, I already dislike the whole setup, and your patch
> just makes it worse.
> 
> And part of the problem is that there is *already* too many stupid
> layers of indirection, and "setup" macros.
> 
> And yes, they really look pretty stupid.
> 
> There's MCOUNT_SAVE_FRAME, which is supposed to create a frame, but,
> as you found out, doesn't actually do a good job of creating a real
> frame, despite its name.

Well, the MCOUNT_SAVE_FRAME is used to set up an "mcount frame" which
is different than a function call frame. Probably bad naming there, but
mcount implementation for compilers suffers from horrible
documentation, that is, lack of documentation.

This isn't an excuse, just an explanation of the history of that macro
name. It use to all be hand coded, and the macro came about in trying
to remove some duplicate code. I agree we should remove it from the
header file.

> 
> It's also in a really stupid place, excuse my french. Why is it in
> that header file?

Probably historical reasons. I could move it.

> 
> Then there's "ftrace_caller_setup", which uses MCOUNT_SAVE_FRAME, and
> sets up %rsi/rdi to have the parent/rip stuff.
> 
> And that one does a particularly bad job *too*, since it's used in
> only two of the four places that want this, with the other two doing
> the whole thing by hand (look for things like
> 
>         movq RIP(%rsp), %rsi
>         subq $MCOUNT_INSN_SIZE, %rsi
> 
> being duplicated..)

The other two places is for the static function tracing (where the call
to "mcount" is never modified into a nop, and suffers horrible
performance issues when tracing isn't enabled). And the function graph
tracer, which does something different with the parent pointer. It uses
the address of the parent as that is how it hijacks the return from the
function to trace the exit code. This is similar to what kprobes does
to trace function returns as well.


> 
> And it's also being particularly stupid about this all, because of the
> horrible MCOUNT_SAVE_FRAME placement somewhere else. That
> MCOUNT_SAVE_FRAME macro actually loads that %rip value into %rdx, but
> because of the extra indirection and odd extra macro in a header file
> (even though that one *.S file is the only apparent user),
> "ftrace_caller_setup" doesn't actually realize that, and instead just
> reloads it from where the previous macro just saved it. So the code it
> generates just looks bad too.
> 
> And now your patch adds a *third* macro, to make up for the fact that
> the first macro did a bad job.
> 
> You already have too many of these macros, and they are already ugly.
> Why not just *fix* the existing macro, instead of adding yet another
> layer of confusion and horror?
> 
> > If you want, I can add more comments to explain all this.
> 
> I'd really prefer the underlying problem to be fixed, not more
> explanations for odd code choices.
> 
> Why can't MCOUNT_SAVE_FRAME just save the frame right? Maybe even do
> it unconditionally - the cost of setting up a frame is not that huge,
> and it might be better to have more understandable code and avoid yet
> another ifdef for a config option.

If frame pointers are not enabled, I rather avoid setting them up, as
function tracing has such a huge overhead, pretty much every single op
is noticeable.

> 
> And while at it, why not move it to where it's used, and maybe make it
> not scream?
> 
> And maybe "ftrace_caller_setup" could be fixed so that the two cases
> that now do that magic MCOUNT_INSN_SIZE stuff could use it?
> 
> Looking at that file, I just get the heebie-jeebies. It's like a
> Halloween house of horrors of macros and #ifdefs. I'd really prefer
> for a fix to not just be band-aid that makes it even worse.

OK, I'll try to take some time tomorrow to see if I can clean up that
code. But anything I do will not be 3.18 or stable compatible. It would
require a redesign. This fix is the only thing I can think of for
pushing in for this late in the -rc release as well as to stable. But I
will definitely work on something to make things better.

I'm fine if you don't want to pull this change until I come up with a
better solution for my 3.19 queue. But I would not feel comfortable
with anything bigger to go into stable.

-- Steve