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

[Linus Torvalds]

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