Re: SHA1-MB algorithm broken on latest kernel

2016-05-16 Thread Megha Dey 
On Mon, 2016-05-16 at 09:44 -0500, Josh Poimboeuf wrote:
> On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote:
> > On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote:
> > > * Herbert Xu  wrote:
> > > 
> > > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> > > > > Hi,
> > > > >  
> > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> > > > > observe a panic.
> > > > >  
> > > > > After having a quick look, on reverting the following patches, I am 
> > > > > able
> > > > > to complete the booting process.
> > > > > aec4d0e301f17bb143341c82cc44685b8af0b945
> > > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> > > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
> > > > >  
> > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems 
> > > > > wrong.
> > > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> > > > > from sha1_mb_mgr_flush_avx2.S.
> > > > >
> > > > > I do not think the functionality of the SHA1-MB crypto algorithm has
> > > > > been tested after applying these changes. (I am not sure if any of the
> > > > > other crypto algorithms have been affected by these changes).
> > > > 
> > > > Josh, Ingo:
> > > > 
> > > > Any ideas on this? Should we revert?
> > > 
> > > Yeah, I think so - although another option would be to standardize 
> > > sha1_x8_avx2() 
> > > - the problem is that it is a function that clobbers registers without 
> > > saving/restoring them, breaking the C function ABI. I realize it's 
> > > written in 
> > > assembly, but unless there are strong performance reasons to deviate from 
> > > the 
> > > regular calling convention it might make sense to fix that.
> > > 
> > > Do any warnings get generated after the revert, if you enable 
> > > CONFIG_STACK_VALIDATION=y?
> > 
> > After the revert and enabling CONFIG_STACK_VALIDATION:
> > arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool:
> > sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup
> > 
> > arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool:
> > sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup
> 
> Megha,
> 
> Sorry for breaking it.  I completely missed the fact that the function
> calls sha1_x8_avx2() which clobbers registers.
> 
> If the performance penalty isn't too bad, I'll submit a patch to
> standardize sha1_x8_avx2() to follow the C ABI.
> 
> Do you have any tips for testing this code?  I've tried using the tcrypt
> module, but no luck.
> 
Josh,
Build the kernel with the following configs:
CONFIG_CRYPTO_SHA1_MB=y
CONFIG_CRYPTO_TEST=m
CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n
There was a kernel panic while booting. 
So if after applying your new patch, we are able to get complete the
boot, then we are good.

Could you please send a copy of the patch, I could test it on my end
too.

Re: SHA1-MB algorithm broken on latest kernel

2016-05-16 Thread Megha Dey 
On Mon, 2016-05-16 at 09:44 -0500, Josh Poimboeuf wrote:
> On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote:
> > On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote:
> > > * Herbert Xu  wrote:
> > > 
> > > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> > > > > Hi,
> > > > >  
> > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> > > > > observe a panic.
> > > > >  
> > > > > After having a quick look, on reverting the following patches, I am 
> > > > > able
> > > > > to complete the booting process.
> > > > > aec4d0e301f17bb143341c82cc44685b8af0b945
> > > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> > > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
> > > > >  
> > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems 
> > > > > wrong.
> > > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> > > > > from sha1_mb_mgr_flush_avx2.S.
> > > > >
> > > > > I do not think the functionality of the SHA1-MB crypto algorithm has
> > > > > been tested after applying these changes. (I am not sure if any of the
> > > > > other crypto algorithms have been affected by these changes).
> > > > 
> > > > Josh, Ingo:
> > > > 
> > > > Any ideas on this? Should we revert?
> > > 
> > > Yeah, I think so - although another option would be to standardize 
> > > sha1_x8_avx2() 
> > > - the problem is that it is a function that clobbers registers without 
> > > saving/restoring them, breaking the C function ABI. I realize it's 
> > > written in 
> > > assembly, but unless there are strong performance reasons to deviate from 
> > > the 
> > > regular calling convention it might make sense to fix that.
> > > 
> > > Do any warnings get generated after the revert, if you enable 
> > > CONFIG_STACK_VALIDATION=y?
> > 
> > After the revert and enabling CONFIG_STACK_VALIDATION:
> > arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool:
> > sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup
> > 
> > arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool:
> > sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup
> 
> Megha,
> 
> Sorry for breaking it.  I completely missed the fact that the function
> calls sha1_x8_avx2() which clobbers registers.
> 
> If the performance penalty isn't too bad, I'll submit a patch to
> standardize sha1_x8_avx2() to follow the C ABI.
> 
> Do you have any tips for testing this code?  I've tried using the tcrypt
> module, but no luck.
> 
Josh,
Build the kernel with the following configs:
CONFIG_CRYPTO_SHA1_MB=y
CONFIG_CRYPTO_TEST=m
CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n
There was a kernel panic while booting. 
So if after applying your new patch, we are able to get complete the
boot, then we are good.

