On Wed, 15 Mar 2017 19:11:24 -0400 Steven Rostedt <[email protected]> wrote:
> On Wed, 15 Mar 2017 16:13:43 -0500 > Josh Poimboeuf <[email protected]> wrote: > > > On Wed, Mar 15, 2017 at 03:55:31PM -0400, Steven Rostedt wrote: > > > 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(-) > > > > > > diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S > > > index 73d61a62649d..8ca33d9806ac 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, ®s->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, (%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. Since mcount/fentry is just "called", %eax (caller-saved register) will be ignored :) Of course for kprobes and livepatch, this should be critical. Except for the above point, it seems good to me. Reviewed-by: Masami Hiramatsu <[email protected]> Thanks, -- Masami Hiramatsu <[email protected]>
