Re: [PATCH] arm64: SHA-224/SHA-256 using ARMv8 Crypto Extensions
On Thursday, March 20, 2014 at 03:48:06 PM, Ard Biesheuvel wrote:
> This patch adds support for the SHA-224 and SHA-256 hash algorithms using
> the NEON based SHA-256 instructions that were introduced in ARM v8.
>
> Signed-off-by: Ard Biesheuvel
> ---
[...]
> + * Copyright (c) Alan Smithee.
Email contact is missing here.
[...]
> +static int sha224_init(struct shash_desc *desc)
> +{
> + struct sha256_state *sctx = shash_desc_ctx(desc);
> +
> + *sctx = (struct sha256_state){
This cast is interesting, I don't quite understand it. Can you please explain
that to me ?
> + .state = {
> + SHA224_H0, SHA224_H1, SHA224_H2, SHA224_H3,
> + SHA224_H4, SHA224_H5, SHA224_H6, SHA224_H7,
> + }
> + };
> + return 0;
> +}
[...]
> +static int sha224_final(struct shash_desc *desc, u8 *out)
> +{
> + struct sha256_state *sctx = shash_desc_ctx(desc);
> + __be32 *dst = (__be32 *)out;
> + int i;
> +
> + sha2_final(desc);
> +
> + for (i = 0; i < SHA224_DIGEST_SIZE / sizeof(*dst); i++)
> + dst[i] = cpu_to_be32(sctx->state[i]);
Won't this cause unaligned access if *dst is not aligned to 32 bytes ?
Try the crypto tests with this patch to see if this explodes please.
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 7795550..b9b7144 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -191,7 +191,8 @@ static int __test_hash(struct crypto_ahash *tfm, struct
hash_testvec *template,
const char *algo = crypto_tfm_alg_driver_name(crypto_ahash_tfm(tfm));
unsigned int i, j, k, temp;
struct scatterlist sg[8];
- char result[64];
+ char _result[68];
+ char *result = _result + 1;
struct ahash_request *req;
struct tcrypt_result tresult;
void *hash_buff;
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] crypto: x86/sha1 - regression and other fixes
On Monday, March 24, 2014 at 05:10:36 PM, Mathias Krause wrote: > The recent addition of the AVX2 variant of the SHA1 hash function wrongly > disabled the AVX variant by introducing a flaw in the feature test. Fixed > in patch 1. > > The alignment calculations of the AVX2 assembler implementation are > questionable, too. Especially the page alignment of the stack pointer is > broken in multiple ways. Fixed in patch 2. In patch 3 another issue for > code alignment is fixed. > > Please apply! Nice, Reviewed-by: Marek Vasut Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] crypto: x86/sha1 - regression and other fixes
On 03/24/2014 09:10 AM, Mathias Krause wrote: > The recent addition of the AVX2 variant of the SHA1 hash function wrongly > disabled the AVX variant by introducing a flaw in the feature test. Fixed > in patch 1. > > The alignment calculations of the AVX2 assembler implementation are > questionable, too. Especially the page alignment of the stack pointer is > broken in multiple ways. Fixed in patch 2. In patch 3 another issue for > code alignment is fixed. > > Please apply! > > Mathias Krause (3): > crypto: x86/sha1 - re-enable the AVX variant > crypto: x86/sha1 - fix stack alignment of AVX2 variant > crypto: x86/sha1 - reduce size of the AVX2 asm implementation > > arch/x86/crypto/sha1_avx2_x86_64_asm.S |8 ++-- > arch/x86/crypto/sha1_ssse3_glue.c | 26 -- > 2 files changed, 18 insertions(+), 16 deletions(-) > For all these patches: Reviewed-by: H. Peter Anvin Thank you for doing these. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] crypto: x86/sha1 - regression and other fixes
On Mon, 2014-03-24 at 17:10 +0100, Mathias Krause wrote: > The recent addition of the AVX2 variant of the SHA1 hash function wrongly > disabled the AVX variant by introducing a flaw in the feature test. Fixed > in patch 1. > > The alignment calculations of the AVX2 assembler implementation are > questionable, too. Especially the page alignment of the stack pointer is > broken in multiple ways. Fixed in patch 2. In patch 3 another issue for > code alignment is fixed. > > Please apply! > > Mathias Krause (3): > crypto: x86/sha1 - re-enable the AVX variant > crypto: x86/sha1 - fix stack alignment of AVX2 variant > crypto: x86/sha1 - reduce size of the AVX2 asm implementation > > arch/x86/crypto/sha1_avx2_x86_64_asm.S |8 ++-- > arch/x86/crypto/sha1_ssse3_glue.c | 26 -- > 2 files changed, 18 insertions(+), 16 deletions(-) > Your fixes are the right on mark. I went through your patches and tested them and found to be correct. Sorry for causing regression and missing alignment issues in the patches I submitted. - mouli -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] crypto: x86/sha1 - regression and other fixes
On 03/24/2014 09:10 AM, Mathias Krause wrote: > The recent addition of the AVX2 variant of the SHA1 hash function wrongly > disabled the AVX variant by introducing a flaw in the feature test. Fixed > in patch 1. > > The alignment calculations of the AVX2 assembler implementation are > questionable, too. Especially the page alignment of the stack pointer is > broken in multiple ways. Fixed in patch 2. In patch 3 another issue for > code alignment is fixed. > > Please apply! > Yikes. Yes... the alignment calculation is confused in the extreme. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] crypto: x86/sha1 - regression and other fixes
The recent addition of the AVX2 variant of the SHA1 hash function wrongly disabled the AVX variant by introducing a flaw in the feature test. Fixed in patch 1. The alignment calculations of the AVX2 assembler implementation are questionable, too. Especially the page alignment of the stack pointer is broken in multiple ways. Fixed in patch 2. In patch 3 another issue for code alignment is fixed. Please apply! Mathias Krause (3): crypto: x86/sha1 - re-enable the AVX variant crypto: x86/sha1 - fix stack alignment of AVX2 variant crypto: x86/sha1 - reduce size of the AVX2 asm implementation arch/x86/crypto/sha1_avx2_x86_64_asm.S |8 ++-- arch/x86/crypto/sha1_ssse3_glue.c | 26 -- 2 files changed, 18 insertions(+), 16 deletions(-) -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] crypto: x86/sha1 - reduce size of the AVX2 asm implementation
There is really no need to page align sha1_transform_avx2. The default alignment is just fine. This is not the hot code but only the entry point, after all. Cc: Chandramouli Narayanan Cc: H. Peter Anvin Cc: Marek Vasut Signed-off-by: Mathias Krause --- arch/x86/crypto/sha1_avx2_x86_64_asm.S |1 - 1 file changed, 1 deletion(-) diff --git a/arch/x86/crypto/sha1_avx2_x86_64_asm.S b/arch/x86/crypto/sha1_avx2_x86_64_asm.S index bacac22b20..1cd792db15 100644 --- a/arch/x86/crypto/sha1_avx2_x86_64_asm.S +++ b/arch/x86/crypto/sha1_avx2_x86_64_asm.S @@ -623,7 +623,6 @@ _loop3: */ .macro SHA1_VECTOR_ASM name ENTRY(\name) - .align 4096 push%rbx push%rbp -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] crypto: x86/sha1 - fix stack alignment of AVX2 variant
The AVX2 implementation might waste up to a page of stack memory because of a wrong alignment calculation. This will, in the worst case, increase the stack usage of sha1_transform_avx2() alone to 5.4 kB -- way to big for a kernel function. Even worse, it might also allocate *less* bytes than needed if the stack pointer is already aligned bacause in that case the 'sub %rbx, %rsp' is effectively moving the stack pointer upwards, not downwards. Fix those issues by changing and simplifying the alignment calculation to use a 32 byte alignment, the alignment really needed. Cc: Chandramouli Narayanan Cc: H. Peter Anvin Cc: Marek Vasut Signed-off-by: Mathias Krause --- arch/x86/crypto/sha1_avx2_x86_64_asm.S |7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/arch/x86/crypto/sha1_avx2_x86_64_asm.S b/arch/x86/crypto/sha1_avx2_x86_64_asm.S index 4f348544d1..bacac22b20 100644 --- a/arch/x86/crypto/sha1_avx2_x86_64_asm.S +++ b/arch/x86/crypto/sha1_avx2_x86_64_asm.S @@ -636,9 +636,7 @@ _loop3: /* Align stack */ mov %rsp, %rbx - and $(0x1000-1), %rbx - sub $(8+32), %rbx - sub %rbx, %rsp + and $~(0x20-1), %rsp push%rbx sub $RESERVE_STACK, %rsp @@ -665,8 +663,7 @@ _loop3: avx2_zeroupper add $RESERVE_STACK, %rsp - pop %rbx - add %rbx, %rsp + pop %rsp pop %r15 pop %r14 -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] crypto: x86/sha1 - re-enable the AVX variant
Commit 7c1da8d0d0 "crypto: sha - SHA1 transform x86_64 AVX2"
accidentally disabled the AVX variant by making the avx_usable() test
not only fail in case the CPU doesn't support AVX or OSXSAVE but also
if it doesn't support AVX2.
Fix that regression by splitting up the AVX/AVX2 test into two
functions. Also test for the BMI1 extension in the avx2_usable() test
as the AVX2 implementation not only makes use of BMI2 but also BMI1
instructions.
Cc: Chandramouli Narayanan
Cc: H. Peter Anvin
Cc: Marek Vasut
Signed-off-by: Mathias Krause
---
arch/x86/crypto/sha1_ssse3_glue.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/arch/x86/crypto/sha1_ssse3_glue.c
b/arch/x86/crypto/sha1_ssse3_glue.c
index 139a55c04d..74d16ef707 100644
--- a/arch/x86/crypto/sha1_ssse3_glue.c
+++ b/arch/x86/crypto/sha1_ssse3_glue.c
@@ -208,11 +208,7 @@ static bool __init avx_usable(void)
{
u64 xcr0;
-#if defined(CONFIG_AS_AVX2)
- if (!cpu_has_avx || !cpu_has_avx2 || !cpu_has_osxsave)
-#else
if (!cpu_has_avx || !cpu_has_osxsave)
-#endif
return false;
xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
@@ -224,11 +220,23 @@ static bool __init avx_usable(void)
return true;
}
+
+#ifdef CONFIG_AS_AVX2
+static bool __init avx2_usable(void)
+{
+ if (avx_usable() && cpu_has_avx2 && boot_cpu_has(X86_FEATURE_BMI1) &&
+ boot_cpu_has(X86_FEATURE_BMI2))
+ return true;
+
+ return false;
+}
+#endif
#endif
static int __init sha1_ssse3_mod_init(void)
{
char *algo_name;
+
/* test for SSSE3 first */
if (cpu_has_ssse3) {
sha1_transform_asm = sha1_transform_ssse3;
@@ -238,13 +246,11 @@ static int __init sha1_ssse3_mod_init(void)
#ifdef CONFIG_AS_AVX
/* allow AVX to override SSSE3, it's a little faster */
if (avx_usable()) {
- if (cpu_has_avx) {
- sha1_transform_asm = sha1_transform_avx;
- algo_name = "AVX";
- }
+ sha1_transform_asm = sha1_transform_avx;
+ algo_name = "AVX";
#ifdef CONFIG_AS_AVX2
- if (cpu_has_avx2 && boot_cpu_has(X86_FEATURE_BMI2)) {
- /* allow AVX2 to override AVX, it's a little faster */
+ /* allow AVX2 to override AVX, it's a little faster */
+ if (avx2_usable()) {
sha1_transform_asm = sha1_apply_transform_avx2;
algo_name = "AVX2";
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] crypto: caam - power management support for caam job-ring
On 3/22/2014 6:24 PM, Ben Hutchings wrote: On Fri, 2014-03-21 at 00:35 +0545, Yashpal Dutta wrote: Job ring is suspended gracefully and resume afresh. Both Sleep (where device will remain powered-on) and Deep-sleep (where device will be powered-down are handled gracefully. Persistance sessions are not supported across deep-sleep. Cc: sta...@vger.kernel.org Signed-off-by: Yashpal Dutta --- drivers/crypto/caam/intern.h | 2 + drivers/crypto/caam/jr.c | 257 +++ 2 files changed, 190 insertions(+), 69 deletions(-) [...] This is too big for stable; is a simpler fix possible? Besides size, I'd also question whether this is a bug fix or a feature. From Documentation/stable_kernel_rules.txt: - It must fix a problem that causes a build error (but not for things marked CONFIG_BROKEN), an oops, a hang, data corruption, a real security issue, or some "oh, that's not good" issue. In short, something critical. On top of that - does it apply cleanly && has it been tested with -stable kernels? Regards, Horia -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drivers/crypto: Use RCU_INIT_POINTER(x, NULL) in nx/nx-842.c
On 24/03/14 12:16, Neil Horman wrote: On Mon, Mar 24, 2014 at 01:01:04AM +0530, Monam Agarwal wrote: This patch replaces rcu_assign_pointer(x, NULL) with RCU_INIT_POINTER(x, NULL) The rcu_assign_pointer() ensures that the initialization of a structure is carried out before storing a pointer to that structure. And in the case of the NULL pointer, there is no structure to initialize. So, rcu_assign_pointer(p, NULL) can be safely converted to RCU_INIT_POINTER(p, NULL) Signed-off-by: Monam Agarwal --- drivers/crypto/nx/nx-842.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/crypto/nx/nx-842.c b/drivers/crypto/nx/nx-842.c index 1e5481d..c4fcbf4 100644 --- a/drivers/crypto/nx/nx-842.c +++ b/drivers/crypto/nx/nx-842.c @@ -1234,7 +1234,7 @@ static int __exit nx842_remove(struct vio_dev *viodev) old_devdata = rcu_dereference_check(devdata, lockdep_is_held(&devdata_mutex)); of_reconfig_notifier_unregister(&nx842_of_nb); - rcu_assign_pointer(devdata, NULL); + RCU_INIT_POINTER(devdata, NULL); spin_unlock_irqrestore(&devdata_mutex, flags); synchronize_rcu(); dev_set_drvdata(&viodev->dev, NULL); @@ -1285,7 +1285,7 @@ static void __exit nx842_exit(void) spin_lock_irqsave(&devdata_mutex, flags); old_devdata = rcu_dereference_check(devdata, lockdep_is_held(&devdata_mutex)); - rcu_assign_pointer(devdata, NULL); + RCU_INIT_POINTER(devdata, NULL); spin_unlock_irqrestore(&devdata_mutex, flags); synchronize_rcu(); if (old_devdata) This really does't seem right. rcu_assign_pointer users ACCESS_ONCE to protect against multiple loads that allow for concurrent read/write use with parallel rcu_dereference calls, whereas RCU_INIT_POINTER does not. It also just doesn't look right. We've already initalized the pointerin nx842_init, we don't need to re-initalize it here, we need to assign it to NULL. I was being cautious as well, but Paul gave me an explanation [1] Hope it helps you as well ;-) But I think I also saw a patch that modifies rcu_assign_pointer to handle NULL assignment. Don't recall the thread. Regards, Arend [1] http://mid.gmane.org/20140320150601.gk4...@linux.vnet.ibm.com Neil -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drivers/crypto: Use RCU_INIT_POINTER(x, NULL) in nx/nx-842.c
On Mon, Mar 24, 2014 at 01:01:04AM +0530, Monam Agarwal wrote: > This patch replaces rcu_assign_pointer(x, NULL) with RCU_INIT_POINTER(x, NULL) > > The rcu_assign_pointer() ensures that the initialization of a structure > is carried out before storing a pointer to that structure. > And in the case of the NULL pointer, there is no structure to initialize. > So, rcu_assign_pointer(p, NULL) can be safely converted to > RCU_INIT_POINTER(p, NULL) > > Signed-off-by: Monam Agarwal > --- > drivers/crypto/nx/nx-842.c |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/crypto/nx/nx-842.c b/drivers/crypto/nx/nx-842.c > index 1e5481d..c4fcbf4 100644 > --- a/drivers/crypto/nx/nx-842.c > +++ b/drivers/crypto/nx/nx-842.c > @@ -1234,7 +1234,7 @@ static int __exit nx842_remove(struct vio_dev *viodev) > old_devdata = rcu_dereference_check(devdata, > lockdep_is_held(&devdata_mutex)); > of_reconfig_notifier_unregister(&nx842_of_nb); > - rcu_assign_pointer(devdata, NULL); > + RCU_INIT_POINTER(devdata, NULL); > spin_unlock_irqrestore(&devdata_mutex, flags); > synchronize_rcu(); > dev_set_drvdata(&viodev->dev, NULL); > @@ -1285,7 +1285,7 @@ static void __exit nx842_exit(void) > spin_lock_irqsave(&devdata_mutex, flags); > old_devdata = rcu_dereference_check(devdata, > lockdep_is_held(&devdata_mutex)); > - rcu_assign_pointer(devdata, NULL); > + RCU_INIT_POINTER(devdata, NULL); > spin_unlock_irqrestore(&devdata_mutex, flags); > synchronize_rcu(); > if (old_devdata) This really does't seem right. rcu_assign_pointer users ACCESS_ONCE to protect against multiple loads that allow for concurrent read/write use with parallel rcu_dereference calls, whereas RCU_INIT_POINTER does not. It also just doesn't look right. We've already initalized the pointerin nx842_init, we don't need to re-initalize it here, we need to assign it to NULL. Neil > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-crypto" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