Could you please send a copy of the patch, I could test it on my end
too.

Re: SHA1-MB algorithm broken on latest kernel

2016-05-16 Thread Josh Poimboeuf 
On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote:
> On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote:
> > * Herbert Xu  wrote:
> > 
> > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> > > > Hi,
> > > >  
> > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> > > > observe a panic.
> > > >  
> > > > After having a quick look, on reverting the following patches, I am able
> > > > to complete the booting process.
> > > > aec4d0e301f17bb143341c82cc44685b8af0b945
> > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
> > > >  
> > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
> > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> > > > from sha1_mb_mgr_flush_avx2.S.
> > > >
> > > > I do not think the functionality of the SHA1-MB crypto algorithm has
> > > > been tested after applying these changes. (I am not sure if any of the
> > > > other crypto algorithms have been affected by these changes).
> > > 
> > > Josh, Ingo:
> > > 
> > > Any ideas on this? Should we revert?
> > 
> > Yeah, I think so - although another option would be to standardize 
> > sha1_x8_avx2() 
> > - the problem is that it is a function that clobbers registers without 
> > saving/restoring them, breaking the C function ABI. I realize it's written 
> > in 
> > assembly, but unless there are strong performance reasons to deviate from 
> > the 
> > regular calling convention it might make sense to fix that.
> > 
> > Do any warnings get generated after the revert, if you enable 
> > CONFIG_STACK_VALIDATION=y?
> 
> After the revert and enabling CONFIG_STACK_VALIDATION:
> arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool:
> sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup
> 
> arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool:
> sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup

Megha,

Sorry for breaking it.  I completely missed the fact that the function
calls sha1_x8_avx2() which clobbers registers.

If the performance penalty isn't too bad, I'll submit a patch to
standardize sha1_x8_avx2() to follow the C ABI.

Do you have any tips for testing this code?  I've tried using the tcrypt
module, but no luck.

-- 
Josh

Re: SHA1-MB algorithm broken on latest kernel

2016-05-16 Thread Josh Poimboeuf 
On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote:
> On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote:
> > * Herbert Xu  wrote:
> > 
> > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> > > > Hi,
> > > >  
> > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> > > > observe a panic.
> > > >  
> > > > After having a quick look, on reverting the following patches, I am able
> > > > to complete the booting process.
> > > > aec4d0e301f17bb143341c82cc44685b8af0b945
> > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
> > > >  
> > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
> > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> > > > from sha1_mb_mgr_flush_avx2.S.
> > > >
> > > > I do not think the functionality of the SHA1-MB crypto algorithm has
> > > > been tested after applying these changes. (I am not sure if any of the
> > > > other crypto algorithms have been affected by these changes).
> > > 
> > > Josh, Ingo:
> > > 
> > > Any ideas on this? Should we revert?
> > 
> > Yeah, I think so - although another option would be to standardize 
> > sha1_x8_avx2() 
> > - the problem is that it is a function that clobbers registers without 
> > saving/restoring them, breaking the C function ABI. I realize it's written 
> > in 
> > assembly, but unless there are strong performance reasons to deviate from 
> > the 
> > regular calling convention it might make sense to fix that.
> > 
> > Do any warnings get generated after the revert, if you enable 
> > CONFIG_STACK_VALIDATION=y?
> 
> After the revert and enabling CONFIG_STACK_VALIDATION:
> arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool:
> sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup
> 
> arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool:
> sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup

