Re: [PATCH 3/6] x86: entry_64.S: rename save_paranoid to paranoid_entry, no code changes

[Andy Lutomirski]

On Mon, Feb 23, 2015 at 4:12 PM, Denys Vlasenko <dvlasenk@xxxxxxxxxx> wrote:
> This patch does a lot of cleanup in comments and formatting,
> but it does not change any code.
>
> Rename save_paranoid to paranoid_entry: this makes naming
> similar to its "non-paranoid" sibling, error_entry,
> and to its counterpart, paranoid_exit.
>
> Use the same CFI annotation atop paranoid_entry and error_entry.
>
> Fix irregular indentation of assembler operands.
>
> Add/fix comments on top of paranoid_entry and error_entry.
> Remove stale comment about "oldrax".
> Make comments about "no swapgs" flag in ebx more prominent.
> Deindent wrongly indented top-level comment atop paranoid_exit.
> Indent wrongly deindented comment inside error_entry.

Applied.

>
> Signed-off-by: Denys Vlasenko <dvlasenk@xxxxxxxxxx>
> CC: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> CC: Oleg Nesterov <oleg@xxxxxxxxxx>
> CC: Borislav Petkov <bp@xxxxxxxxx>
> CC: "H. Peter Anvin" <hpa@xxxxxxxxx>
> CC: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> CC: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> CC: X86 ML <x86@xxxxxxxxxx>
> CC: Alexei Starovoitov <ast@xxxxxxxxxxxx>
> CC: Will Drewry <wad@xxxxxxxxxxxx>
> CC: Kees Cook <keescook@xxxxxxxxxxxx>
> CC: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  arch/x86/kernel/entry_64.S | 68 ++++++++++++++++++++++++----------------------
>  1 file changed, 36 insertions(+), 32 deletions(-)
>
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index 71b549a..03498d0 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -981,10 +981,11 @@ ENTRY(\sym)
>         testl $3, CS(%rsp)              /* If coming from userspace, switch */
>         jnz 1f                          /* stacks. */
>         .endif
> -       call save_paranoid
> +       call paranoid_entry
>         .else
>         call error_entry
>         .endif
> +       /* returned flag: ebx=0: need swapgs on exit, ebx=1: don't need it */
>
>         DEFAULT_FRAME 0
>
> @@ -1015,10 +1016,11 @@ ENTRY(\sym)
>         addq $EXCEPTION_STKSZ, INIT_TSS_IST(\shift_ist)
>         .endif
>
> +       /* these procedures expect "no swapgs" flag in ebx */
>         .if \paranoid
> -       jmp paranoid_exit               /* %ebx: no swapgs flag */
> +       jmp paranoid_exit
>         .else
> -       jmp error_exit                  /* %ebx: no swapgs flag */
> +       jmp error_exit
>         .endif
>
>         .if \paranoid == 1
> @@ -1253,8 +1255,13 @@ idtentry async_page_fault do_async_page_fault has_error_code=1
>  idtentry machine_check has_error_code=0 paranoid=1 do_sym=*machine_check_vector(%rip)
>  #endif
>
> -ENTRY(save_paranoid)
> -       XCPT_FRAME 1 RDI+8
> +/*
> + * Save all registers in pt_regs, and switch gs if needed.
> + * Use slow, but surefire "are we in kernel?" check.
> + * Return: ebx=0: need swapgs on exit, ebx=1: otherwise
> + */
> +ENTRY(paranoid_entry)
> +       XCPT_FRAME 1 15*8
>         cld
>         SAVE_C_REGS 8
>         SAVE_EXTRA_REGS 8
> @@ -1267,20 +1274,19 @@ ENTRY(save_paranoid)
>         xorl %ebx,%ebx
>  1:     ret
>         CFI_ENDPROC
> -END(save_paranoid)
> -
> -       /*
> -        * "Paranoid" exit path from exception stack.  This is invoked
> -        * only on return from non-NMI IST interrupts that came
> -        * from kernel space.
> -        *
> -        * We may be returning to very strange contexts (e.g. very early
> -        * in syscall entry), so checking for preemption here would
> -        * be complicated.  Fortunately, we there's no good reason
> -        * to try to handle preemption here.
> -        */
> +END(paranoid_entry)
>
> -       /* ebx: no swapgs flag */
> +/*
> + * "Paranoid" exit path from exception stack.  This is invoked
> + * only on return from non-NMI IST interrupts that came
> + * from kernel space.
> + *
> + * We may be returning to very strange contexts (e.g. very early
> + * in syscall entry), so checking for preemption here would
> + * be complicated.  Fortunately, we there's no good reason
> + * to try to handle preemption here.
> + */
> +/* On entry, ebx is "no swapgs" flag (1: don't need swapgs, 0: need it) */
>  ENTRY(paranoid_exit)
>         DEFAULT_FRAME
>         DISABLE_INTERRUPTS(CLBR_NONE)
> @@ -1301,13 +1307,11 @@ paranoid_exit_restore:
>  END(paranoid_exit)
>
>  /*
> - * Exception entry point. This expects an error code/orig_rax on the stack.
> - * returns in "no swapgs flag" in %ebx.
> + * Save all registers in pt_regs, and switch gs if needed.
> + * Return: ebx=0: need swapgs on exit, ebx=1: otherwise
>   */
>  ENTRY(error_entry)
> -       XCPT_FRAME
> -       CFI_ADJUST_CFA_OFFSET 15*8
> -       /* oldrax contains error code */
> +       XCPT_FRAME 1 15*8
>         cld
>         SAVE_C_REGS 8
>         SAVE_EXTRA_REGS 8
> @@ -1320,12 +1324,12 @@ error_sti:
>         TRACE_IRQS_OFF
>         ret
>
> -/*
> - * There are two places in the kernel that can potentially fault with
> - * usergs. Handle them here.  B stepping K8s sometimes report a
> - * truncated RIP for IRET exceptions returning to compat mode. Check
> - * for these here too.
> - */
> +       /*
> +        * There are two places in the kernel that can potentially fault with
> +        * usergs. Handle them here.  B stepping K8s sometimes report a
> +        * truncated RIP for IRET exceptions returning to compat mode. Check
> +        * for these here too.
> +        */
>  error_kernelspace:
>         CFI_REL_OFFSET rcx, RCX+8
>         incl %ebx
> @@ -1355,7 +1359,7 @@ error_bad_iret:
>  END(error_entry)
>
>
> -/* ebx:        no swapgs flag (1: don't need swapgs, 0: need it) */
> +/* On entry, ebx is "no swapgs" flag (1: don't need swapgs, 0: need it) */
>  ENTRY(error_exit)
>         DEFAULT_FRAME
>         movl %ebx,%eax
> @@ -1581,13 +1585,13 @@ end_repeat_nmi:
>         ALLOC_PT_GPREGS_ON_STACK
>
>         /*
> -        * Use save_paranoid to handle SWAPGS, but no need to use paranoid_exit
> +        * Use paranoid_entry to handle SWAPGS, but no need to use paranoid_exit
>          * as we should not be calling schedule in NMI context.
>          * Even with normal interrupts enabled. An NMI should not be
>          * setting NEED_RESCHED or anything that normal interrupts and
>          * exceptions might do.
>          */
> -       call save_paranoid
> +       call paranoid_entry
>         DEFAULT_FRAME 0
>
>         /*
> --
> 1.8.1.4
>



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/