linux-kernel - Re: [RFC v2 PATCH 6/7] x86/entry: get rid of ALLOC_PT_GPREGS_ON_STACK and SAVE_AND_CLEAR

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] [day] [month] [year] [list]

Message-ID: <20180208183545.meosbkyibfe4evnp@treble>
Date:   Thu, 8 Feb 2018 12:35:45 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Dominik Brodowski <linux@...inikbrodowski.net>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        the arch/x86 maintainers <x86@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andi Kleen <ak@...ux.intel.com>,
        Andrew Lutomirski <luto@...nel.org>
Subject: Re: [RFC v2 PATCH 6/7] x86/entry: get rid of
 ALLOC_PT_GPREGS_ON_STACK and SAVE_AND_CLEAR_REGS

On Wed, Feb 07, 2018 at 10:29:15PM +0100, Dominik Brodowski wrote:
> On Wed, Feb 07, 2018 at 12:44:41PM -0800, Linus Torvalds wrote:
> > On Wed, Feb 7, 2018 at 12:15 PM, Dominik Brodowski
> > <linux@...inikbrodowski.net> wrote:
> > >
> > > Note: The testb $3, CS(%rsp) instruction in idtentry() does not need
> > > modification. Previously %rsp was manually decreased by 15*8; with
> > > this patch, %rsp is decreased by 15 pushq instructions. Moreover,
> > > error_entry did and does the exact same test (with offset=8) after
> > > the registers have been moved/pushed and cleared.
> > 
> > So this has the problem that now those save/clear instructions will
> > all be done in that idtentry macro.
> > 
> > So now that code will be duplicated for all the users of that macro.
> > 
> > The old code did the saving in the common error_entry and
> > paranoid_entry routines, in order to be able to share all the code,
> > and making the duplicated stub functions generated by the idtentry
> > macro smaller.
> > 
> > Now, admittedly the new push sequence is much smaller than the old
> > movq sequence, so the duplication doesn't hurt as much, but it's still
> > likely quite noticeable.
> > 
> > So this removes lines of asm code, but it adds a lot of instructions
> > to the end result thanks to the macro, I think.
> 
> Indeed, that is the case (see below). However, if we want to switch to
> PUSH instructions and do this in a routine which is call'ed and which
> ret'urns, %rsp needs to be moved around even more often than the old
> ALLOC_PT_GPREGS_ON_STACK macro did (which you wanted to get rid of,
> IIUYC). Or do I miss something?
> 
>    text	   data	    bss	    dec	    hex	filename
>   19500	      0	      0	  19500	   4c2c	arch/x86/entry/entry_64.o-orig
>   19510	      0	      0	  19510	   4c36	arch/x86/entry/entry_64.o-3_of_7
>   21105	      0	      0	  21105	   5271	arch/x86/entry/entry_64.o-5_of_7
>   24307	      0	      0	  24307	   5ef3	arch/x86/entry/entry_64.o-7_of_7
> 
> In any case, here's a v2.1 for this patch 6/7, which silences an objtool
> warning in error_entry.
> 
> Thanks,
> 	Dominik

I like it.  Some minor unwind hint improvements below, feel free to fold
them into this patch:

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index ae1b09a2e872..d7bd489fa360 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1135,7 +1135,6 @@ idtentry machine_check		do_mce			has_error_code=0	paranoid=1
  * Return: ebx=0: need swapgs on exit, ebx=1: otherwise
  */
 ENTRY(paranoid_entry)
-	UNWIND_HINT_FUNC
 	cld
 	movl	$1, %ebx
 	movl	$MSR_GS_BASE, %ecx
@@ -1149,7 +1148,7 @@ ENTRY(paranoid_entry)
 	SAVE_AND_SWITCH_TO_KERNEL_CR3 scratch_reg=%rax save_reg=%r14
 
 	ret
-END(paranoid_entry)
+ENDPROC(paranoid_entry)
 
 /*
  * "Paranoid" exit path from exception stack.  This is invoked
@@ -1184,7 +1183,6 @@ END(paranoid_exit)
  * Return: EBX=0: came from user mode; EBX=1: otherwise
  */
 ENTRY(error_entry)
-	UNWIND_HINT_FUNC
 	UNWIND_HINT_REGS offset=8
 	cld
 	testb	$3, CS+8(%rsp)