Megha,

Sorry for breaking it.  I completely missed the fact that the function
calls sha1_x8_avx2() which clobbers registers.

If the performance penalty isn't too bad, I'll submit a patch to
standardize sha1_x8_avx2() to follow the C ABI.

Do you have any tips for testing this code?  I've tried using the tcrypt
module, but no luck.

-- 
Josh

Re: SHA1-MB algorithm broken on latest kernel

2016-05-13 Thread Megha Dey 
On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote:
> * Herbert Xu  wrote:
> 
> > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> > > Hi,
> > >  
> > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> > > observe a panic.
> > >  
> > > After having a quick look, on reverting the following patches, I am able
> > > to complete the booting process.
> > > aec4d0e301f17bb143341c82cc44685b8af0b945
> > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
> > >  
> > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
> > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> > > from sha1_mb_mgr_flush_avx2.S.
> > >
> > > I do not think the functionality of the SHA1-MB crypto algorithm has
> > > been tested after applying these changes. (I am not sure if any of the
> > > other crypto algorithms have been affected by these changes).
> > 
> > Josh, Ingo:
> > 
> > Any ideas on this? Should we revert?
> 
> Yeah, I think so - although another option would be to standardize 
> sha1_x8_avx2() 
> - the problem is that it is a function that clobbers registers without 
> saving/restoring them, breaking the C function ABI. I realize it's written in 
> assembly, but unless there are strong performance reasons to deviate from the 
> regular calling convention it might make sense to fix that.
> 
> Do any warnings get generated after the revert, if you enable 
> CONFIG_STACK_VALIDATION=y?

After the revert and enabling CONFIG_STACK_VALIDATION:
arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool:
sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup

arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool:
sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup

> 
> Thanks,
> 
>   Ingo

Re: SHA1-MB algorithm broken on latest kernel

2016-05-13 Thread Megha Dey 
On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote:
> * Herbert Xu  wrote:
> 
> > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> > > Hi,
> > >  
> > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> > > observe a panic.
> > >  
> > > After having a quick look, on reverting the following patches, I am able
> > > to complete the booting process.
> > > aec4d0e301f17bb143341c82cc44685b8af0b945
> > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
> > >  
> > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
> > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> > > from sha1_mb_mgr_flush_avx2.S.
> > >
> > > I do not think the functionality of the SHA1-MB crypto algorithm has
> > > been tested after applying these changes. (I am not sure if any of the
> > > other crypto algorithms have been affected by these changes).
> > 
> > Josh, Ingo:
> > 
> > Any ideas on this? Should we revert?
> 
> Yeah, I think so - although another option would be to standardize 
> sha1_x8_avx2() 
> - the problem is that it is a function that clobbers registers without 
> saving/restoring them, breaking the C function ABI. I realize it's written in 
> assembly, but unless there are strong performance reasons to deviate from the 
> regular calling convention it might make sense to fix that.
> 
> Do any warnings get generated after the revert, if you enable 
> CONFIG_STACK_VALIDATION=y?

After the revert and enabling CONFIG_STACK_VALIDATION:
arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool:
sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup

arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool:
sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup

> 
> Thanks,
> 
>   Ingo

Re: SHA1-MB algorithm broken on latest kernel

2016-05-12 Thread Ingo Molnar 

* Herbert Xu  wrote:

> On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> > Hi,
> >  
> > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> > observe a panic.
> >  
> > After having a quick look, on reverting the following patches, I am able
> > to complete the booting process.
> > aec4d0e301f17bb143341c82cc44685b8af0b945
> > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
> >  
> > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
> > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> > from sha1_mb_mgr_flush_avx2.S.
> >
> > I do not think the functionality of the SHA1-MB crypto algorithm has
> > been tested after applying these changes. (I am not sure if any of the
> > other crypto algorithms have been affected by these changes).
> 
> Josh, Ingo:
> 
> Any ideas on this? Should we revert?

Yeah, I think so - although another option would be to standardize 
sha1_x8_avx2() 
- the problem is that it is a function that clobbers registers without 
saving/restoring them, breaking the C function ABI. I realize it's written in 
assembly, but unless there are strong performance reasons to deviate from the 
regular calling convention it might make sense to fix that.

