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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
* Herbert Xu <[email protected]> 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
On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote:
> * Herbert Xu <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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.
On Mon, May 16, 2016 at 11:31:12AM -0700, Megha Dey wrote:
> 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 <[email protected]> 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.
Thanks. I was able to run the tests, though I didn't see a panic. Can
you test with this patch?
----
From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI
Megha Day reported a kernel panic in crypto code. The problem is that
sha1_x8_avx2() clobbers registers r12-r15 without saving and restoring
them.
Before commit aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in
sha-mb functions"), those registers were saved and restored by the
callers of the function. I removed them with that commit because I
didn't realize sha1_x8_avx2() clobbered them.
Fix the potential undefined behavior associated with clobbering the
registers and make the behavior less surprising by changing the
registers to be callee saved/restored to conform with the C function
call ABI.
Also, rdx (aka RSP_SAVE) doesn't need to be saved: I verified that none
of the callers rely on it being saved, and it's not a callee-saved
register in the C ABI.
Fixes: aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in sha-mb functions")
Cc: [email protected] # 4.6
Reported-by: Megha Dey <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/crypto/sha-mb/sha1_x8_avx2.S | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
index 8e1b477..c9dae1c 100644
--- a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
+++ b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
@@ -296,7 +296,11 @@ W14 = TMP_
#
ENTRY(sha1_x8_avx2)
- push RSP_SAVE
+ # save callee-saved clobbered registers to comply with C function ABI
+ push %r12
+ push %r13
+ push %r14
+ push %r15
#save rsp
mov %rsp, RSP_SAVE
@@ -446,7 +450,12 @@ lloop:
## Postamble
mov RSP_SAVE, %rsp
- pop RSP_SAVE
+
+ # restore callee-saved clobbered registers
+ pop %r15
+ pop %r14
+ pop %r13
+ pop %r12
ret
ENDPROC(sha1_x8_avx2)
--
2.4.11
On Mon, 2016-05-16 at 15:16 -0500, Josh Poimboeuf wrote:
> On Mon, May 16, 2016 at 11:31:12AM -0700, Megha Dey wrote:
> > 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 <[email protected]> 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.
>
> Thanks. I was able to run the tests, though I didn't see a panic. Can
> you test with this patch?
>
> ----
>
> From: Josh Poimboeuf <[email protected]>
> Subject: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI
>
> Megha Day reported a kernel panic in crypto code. The problem is that
> sha1_x8_avx2() clobbers registers r12-r15 without saving and restoring
> them.
>
> Before commit aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in
> sha-mb functions"), those registers were saved and restored by the
> callers of the function. I removed them with that commit because I
> didn't realize sha1_x8_avx2() clobbered them.
>
> Fix the potential undefined behavior associated with clobbering the
> registers and make the behavior less surprising by changing the
> registers to be callee saved/restored to conform with the C function
> call ABI.
>
> Also, rdx (aka RSP_SAVE) doesn't need to be saved: I verified that none
> of the callers rely on it being saved, and it's not a callee-saved
> register in the C ABI.
>
> Fixes: aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in sha-mb functions")
> Cc: [email protected] # 4.6
> Reported-by: Megha Dey <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> arch/x86/crypto/sha-mb/sha1_x8_avx2.S | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
> index 8e1b477..c9dae1c 100644
> --- a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
> +++ b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
> @@ -296,7 +296,11 @@ W14 = TMP_
> #
> ENTRY(sha1_x8_avx2)
>
> - push RSP_SAVE
> + # save callee-saved clobbered registers to comply with C function ABI
> + push %r12
> + push %r13
> + push %r14
> + push %r15
>
> #save rsp
> mov %rsp, RSP_SAVE
> @@ -446,7 +450,12 @@ lloop:
> ## Postamble
>
> mov RSP_SAVE, %rsp
> - pop RSP_SAVE
> +
> + # restore callee-saved clobbered registers
> + pop %r15
> + pop %r14
> + pop %r13
> + pop %r12
>
> ret
> ENDPROC(sha1_x8_avx2)
Hi Josh,
I don't see the panic and am able to boot. However, I am not able to see
the tests running. You said you were able to insert the tcrypt module
and test this right?
On Mon, May 16, 2016 at 02:39:06PM -0700, Megha Dey wrote:
> On Mon, 2016-05-16 at 15:16 -0500, Josh Poimboeuf wrote:
> > On Mon, May 16, 2016 at 11:31:12AM -0700, Megha Dey wrote:
> > > 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 <[email protected]> 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.
> >
> > Thanks. I was able to run the tests, though I didn't see a panic. Can
> > you test with this patch?
> >
> > ----
> >
> > From: Josh Poimboeuf <[email protected]>
> > Subject: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI
> >
> > Megha Day reported a kernel panic in crypto code. The problem is that
> > sha1_x8_avx2() clobbers registers r12-r15 without saving and restoring
> > them.
> >
> > Before commit aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in
> > sha-mb functions"), those registers were saved and restored by the
> > callers of the function. I removed them with that commit because I
> > didn't realize sha1_x8_avx2() clobbered them.
> >
> > Fix the potential undefined behavior associated with clobbering the
> > registers and make the behavior less surprising by changing the
> > registers to be callee saved/restored to conform with the C function
> > call ABI.
> >
> > Also, rdx (aka RSP_SAVE) doesn't need to be saved: I verified that none
> > of the callers rely on it being saved, and it's not a callee-saved
> > register in the C ABI.
> >
> > Fixes: aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in sha-mb functions")
> > Cc: [email protected] # 4.6
> > Reported-by: Megha Dey <[email protected]>
> > Signed-off-by: Josh Poimboeuf <[email protected]>
> > ---
> > arch/x86/crypto/sha-mb/sha1_x8_avx2.S | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
> > index 8e1b477..c9dae1c 100644
> > --- a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
> > +++ b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
> > @@ -296,7 +296,11 @@ W14 = TMP_
> > #
> > ENTRY(sha1_x8_avx2)
> >
> > - push RSP_SAVE
> > + # save callee-saved clobbered registers to comply with C function ABI
> > + push %r12
> > + push %r13
> > + push %r14
> > + push %r15
> >
> > #save rsp
> > mov %rsp, RSP_SAVE
> > @@ -446,7 +450,12 @@ lloop:
> > ## Postamble
> >
> > mov RSP_SAVE, %rsp
> > - pop RSP_SAVE
> > +
> > + # restore callee-saved clobbered registers
> > + pop %r15
> > + pop %r14
> > + pop %r13
> > + pop %r12
> >
> > ret
> > ENDPROC(sha1_x8_avx2)
>
> Hi Josh,
> I don't see the panic and am able to boot. However, I am not able to see
> the tests running. You said you were able to insert the tcrypt module
> and test this right?
I didn't insert tcrypt manually, but I set the config flags you
suggested, plus I added a few printks, and was at least able to verify
that this code ran during boot without any crypto test errors or other
warnings being reported.
--
Josh
On Mon, 2016-05-16 at 16:46 -0500, Josh Poimboeuf wrote:
> On Mon, May 16, 2016 at 02:39:06PM -0700, Megha Dey wrote:
> > On Mon, 2016-05-16 at 15:16 -0500, Josh Poimboeuf wrote:
> > > On Mon, May 16, 2016 at 11:31:12AM -0700, Megha Dey wrote:
> > > > 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 <[email protected]> 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.
> > >
> > > Thanks. I was able to run the tests, though I didn't see a panic. Can
> > > you test with this patch?
> > >
> > > ----
> > >
> > > From: Josh Poimboeuf <[email protected]>
> > > Subject: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI
> > >
> > > Megha Day reported a kernel panic in crypto code. The problem is that
> > > sha1_x8_avx2() clobbers registers r12-r15 without saving and restoring
> > > them.
> > >
> > > Before commit aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in
> > > sha-mb functions"), those registers were saved and restored by the
> > > callers of the function. I removed them with that commit because I
> > > didn't realize sha1_x8_avx2() clobbered them.
> > >
> > > Fix the potential undefined behavior associated with clobbering the
> > > registers and make the behavior less surprising by changing the
> > > registers to be callee saved/restored to conform with the C function
> > > call ABI.
> > >
> > > Also, rdx (aka RSP_SAVE) doesn't need to be saved: I verified that none
> > > of the callers rely on it being saved, and it's not a callee-saved
> > > register in the C ABI.
> > >
> > > Fixes: aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in sha-mb functions")
> > > Cc: [email protected] # 4.6
> > > Reported-by: Megha Dey <[email protected]>
> > > Signed-off-by: Josh Poimboeuf <[email protected]>
> > > ---
> > > arch/x86/crypto/sha-mb/sha1_x8_avx2.S | 13 +++++++++++--
> > > 1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
> > > index 8e1b477..c9dae1c 100644
> > > --- a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
> > > +++ b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S
> > > @@ -296,7 +296,11 @@ W14 = TMP_
> > > #
> > > ENTRY(sha1_x8_avx2)
> > >
> > > - push RSP_SAVE
> > > + # save callee-saved clobbered registers to comply with C function ABI
> > > + push %r12
> > > + push %r13
> > > + push %r14
> > > + push %r15
> > >
> > > #save rsp
> > > mov %rsp, RSP_SAVE
> > > @@ -446,7 +450,12 @@ lloop:
> > > ## Postamble
> > >
> > > mov RSP_SAVE, %rsp
> > > - pop RSP_SAVE
> > > +
> > > + # restore callee-saved clobbered registers
> > > + pop %r15
> > > + pop %r14
> > > + pop %r13
> > > + pop %r12
> > >
> > > ret
> > > ENDPROC(sha1_x8_avx2)
> >
> > Hi Josh,
> > I don't see the panic and am able to boot. However, I am not able to see
> > the tests running. You said you were able to insert the tcrypt module
> > and test this right?
>
> I didn't insert tcrypt manually, but I set the config flags you
> suggested, plus I added a few printks, and was at least able to verify
> that this code ran during boot without any crypto test errors or other
> warnings being reported.
>
ok. I tried at my end too using some printk's and nothing seems to be
broken. Also, if you intend to use the same commit message, I spell my
name as Megha Dey and not Day :)
On Mon, May 16, 2016 at 04:07:53PM -0700, Megha Dey wrote:
>
> ok. I tried at my end too using some printk's and nothing seems to be
> broken. Also, if you intend to use the same commit message, I spell my
> name as Megha Dey and not Day :)
Patch applied with the name corrected.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt