linux-kernel - Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Mar 2017 13:55:01 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...capital.net>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller


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

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

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