Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1602685 > unrolled thread

[PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 18:30 +0100
    Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-16 18:50 +0100
      Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 19:00 +0100
      [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 19:10 +0100
        Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-16 19:30 +0100
          Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-16 20:30 +0100
            Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 20:50 +0100
          Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 20:30 +0100
            Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-16 20:40 +0100
              Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 21:00 +0100
          Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 20:30 +0100
          [PATCH 4/5 v3.1] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 21:20 +0100

#1602685 — [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller

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

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.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace_32.S | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 6f855dfb786d..2002aadd25f8 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -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
 
-	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, 5*4(%esp)			/* Load 0 into orig_ax */
+	movl	%eax, 8*4(%esp)			/* Load flags in return ip */
+
 	pushl	%ebp
 	pushl	%edi
 	pushl	%esi
@@ -79,11 +86,6 @@ ENTRY(ftrace_regs_caller)
 	pushl	%ecx
 	pushl	%ebx
 
-	movl	13*4(%esp), %eax		/* Get the saved flags */
-	movl	%eax, 14*4(%esp)		/* Move saved flags into regs->flags location */
-						/* clobbering return ip */
-	movl	$__KERNEL_CS, 13*4(%esp)
-
 	movl	12*4(%esp), %eax		/* Load ip (1st parameter) */
 	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
 	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
@@ -94,10 +96,14 @@ GLOBAL(ftrace_regs_call)
 	call	ftrace_stub
 
 	addl	$4, %esp			/* Skip pt_regs */
-	movl	14*4(%esp), %eax		/* Move flags back into cs */
-	movl	%eax, 13*4(%esp)		/* Needed to keep addl	from modifying flags */
-	movl	12*4(%esp), %eax		/* Get return ip from regs->ip */
-	movl	%eax, 14*4(%esp)		/* Put return ip back for ret */
+
+	/* Since we don't care about cs, move flags there to simplify return */
+	movl	14*4(%esp), %eax
+	movl	%eax, 13*4(%esp)
+
+	/* Move return ip back to its original location */
+	movl	12*4(%esp), %eax
+	movl	%eax, 14*4(%esp)
 
 	popl	%ebx
 	popl	%ecx
@@ -110,12 +116,11 @@ GLOBAL(ftrace_regs_call)
 	popl	%es
 	popl	%fs
 	popl	%gs
-	addl	$8, %esp			/* Skip orig_ax and ip */
-	popf					/* Pop flags at end (no addl to corrupt flags) */
-	jmp	.Lftrace_ret
+	addl	$8, %esp			/* Skip orig_ax and old ip */
 
-	popf
-	jmp	ftrace_stub
+	popf					/* flags is in the cs location */
+
+	jmp	.Lftrace_ret
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
 ENTRY(mcount)
-- 
2.10.2

[toc] | [next] | [standalone]

#1602708

On Thu, Mar 16, 2017 at 10:20 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> 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.

It still looks overly complicated to me.

The snippet below is the patch without the "-" lines, so it's the end result:

>  ENTRY(ftrace_regs_caller)
>         /*
>          * 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
> +        * 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
>
>         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, 5*4(%esp)                   /* Load 0 into orig_ax */
> +       movl    %eax, 8*4(%esp)                 /* Load flags in return ip */

Why do you do that silly "temporarily save flags" thing?

Why not just push $0 there?

Afaik, the sequence could/should be:

        pushl   $__KERNEL_CS
        pushl   4(%esp)         /* Save the return ip */
        pushl   $0
        pushl   %gs
        pushl   %fs
        pushl   %es
        pushl   %ds
        pushl   %eax

        /* Fix up eflags now that we have a scratch register */
        pushfl
        popl    %eax
        movl    %eax,8(%rsp)

Or something. There's no point in the unnecessary "shuffle values back
and forth with odd stack offsets".

           Linus

[toc] | [prev] | [next] | [standalone]

#1602720

[ Resending again with a "reply-all" instead of just "reply" ]

On Thu, 16 Mar 2017 10:40:24 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Mar 16, 2017 at 10:20 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > 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.
> 
> It still looks overly complicated to me.
> 
> The snippet below is the patch without the "-" lines, so it's the end result:
> 
> >  ENTRY(ftrace_regs_caller)
> >         /*
> >          * 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
> > +        * 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
> >
> >         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, 5*4(%esp)                   /* Load 0 into orig_ax */
> > +       movl    %eax, 8*4(%esp)                 /* Load flags in return ip */
> 
> Why do you do that silly "temporarily save flags" thing?
> 
> Why not just push $0 there?
> 
> Afaik, the sequence could/should be:
> 
>         pushl   $__KERNEL_CS
>         pushl   4(%esp)         /* Save the return ip */
>         pushl   $0
>         pushl   %gs
>         pushl   %fs
>         pushl   %es
>         pushl   %ds
>         pushl   %eax
> 
>         /* Fix up eflags now that we have a scratch register */
>         pushfl
>         popl    %eax
>         movl    %eax,8(%rsp)
> 
> Or something. There's no point in the unnecessary "shuffle values back
> and forth with odd stack offsets".


Sure, I can do this. This is the issue of trying to do too much at
first and then eliminating what you don't need. :-p

I think previous versions (where I was trying to horribly add a stack
frame here) had some more logic.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1602728 — [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller

[ I'll wait to post the full v3 series in case there is more comments ]

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

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.

Link: http://lkml.kernel.org/r/20170316135328.36123c3e@gandalf.local.home
[ simpler logic to save flags ]
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace_32.S | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 6f855df..691897e 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -55,23 +55,27 @@ 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 */
 	pushl	$0				/* Load 0 into orig_ax */
 	pushl	%gs
 	pushl	%fs
 	pushl	%es
 	pushl	%ds
 	pushl	%eax
+
+	/* Get flags and place them into the return ip slot */
+	pushf
+	popl	%eax
+	movl	%eax, 8*4(%esp)
+
 	pushl	%ebp
 	pushl	%edi
 	pushl	%esi
@@ -79,11 +83,6 @@ ENTRY(ftrace_regs_caller)
 	pushl	%ecx
 	pushl	%ebx
 
-	movl	13*4(%esp), %eax		/* Get the saved flags */
-	movl	%eax, 14*4(%esp)		/* Move saved flags into regs->flags location */
-						/* clobbering return ip */
-	movl	$__KERNEL_CS, 13*4(%esp)
-
 	movl	12*4(%esp), %eax		/* Load ip (1st parameter) */
 	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
 	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
@@ -94,10 +93,14 @@ GLOBAL(ftrace_regs_call)
 	call	ftrace_stub
 
 	addl	$4, %esp			/* Skip pt_regs */
-	movl	14*4(%esp), %eax		/* Move flags back into cs */
-	movl	%eax, 13*4(%esp)		/* Needed to keep addl	from modifying flags */
-	movl	12*4(%esp), %eax		/* Get return ip from regs->ip */
-	movl	%eax, 14*4(%esp)		/* Put return ip back for ret */
+
+	/* Since we don't care about cs, move flags there to simplify return */
+	movl	14*4(%esp), %eax
+	movl	%eax, 13*4(%esp)
+
+	/* Move return ip back to its original location */
+	movl	12*4(%esp), %eax
+	movl	%eax, 14*4(%esp)
 
 	popl	%ebx
 	popl	%ecx
@@ -110,12 +113,11 @@ GLOBAL(ftrace_regs_call)
 	popl	%es
 	popl	%fs
 	popl	%gs
-	addl	$8, %esp			/* Skip orig_ax and ip */
-	popf					/* Pop flags at end (no addl to corrupt flags) */
-	jmp	.Lftrace_ret
+	addl	$8, %esp			/* Skip orig_ax and old ip */
 
-	popf
-	jmp	ftrace_stub
+	popf					/* flags is in the cs location */
+
+	jmp	.Lftrace_ret
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
 ENTRY(mcount)
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1602747 — Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller

On Thu, Mar 16, 2017 at 11:09 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> +
> +       /* Since we don't care about cs, move flags there to simplify return */
> +       movl    14*4(%esp), %eax
> +       movl    %eax, 13*4(%esp)
> +
> +       /* Move return ip back to its original location */
> +       movl    12*4(%esp), %eax
> +       movl    %eax, 14*4(%esp)

Could this perhaps be removed entirely?

The return code could instead do:

        ... restore all the normal registers ..

        # Now restore flags that is under the return address and our
fake __KERNEL_CS
        pushl 8(%esp)
        popfl

        # and then return, skipping __KERNEL_CS and %flasg
        ret $8

which is smaller and simpler than (again) playing games with stack entries.

                Linus

[toc] | [prev] | [next] | [standalone]

#1602794 — Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller

On Thu, Mar 16, 2017 at 12:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> The thing is we don't return, we jump to the location that may be
> modified to run the function graph tracer.

Hmm.

How about just making the stack frame a tiny bit bigger, and getting
rid of *all* the games.

IOW, just duplicate the return address, and make the entry code do

        pushfl
        pushl   $__KERNEL_CS
        pushl   8(%esp)         /* Save the return ip *again* */
        pushl   $0
        pushl   %gs
        pushl   %fs
        pushl   %es
        pushl   %ds
        pushl   %eax
        ....

and not have any silly code to modify the old stack frame at all. Just
skip the values (all the segments, ORIG_EAX, duplicated return
address, __KERNEL_CS), and you can finish off with a "popf", and all
you have left i the original return ip that you didn't touch.

Hmm?

               Linus

[toc] | [prev] | [next] | [standalone]

#1602811 — Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller

On Thu, 16 Mar 2017 12:28:13 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Mar 16, 2017 at 12:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > The thing is we don't return, we jump to the location that may be
> > modified to run the function graph tracer.  
> 
> Hmm.
> 
> How about just making the stack frame a tiny bit bigger, and getting
> rid of *all* the games.

Unfortunately, x86_32 causes us to play these games. The
ftrace_regs_caller is used by kprobes to simulate an int3 breakpoint.
That means I need to have it behave exactly the same as int3. On
x86_32, the int3 does not save ESP or SS for that matter, but instead
kprobes, et al. uses the address of the regs->sp parameter to get the
stack location when the interrupt triggered (coming from kernel).

That means, I can't duplicate pt_regs any place else. The pt_regs passed
into the caller (kprobes) has to have &regs->esp be equal to the stack
address when the int3 was hit. Or in this case, when fentry was called.

> 
> IOW, just duplicate the return address, and make the entry code do
> 
>         pushfl
>         pushl   $__KERNEL_CS
>         pushl   8(%esp)         /* Save the return ip *again* */
>         pushl   $0
>         pushl   %gs
>         pushl   %fs
>         pushl   %es
>         pushl   %ds
>         pushl   %eax
>         ....
> 
> and not have any silly code to modify the old stack frame at all. Just
> skip the values (all the segments, ORIG_EAX, duplicated return
> address, __KERNEL_CS), and you can finish off with a "popf", and all
> you have left i the original return ip that you didn't touch.

Most likely those will not be touched, but as ftrace_regs_caller (which
is much heavier weight than ftrace_caller) must act the same as an
int3. If a kprobes callback modifies any of those, the
ftrace_regs_caller must act the same as if it was done by int3.

Now, most likely a kprobe's callback will never touch those. But we
never know.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1602796 — Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller

On Thu, 16 Mar 2017 15:19:32 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> 
> The thing is we don't return, we jump to the location that may be
> modified to run the function graph tracer.

That said, maybe the below is better?

	/* restore flags */
	pushl	14*4(%esp)
	popf

	/* Move return ip back to its original location */
	movl	12*4(%esp), %eax
	movl	%eax, 14*4(%esp)

	popl	%ebx
	popl	%ecx
	popl	%edx
	popl	%esi
	popl	%edi
	popl	%ebp
	popl	%eax
	popl	%ds
	popl	%es
	popl	%fs
	popl	%gs

	/* use lea to not affect flags */
	lea	(3*4)%esp, %esp			/* Skip orig_ax, ip and flags */

	jmp	.Lftrace_ret

-- Steve

[toc] | [prev] | [next] | [standalone]

#1602805 — Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller

On Thu, Mar 16, 2017 at 12:24 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>         popl    %ds
>         popl    %es
>         popl    %fs
>         popl    %gs

Do you even need these? Can they be changed by ftrace?

And see the previous email about not touching the original location at
all, adn just duplicating that entry, avoiding all the need for
shuffling. I *really* hope that ftrace doesn't depend on changing the
return %eip on the stack. That would be truly nasty.

                Linus

[toc] | [prev] | [next] | [standalone]

#1602814 — Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller

On Thu, 16 Mar 2017 12:30:25 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Mar 16, 2017 at 12:24 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >         popl    %ds
> >         popl    %es
> >         popl    %fs
> >         popl    %gs  
> 
> Do you even need these? Can they be changed by ftrace?

ftrace no, kprobes, maybe.

> 
> And see the previous email about not touching the original location at
> all, adn just duplicating that entry, avoiding all the need for
> shuffling. I *really* hope that ftrace doesn't depend on changing the
> return %eip on the stack. That would be truly nasty.

How do you think the live patching works?

-- Steve

[toc] | [prev] | [next] | [standalone]

#1602800 — Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller

On Thu, 16 Mar 2017 11:19:44 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Mar 16, 2017 at 11:09 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > +
> > +       /* Since we don't care about cs, move flags there to simplify return */
> > +       movl    14*4(%esp), %eax
> > +       movl    %eax, 13*4(%esp)
> > +
> > +       /* Move return ip back to its original location */
> > +       movl    12*4(%esp), %eax
> > +       movl    %eax, 14*4(%esp)  
> 
> Could this perhaps be removed entirely?
> 
> The return code could instead do:
> 
>         ... restore all the normal registers ..
> 
>         # Now restore flags that is under the return address and our
> fake __KERNEL_CS
>         pushl 8(%esp)
>         popfl
> 
>         # and then return, skipping __KERNEL_CS and %flasg
>         ret $8
> 
> which is smaller and simpler than (again) playing games with stack entries.
> 
>                 Linus


The thing is we don't return, we jump to the location that may be
modified to run the function graph tracer.

.Lftrace_ret:
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
.globl ftrace_graph_call
ftrace_graph_call:
	jmp	ftrace_stub  <- this can turn to a jump to function graph tracing
#endif

WEAK(ftrace_stub)
	ret
END(ftrace_caller)


[...]

	popf					/* flags is in the cs location */
	jmp	.Lftrace_ret


-- Steve

[toc] | [prev] | [next] | [standalone]

#1602822 — [PATCH 4/5 v3.1] ftrace/x86_32: Clean up ftrace_regs_caller

[ Unless we come up with something else, here's the latest ]

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

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.

Link: http://lkml.kernel.org/r/20170316135328.36123c3e@gandalf.local.home
[ simpler logic to save flags ]
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace_32.S | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 6f855df..068a582 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -55,23 +55,27 @@ 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.
+	 * regs->ip location, and move flags into the return ip location.
 	 */
-	pushl	4(%esp)				/* save return ip into ip slot */
-
+	pushl	$__KERNEL_CS
+	pushl	4(%esp)				/* Save the return ip */
 	pushl	$0				/* Load 0 into orig_ax */
 	pushl	%gs
 	pushl	%fs
 	pushl	%es
 	pushl	%ds
 	pushl	%eax
+
+	/* Get flags and place them into the return ip slot */
+	pushf
+	popl	%eax
+	movl	%eax, 8*4(%esp)
+
 	pushl	%ebp
 	pushl	%edi
 	pushl	%esi
@@ -79,11 +83,6 @@ ENTRY(ftrace_regs_caller)
 	pushl	%ecx
 	pushl	%ebx
 
-	movl	13*4(%esp), %eax		/* Get the saved flags */
-	movl	%eax, 14*4(%esp)		/* Move saved flags into regs->flags location */
-						/* clobbering return ip */
-	movl	$__KERNEL_CS, 13*4(%esp)
-
 	movl	12*4(%esp), %eax		/* Load ip (1st parameter) */
 	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
 	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
@@ -94,10 +93,14 @@ GLOBAL(ftrace_regs_call)
 	call	ftrace_stub
 
 	addl	$4, %esp			/* Skip pt_regs */
-	movl	14*4(%esp), %eax		/* Move flags back into cs */
-	movl	%eax, 13*4(%esp)		/* Needed to keep addl	from modifying flags */
-	movl	12*4(%esp), %eax		/* Get return ip from regs->ip */
-	movl	%eax, 14*4(%esp)		/* Put return ip back for ret */
+
+	/* restore flags */
+	push	14*4(%esp)
+	popf
+
+	/* Move return ip back to its original location */
+	movl	12*4(%esp), %eax
+	movl	%eax, 14*4(%esp)
 
 	popl	%ebx
 	popl	%ecx
@@ -110,12 +113,11 @@ GLOBAL(ftrace_regs_call)
 	popl	%es
 	popl	%fs
 	popl	%gs
-	addl	$8, %esp			/* Skip orig_ax and ip */
-	popf					/* Pop flags at end (no addl to corrupt flags) */
-	jmp	.Lftrace_ret
 
-	popf
-	jmp	ftrace_stub
+	/* use lea to not affect flags */
+	lea	3*4(%esp), %esp			/* Skip orig_ax, ip and flags */
+
+	jmp	.Lftrace_ret
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
 ENTRY(mcount)
-- 
2.9.3

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web