Do any warnings get generated after the revert, if you enable 
CONFIG_STACK_VALIDATION=y?

Thanks,

Ingo

Re: SHA1-MB algorithm broken on latest kernel

2016-05-12 Thread Ingo Molnar 

* Herbert Xu  wrote:

> On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> > Hi,
> >  
> > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> > observe a panic.
> >  
> > After having a quick look, on reverting the following patches, I am able
> > to complete the booting process.
> > aec4d0e301f17bb143341c82cc44685b8af0b945
> > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
> >  
> > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
> > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> > from sha1_mb_mgr_flush_avx2.S.
> >
> > I do not think the functionality of the SHA1-MB crypto algorithm has
> > been tested after applying these changes. (I am not sure if any of the
> > other crypto algorithms have been affected by these changes).
> 
> Josh, Ingo:
> 
> Any ideas on this? Should we revert?

Yeah, I think so - although another option would be to standardize 
sha1_x8_avx2() 
- the problem is that it is a function that clobbers registers without 
saving/restoring them, breaking the C function ABI. I realize it's written in 
assembly, but unless there are strong performance reasons to deviate from the 
regular calling convention it might make sense to fix that.

Do any warnings get generated after the revert, if you enable 
CONFIG_STACK_VALIDATION=y?

Thanks,

Ingo

Re: SHA1-MB algorithm broken on latest kernel

2016-05-12 Thread Herbert Xu 
On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> Hi,
>  
> When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> observe a panic.
>  
> After having a quick look, on reverting the following patches, I am able
> to complete the booting process.
> aec4d0e301f17bb143341c82cc44685b8af0b945
> 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
>  
> Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
> The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> from sha1_mb_mgr_flush_avx2.S.
>  
> I do not think the functionality of the SHA1-MB crypto algorithm has
> been tested after applying these changes. (I am not sure if any of the
> other crypto algorithms have been affected by these changes).

Josh, Ingo:

Any ideas on this? Should we revert?

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: SHA1-MB algorithm broken on latest kernel

2016-05-12 Thread Herbert Xu 
On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote:
> Hi,
>  
> When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
> observe a panic.
>  
> After having a quick look, on reverting the following patches, I am able
> to complete the booting process.
> aec4d0e301f17bb143341c82cc44685b8af0b945
> 8691ccd764f9ecc69a6812dfe76214c86ac9ba06
> 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
>  
> Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
> The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
> from sha1_mb_mgr_flush_avx2.S.
>  
> I do not think the functionality of the SHA1-MB crypto algorithm has
> been tested after applying these changes. (I am not sure if any of the
> other crypto algorithms have been affected by these changes).

Josh, Ingo:

Any ideas on this? Should we revert?

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

SHA1-MB algorithm broken on latest kernel

2016-05-12 Thread Megha Dey 
Hi,
 
When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
observe a panic.
 
After having a quick look, on reverting the following patches, I am able
to complete the booting process.
aec4d0e301f17bb143341c82cc44685b8af0b945
8691ccd764f9ecc69a6812dfe76214c86ac9ba06
68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
 
Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
from sha1_mb_mgr_flush_avx2.S.
 
I do not think the functionality of the SHA1-MB crypto algorithm has
been tested after applying these changes. (I am not sure if any of the
other crypto algorithms have been affected by these changes).
 
Thanks,
Megha

SHA1-MB algorithm broken on latest kernel

2016-05-12 Thread Megha Dey 
Hi,
 
When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I
observe a panic.
 
After having a quick look, on reverting the following patches, I am able
to complete the booting process.
aec4d0e301f17bb143341c82cc44685b8af0b945
8691ccd764f9ecc69a6812dfe76214c86ac9ba06
68874ac3304ade7ed5ebb12af00d6b9bbbca0a16
 
Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong.
The r10 to r15 registers are used in sha1_x8_avx2.S, which is called
from sha1_mb_mgr_flush_avx2.S.
 
I do not think the functionality of the SHA1-MB crypto algorithm has
been tested after applying these changes. (I am not sure if any of the
other crypto algorithms have been affected by these changes).
 
Thanks,
Megha