Re: [PATCH 7/7] DWARF: add the config option

[Josh Poimboeuf]

On Fri, May 26, 2017 at 01:29:01PM +0200, Jiri Slaby wrote:
> On 05/26/2017, 08:54 AM, Jiri Slaby wrote:
> > On 05/19/2017, 11:35 PM, Josh Poimboeuf wrote:
> >>   https://github.com/jpoimboe/linux/blob/undwarf/arch/x86/kernel/unwind_undwarf.c
> > 
> > JFYI, it crashes in sha1_transform_avx due to crypto changes. You
> > perhaps missed that this beast uses ebp (not rbp) register for
> > computations. I had to do:
> > 
> > --- a/arch/x86/crypto/sha1_ssse3_asm.S
> > +++ b/arch/x86/crypto/sha1_ssse3_asm.S
> > @@ -37,7 +37,7 @@
> >  #define REG_A  %ecx
> >  #define REG_B  %esi
> >  #define REG_C  %edi
> > -#define REG_D  %ebp
> > +#define REG_D  %r12d
> >  #define REG_E  %edx
> > 
> >  #define REG_T1 %eax
> > @@ -74,6 +74,7 @@
> >         SYM_FUNC_START(\name)
> > 
> >         push    %rbx
> > +       push    %r12
> >         push    %rbp
> > 
> >         mov     %rsp, %rbp
> > @@ -99,6 +100,7 @@
> >         rep stosq
> > 
> >         leaveq                          # deallocate workspace
> > +       pop     %r12
> >         pop     %rbx
> >         ret
> > 
> > 
> > I am afraid there are more of these, e.g. in aesni-intel_asm.S.
> 
> aesni-intel_asm.S is OK -- only untouched x86_32 part uses ebp.
> 
> But sha1_avx2_x86_64_asm.S is not. They use *all* usable registers
> including ebp in the computations hidden behind the
> SHA1_PIPELINED_MAIN_BODY macro. The only work around I can see is to
> push rbp/pop rbp around the computation as it used to do with rbx:
> 
> --- a/arch/x86/crypto/sha1_avx2_x86_64_asm.S
> +++ b/arch/x86/crypto/sha1_avx2_x86_64_asm.S
> @@ -636,6 +636,7 @@ _loop3:
>         /* Align stack */
>         mov     %rsp, %rbp
>         and     $~(0x20-1), %rsp
> +       push    %rbp
>         sub     $RESERVE_STACK, %rsp
> 
>         avx2_zeroupper
> @@ -661,6 +662,7 @@ _loop3:
>         avx2_zeroupper
> 
>         add     $RESERVE_STACK, %rsp
> +       pop     %rbp
> 
>         leaveq
>         pop     %r15

Thanks, the first fix looks good.  Is the second one needed though?  It
already pushes rbp before it aligns the stack.

DWARF/undwarf will be immune to these issues, so I'll be moving a lot of
these crypto changes to a separate branch.  They were only in this
branch because the new-and-improved objtool can now find rbp misusage in
leaf functions.

It seems that most of the crypto code is frame pointer ignorant.  IMO,
leaf functions shouldn't be allowed to use rbp because it breaks frame
pointers when preempted by an interrupt.  GCC seems to agree.

I added a check to objtool to find the ones which use rbp badly.  Here
are the ones I see with my config:

  arch/x86/crypto/des3_ede-asm_64.o: warning: objtool: des3_ede_x86_64_crypt_blk uses BP as a scratch register
  arch/x86/crypto/des3_ede-asm_64.o: warning: objtool: des3_ede_x86_64_crypt_blk_3way uses BP as a scratch register
  arch/x86/crypto/blowfish-x86_64-asm_64.o: warning: objtool: __blowfish_enc_blk_4way uses BP as a scratch register
  arch/x86/crypto/blowfish-x86_64-asm_64.o: warning: objtool: blowfish_dec_blk_4way uses BP as a scratch register
  arch/x86/crypto/twofish-x86_64-asm_64-3way.o: warning: objtool: __twofish_enc_blk_3way uses BP as a scratch register
  arch/x86/crypto/twofish-x86_64-asm_64-3way.o: warning: objtool: twofish_dec_blk_3way uses BP as a scratch register
  arch/x86/crypto/sha256-avx2-asm.o: warning: objtool: sha256_transform_rorx uses BP as a scratch register
  arch/x86/crypto/cast5-avx-x86_64-asm_64.o: warning: objtool: __cast5_enc_blk16 uses BP as a scratch register
  arch/x86/crypto/cast5-avx-x86_64-asm_64.o: warning: objtool: __cast5_dec_blk16 uses BP as a scratch register
  arch/x86/crypto/cast6-avx-x86_64-asm_64.o: warning: objtool: __cast6_enc_blk8 uses BP as a scratch register
  arch/x86/crypto/cast6-avx-x86_64-asm_64.o: warning: objtool: __cast6_dec_blk8 uses BP as a scratch register
  arch/x86/crypto/twofish-avx-x86_64-asm_64.o: warning: objtool: __twofish_enc_blk8 uses BP as a scratch register
  arch/x86/crypto/twofish-avx-x86_64-asm_64.o: warning: objtool: __twofish_dec_blk8 uses BP as a scratch register

(And that doesn't include the ones which misuse ebp.)

It may be a challenge to fix some of those which use all available
registers.

-- 
Josh