crypto: x86: Do not acquire fpu context for too long
Subsystems (2)
PATCH 1/1 crypto: x86: Do not acquire fpu context for too long
Many drivers such as camellia, cast5, etc are using ECB macro. ECB macro internally calls kernel_fpu_begin() then calls encrypt() and decrypt() and then it calls kernel_fpu_end().
If too many blocks are given, it acquires fpu context for too long.
kernel_fpu_{begin | end}() internally calls preempt_{disable | enable}(). So, RCU stall would occur because of too long FPU context period.
The purpose of this is to not exceed 4096 bytes to encrypt() and decrypt() in an FPU context.
--- arch/x86/crypto/ecb_cbc_helpers.h | 34 ++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)
arch/x86/crypto/ecb_cbc_helpers.h +27 -7
@@ -11,19 +11,35 @@* having to rely on indirect calls and retpolines. */ +#define ECB_CBC_WALK_MAX 4096 +#define ECB_WALK_START (req, bsize, fpu_blocks) do { \ void *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req)); \ + unsigned int walked_bytes = 0; \ const int __bsize = (bsize); \ struct skcipher_walk walk; \ - int err = skcipher_walk_virt(&walk, (req), false); \ + int err; \ + \ + err = skcipher_walk_virt(&walk, (req), false); \ while (walk.nbytes > 0) { \ - unsigned int nbytes = walk.nbytes; \ - bool do_fpu = (fpu_blocks) != -1 && \ - nbytes >= (fpu_blocks) * __bsize; \ const u8 *src = walk.src.virt.addr; \ - u8 *dst = walk.dst.virt.addr; \ u8 __maybe_unused buf[(bsize)]; \ - if (do_fpu) kernel_fpu_begin() + u8 *dst = walk.dst.virt.addr; \ + unsigned int nbytes; \ + bool do_fpu; \ + \ + if (walk.nbytes - walked_bytes > ECB_CBC_WALK_MAX) { \ + nbytes = ECB_CBC_WALK_MAX; \ + walked_bytes += ECB_CBC_WALK_MAX; \ + } else { \ + nbytes = walk.nbytes - walked_bytes; \ + walked_bytes = walk.nbytes; \ + } \ + \ + do_fpu = (fpu_blocks) != -1 && \ + nbytes >= (fpu_blocks) * __bsize; \ + if (do_fpu) \ + kernel_fpu_begin()#define CBC_WALK_START (req, bsize, fpu_blocks) \ ECB_WALK_START(req, bsize, fpu_blocks)
@@ -65,8 +81,12 @@ } while (0)#define ECB_WALK_END () \ - if (do_fpu) kernel_fpu_end(); \ + if (do_fpu) \ + kernel_fpu_end(); \ + if (walked_bytes < walk.nbytes) \ + continue; \ err = skcipher_walk_done(&walk, nbytes); \ + walked_bytes = 0; \ } \ return err; \ } while (0)
Signature · 1 line
2.17.1
Herbert Xu
maintainerQuoted 41 lines · Taehee Yoo
#define
ECB_WALK_START(req, bsize, fpu_blocks)do { \arch/x86/crypto/ecb_cbc_helpers.h +20
-6void *ctx =crypto_skcipher_ctx (crypto_skcipher_reqtfm (req)); \ +unsigned int walked_bytes =0 ; \const int __bsize = (bsize); \struct skcipher_walk walk; \ -int err =skcipher_walk_virt (&walk, (req), false); \ +int err; \ + \ + err =skcipher_walk_virt (&walk, (req), false); \while (walk.nbytes >0 ) { \ -unsigned int nbytes = walk.nbytes; \ -bool do_fpu = (fpu_blocks) !=-1 && \ - nbytes >= (fpu_blocks) * __bsize; \const u8 *src = walk.src.virt.addr; \ -u8 *dst = walk.dst.virt.addr; \ u8 __maybe_unused buf[(bsize)]; \ - if (do_fpu) kernel_fpu_begin() +u8 *dst = walk.dst.virt.addr; \ +unsigned int nbytes; \ +bool do_fpu; \ + \ +if (walk.nbytes - walked_bytes >ECB_CBC_WALK_MAX ) { \ + nbytes =ECB_CBC_WALK_MAX ; \ + walked_bytes +=ECB_CBC_WALK_MAX ; \ + }else { \ + nbytes = walk.nbytes - walked_bytes; \ + walked_bytes = walk.nbytes; \ + } \ + \ + do_fpu = (fpu_blocks) !=-1 && \ + nbytes >= (fpu_blocks) * __bsize; \ +if (do_fpu) \ + kernel_fpu_begin()#define CBC_WALK_START (req, bsize, fpu_blocks) \ ECB_WALK_START(req, bsize, fpu_blocks)@@ -65,8 +81,12 @@ }while (0 ) #define ECB_WALK_END() \
arch/x86/crypto/ecb_cbc_helpers.h line 92
Quoted 10 lines
arch/x86/crypto/ecb_cbc_helpers.h +5
-1-if (do_fpu)kernel_fpu_end (); \ +if (do_fpu) \ +kernel_fpu_end (); \ +if (walked_bytes < walk.nbytes) \ +continue ; \ err =skcipher_walk_done (&walk, nbytes); \ + walked_bytes =0 ; \ } \return err; \ }while (0 )
skcipher_walk_* is supposed to return at most a page. Why is this necessary?
Cheers,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi Herbert,
Thanks a lot for your review!
Quoted 56 lines · Herbert Xu
Quoted 41 lines · Taehee Yoo
#define ECB_WALK_START(req, bsize, fpu_blocks) do { \ void *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req)); \arch/x86/crypto/ecb_cbc_helpers.h +20
-6+unsigned int walked_bytes =0 ; \const int __bsize = (bsize); \struct skcipher_walk walk; \ -int err =skcipher_walk_virt (&walk, (req), false); \ +int err; \ + \ + err =skcipher_walk_virt (&walk, (req), false); \while (walk.nbytes >0 ) { \ -unsigned int nbytes = walk.nbytes; \ -bool do_fpu = (fpu_blocks) !=-1 && \ - nbytes >= (fpu_blocks) * __bsize; \const u8 *src = walk.src.virt.addr; \ -u8 *dst = walk.dst.virt.addr; \ u8 __maybe_unused buf[(bsize)]; \ - if (do_fpu) kernel_fpu_begin() +u8 *dst = walk.dst.virt.addr; \ +unsigned int nbytes; \ +bool do_fpu; \ + \ +if (walk.nbytes - walked_bytes >ECB_CBC_WALK_MAX ) { \ + nbytes =ECB_CBC_WALK_MAX ; \ + walked_bytes +=ECB_CBC_WALK_MAX ; \ + }else { \ + nbytes = walk.nbytes - walked_bytes; \ + walked_bytes = walk.nbytes; \ + } \ + \ + do_fpu = (fpu_blocks) !=-1 && \ + nbytes >= (fpu_blocks) * __bsize; \ +if (do_fpu) \ + kernel_fpu_begin()#define CBC_WALK_START (req, bsize, fpu_blocks) \ ECB_WALK_START(req, bsize, fpu_blocks)@@ -65,8 +81,12 @@ }while (0 ) #define ECB_WALK_END() \arch/x86/crypto/ecb_cbc_helpers.h line 92
Quoted 10 lines
arch/x86/crypto/ecb_cbc_helpers.h +5
-1-if (do_fpu)kernel_fpu_end (); \ +if (do_fpu) \ +kernel_fpu_end (); \ +if (walked_bytes < walk.nbytes) \ +continue ; \ err =skcipher_walk_done (&walk, nbytes); \ + walked_bytes =0 ; \ } \return err; \ }while (0 )skcipher_walk_* is supposed to return at most a page. Why is this necessary?
Quoted 1 line
Cheers,
I referred to below link.
https://lore.kernel.org/all/MW5PR84MB18426EBBA3303770A8BC0BDFAB759@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM/
Sorry for that I didn't check that skcipher_walk_* returns only under 4K sizes. So, I thought fpu context would be too long.
But, I just checked the skcipher_walk_*, and it's right, it returns under 4K sizes. So, there are no problems as you mentioned.
Thank you so much!
Taehee Yoo
Quoted 84 lines
-----Original Message-----
From: Taehee Yoo <[email protected]>
Sent: Tuesday, October 4, 2022 1:02 AM
To: Herbert Xu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] crypto: x86: Do not acquire fpu context for too longHi Herbert,
Thanks a lot for your review!Quoted 56 lines · Herbert Xu
Quoted 41 lines · Taehee Yoo
#define ECB_WALK_START(req, bsize, fpu_blocks) do { \ void *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req)); \arch/x86/crypto/ecb_cbc_helpers.h +20
-6+unsigned int walked_bytes =0 ; \const int __bsize = (bsize); \struct skcipher_walk walk; \ -int err =skcipher_walk_virt (&walk, (req), false); \ +int err; \ + \ + err =skcipher_walk_virt (&walk, (req), false); \while (walk.nbytes >0 ) { \ -unsigned int nbytes = walk.nbytes; \ -bool do_fpu = (fpu_blocks) !=-1 && \ - nbytes >= (fpu_blocks) * __bsize; \const u8 *src = walk.src.virt.addr; \ -u8 *dst = walk.dst.virt.addr; \ u8 __maybe_unused buf[(bsize)]; \ - if (do_fpu) kernel_fpu_begin() +u8 *dst = walk.dst.virt.addr; \ +unsigned int nbytes; \ +bool do_fpu; \ + \ +if (walk.nbytes - walked_bytes >ECB_CBC_WALK_MAX ) { \ + nbytes =ECB_CBC_WALK_MAX ; \ + walked_bytes +=ECB_CBC_WALK_MAX ; \ + }else { \ + nbytes = walk.nbytes - walked_bytes; \ + walked_bytes = walk.nbytes; \ + } \ + \ + do_fpu = (fpu_blocks) !=-1 && \ + nbytes >= (fpu_blocks) * __bsize; \ +if (do_fpu) \ + kernel_fpu_begin()#define CBC_WALK_START (req, bsize, fpu_blocks) \ ECB_WALK_START(req, bsize, fpu_blocks)@@ -65,8 +81,12 @@ }while (0 ) #define ECB_WALK_END() \arch/x86/crypto/ecb_cbc_helpers.h line 92
Quoted 10 lines
arch/x86/crypto/ecb_cbc_helpers.h +5
-1-if (do_fpu)kernel_fpu_end (); \ +if (do_fpu) \ +kernel_fpu_end (); \ +if (walked_bytes < walk.nbytes) \ +continue ; \ err =skcipher_walk_done (&walk, nbytes); \ + walked_bytes =0 ; \ } \return err; \ }while (0 )skcipher_walk_* is supposed to return at most a page. Why is this necessary?
Quoted 1 line
Cheers,
I referred to below link.
https://lore.kernel.org/all/MW5PR84MB18426EBBA3303770A8BC0BDFAB759@MW5PR84MB18
42.NAMPRD84.PROD.OUTLOOK.COM/Sorry for that I didn't check that skcipher_walk_* returns only under 4K sizes.
So, I thought fpu context would be too long.
But, I just checked the skcipher_walk_*, and it's right, it returns under 4K sizes.
So, there are no problems as you mentioned.
Quoted 2 lines
Thank you so much!
Taehee Yoo
I think functions using the ECB and CBC macros (and those helper functions) are safe - notice the third argument is called fpu_blocks. So, Aria's ECB mode is probably safe. There are no CTR macros, so that needs to be checked more carefully.
We need to check all the functions that don't use the macros and functions. The SHA functions (I've started working on a patch for those).
Running modprobe tcrypt mode=0, I encountered RCU stalls
on these:
tcrypt: testing encryption speed of sync skcipher cts(cbc(aes)) using cts(cbc(aes-aesni))
tcrypt: testing encryption speed of sync skcipher cfb(aes) using cfb(aes-aesni)
aesni-intel_glue.c registers "__cts(cbs(aes))", not "cts(cbc(aes-aesni)", and doesn't register any cfb algorithms, so those tests are using the generic templates, which must not be breaking up the loops as needed.
Quoted 98 lines · Robert (Servers)
Quoted 85 lines
-----Original Message-----
From: Taehee Yoo <[email protected]>
Sent: Tuesday, October 4, 2022 1:02 AM
To: Herbert Xu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] crypto: x86: Do not acquire fpu context for too longHi Herbert,
Thanks a lot for your review!Quoted 56 lines · Herbert Xu
Quoted 41 lines · Taehee Yoo
#define ECB_WALK_START(req, bsize, fpu_blocks) do { \ void *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req)); \ + unsigned int walked_bytes = 0; \ const int __bsize = (bsize); \ struct skcipher_walk walk; \- int err =
skcipher_walk_virt(&walk, (req), false); \
+ int err; \arch/x86/crypto/ecb_cbc_helpers.h +18
-5+ \ + err =skcipher_walk_virt (&walk, (req), false); \while (walk.nbytes >0 ) { \ -unsigned int nbytes = walk.nbytes; \ -bool do_fpu = (fpu_blocks) !=-1 && \ - nbytes >= (fpu_blocks) * __bsize; \const u8 *src = walk.src.virt.addr; \ -u8 *dst = walk.dst.virt.addr; \ u8 __maybe_unused buf[(bsize)]; \ - if (do_fpu) kernel_fpu_begin() +u8 *dst = walk.dst.virt.addr; \ +unsigned int nbytes; \ +bool do_fpu; \ + \ +if (walk.nbytes - walked_bytes >ECB_CBC_WALK_MAX ) { \ + nbytes =ECB_CBC_WALK_MAX ; \ + walked_bytes +=ECB_CBC_WALK_MAX ; \ + }else { \ + nbytes = walk.nbytes - walked_bytes; \ + walked_bytes = walk.nbytes; \ + } \ + \ + do_fpu = (fpu_blocks) !=-1 && \ + nbytes >= (fpu_blocks) * __bsize; \ +if (do_fpu) \ + kernel_fpu_begin()#define CBC_WALK_START (req, bsize, fpu_blocks) \ ECB_WALK_START(req, bsize, fpu_blocks)@@ -65,8 +81,12 @@ }while (0 ) #define ECB_WALK_END() \arch/x86/crypto/ecb_cbc_helpers.h line 92
Quoted 10 lines
arch/x86/crypto/ecb_cbc_helpers.h +5
-1-if (do_fpu)kernel_fpu_end (); \ +if (do_fpu) \ +kernel_fpu_end (); \ +if (walked_bytes < walk.nbytes) \ +continue ; \ err =skcipher_walk_done (&walk, nbytes); \ + walked_bytes =0 ; \ } \return err; \ }while (0 )skcipher_walk_* is supposed to return at most a page. Why is this necessary?
Quoted 1 line
Cheers,
I referred to below link.
https://lore.kernel.org/all/MW5PR84MB18426EBBA3303770A8BC0BDFAB759@MW5PR84MB18
42.NAMPRD84.PROD.OUTLOOK.COM/Sorry for that I didn't check that skcipher_walk_* returns only under 4K sizes.
So, I thought fpu context would be too long.
But, I just checked the skcipher_walk_*, and it's right, it returns under 4K sizes.
So, there are no problems as you mentioned.Quoted 2 lines
Thank you so much!
Taehee YooI think functions using the ECB and CBC macros (and those helper functions) are safe - notice the third argument is called
fpu_blocks. So, Aria's ECB mode is probably safe. There are no CTR macros, so that needs to be checked more carefully.We need to check all the functions that don't use the macros and functions. The SHA functions (I've started working on a patch for those).
Quoted 9 lines
Running modprobe tcrypt mode=0, I encountered RCU stalls
on these:
tcrypt: testing encryption speed of sync skcipher cts(cbc(aes)) using cts(cbc(aes-aesni))
tcrypt: testing encryption speed of sync skcipher cfb(aes) using cfb(aes-aesni)
aesni-intel_glue.cregisters "__cts(cbs(aes))", not "cts(cbc(aes-aesni)", and doesn't register any cfb algorithms, so those tests are using the generic templates, which must not be breaking up the loops as needed.
These all use the aes-aesni cipher wrapped in various layers of generic code. The core cipher puts kernel_fpu_begin/end around every AES block {16 bytes) it processes, so I doubt that the crypto code is at fault here, unless the issues is in tcrypt itself.
Quoted 138 lines
-----Original Message-----
From: Ard Biesheuvel <[email protected]>
Sent: Friday, October 7, 2022 4:55 PM
To: Elliott, Robert (Servers) <[email protected]>
Cc: Taehee Yoo <[email protected]>; Herbert Xu <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] crypto: x86: Do not acquire fpu context for too longQuoted 9 lines · Robert (Servers)
Quoted 9 lines
-----Original Message-----
From: Taehee Yoo <[email protected]>
Sent: Tuesday, October 4, 2022 1:02 AM
To: Herbert Xu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];Quoted 10 lines
Quoted 10 lines
[email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] crypto: x86: Do not acquire fpu context for too longHi Herbert,
Thanks a lot for your review!Quoted 3 lines · Herbert Xu
Quoted 2 lines · Taehee Yoo
#define ECB_WALK_START(req, bsize, fpu_blocks) do {\
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
void *ctx =
crypto_skcipher_ctx(crypto_skcipher_reqtfm(req)); \
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ unsigned int walked_bytes = 0; \ const int __bsize = (bsize);\
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
struct skcipher_walk walk;\
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
- int err = skcipher_walk_virt(&walk, (req), false); \ + int err; \ + \ + err = skcipher_walk_virt(&walk, (req), false); \ while (walk.nbytes > 0) {\
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/crypto/ecb_cbc_helpers.h
Quoted 4 lines
arch/x86/crypto/ecb_cbc_helpers.h +0
-3-unsigned int nbytes = walk.nbytes; \ -bool do_fpu = (fpu_blocks) !=-1 && \ - nbytes >= (fpu_blocks) * __bsize; \const u8 *src = walk.src.virt.addr;\
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
- u8 *dst = walk.dst.virt.addr; \ u8 __maybe_unused buf[(bsize)];\
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
arch/x86/crypto/ecb_cbc_helpers.h +16
-1-if (do_fpu)kernel_fpu_begin () +u8 *dst = walk.dst.virt.addr; \ +unsigned int nbytes; \ +bool do_fpu; \ + \ +if (walk.nbytes - walked_bytes >ECB_CBC_WALK_MAX ) { \ + nbytes =ECB_CBC_WALK_MAX ; \ + walked_bytes +=ECB_CBC_WALK_MAX ; \ + }else { \ + nbytes = walk.nbytes - walked_bytes; \ + walked_bytes = walk.nbytes; \ + } \ + \ + do_fpu = (fpu_blocks) !=-1 && \ + nbytes >= (fpu_blocks) * __bsize; \ +if (do_fpu) \ +kernel_fpu_begin ()Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
#define CBC_WALK_START(req, bsize, fpu_blocks)\
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
arch/x86/crypto/ecb_cbc_helpers.h line 83
Quoted 5 lines
ECB_WALK_START(req, bsize, fpu_blocks)arch/x86/crypto/ecb_cbc_helpers.h +0
-0@@ -65,8 +81,12 @@ }while (0 ) #define ECB_WALK_END()\
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
arch/x86/crypto/ecb_cbc_helpers.h
Quoted 6 lines
arch/x86/crypto/ecb_cbc_helpers.h +4
-1-if (do_fpu)kernel_fpu_end (); \ +if (do_fpu) \ +kernel_fpu_end (); \ +if (walked_bytes < walk.nbytes) \ +continue ; \ err =skcipher_walk_done (&walk, nbytes);\
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Review line 90
Quoted 2 lines
(quoted diff) +1
-0+ walked_bytes = 0; \ }\
Quoted 1 line
Quoted 1 line
Quoted 1 line
Review line 91
Quoted 1 line
(quoted diff) +0
-0return err;\
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Review line 92
Quoted 1 line
(quoted diff) +0
-0} while (0)skcipher_walk_* is supposed to return at most a page. Why is this necessary?
Cheers,
I referred to below link.
https://lore.kernel.org/all/MW5PR84MB18426EBBA3303770A8BC0BDFAB759@MW5PR84MB18
Quoted 18 lines
Quoted 11 lines
42.NAMPRD84.PROD.OUTLOOK.COM/
Sorry for that I didn't check that skcipher_walk_* returns only under 4K sizes.
So, I thought fpu context would be too long.
But, I just checked the skcipher_walk_*, and it's right, it returns under 4K sizes.
So, there are no problems as you mentioned.Thank you so much!
Taehee YooI think functions using the ECB and CBC macros (and those helper functions) are safe - notice the third argument is called
fpu_blocks. So, Aria's ECB mode is probably safe. There are no CTR macros, so that needs to be checked more carefully.Quoted 7 lines
We need to check all the functions that don't use the macros and functions. The SHA functions (I've started working on a patch for those).
Running modprobe tcrypt mode=0, I encountered RCU stalls
on these:
tcrypt: testing encryption speed of sync skcipher cts(cbc(aes))using cts(cbc(aes-aesni))
Quoted 1 line
tcrypt: testing encryption speed of sync skcipher cfb(aes) using
cfb(aes-aesni)
Quoted 5 lines
aesni-intel_glue.cregisters "__cts(cbs(aes))", not "cts(cbc(aes-aesni)", and doesn't register any cfb algorithms, so those tests are using the generic templates, which must not be breaking up the loops as needed.
Quoted 4 lines
These all use the aes-aesni cipher wrapped in various layers of generic code. The core cipher puts kernel_fpu_begin/end around every AES block {16 bytes) it processes, so I doubt that the crypto code is at fault here, unless the issues is in tcrypt itself.
In 2018, commit 2af632996b89 ("crypto: tcrypt - reschedule during speed tests") added cond_resched() calls to "Avoid RCU stalls in the case of non-preemptible kernel and lengthy speed tests by rescheduling when advancing from one block size to another."
It only makes those calls if the sec module parameter is used (run the speed test for a certain number of seconds), not the default "cycles" mode.
if (secs) {
ret = test_mb_acipher_jiffies(data, enc, bs, secs, num_mb);
cond_resched();
} else {
ret = test_mb_acipher_cycles(data, enc, bs, num_mb);
}In the original sighting, all three occurred during mode 200.
The cycle counts range from 151 to 221840 clock cycles. The stalls do not correlate to the longer cycle counts.
The first 8 tests were fine:
Aug 11 18:55:06 adevxp033-sys unknown: Running modprobe tcrypt mode=200
tcrypt: testing encryption speed of sync skcipher ecb(aes) using ecb(aes-aesni)
tcrypt: testing decryption speed of sync skcipher ecb(aes) using ecb(aes-aesni)
tcrypt: testing encryption speed of sync skcipher cbc(aes) using cbc(aes-aesni)
tcrypt: testing decryption speed of sync skcipher cbc(aes) using cbc(aes-aesni)
tcrypt: testing encryption speed of sync skcipher lrw(aes) using lrw(ecb(aes-aesni))
tcrypt: testing decryption speed of sync skcipher lrw(aes) using lrw(ecb(aes-aesni))
tcrypt: testing encryption speed of sync skcipher xts(aes) using xts(ecb(aes-aesni))
tcrypt: testing decryption speed of sync skcipher xts(aes) using xts(ecb(aes-aesni))
but the 9th triggered the first stall warning:
tcrypt: testing encryption speed of sync skcipher cts(cbc(aes)) using cts(cbc(aes-aesni))
tcrypt: test 0 (128 bit key, 16 byte blocks): 1 operation in 243 cycles (16 bytes)
tcrypt: test 1 (128 bit key, 64 byte blocks): 1 operation in 728 cycles (64 bytes)
tcrypt: test 2 (128 bit key, 128 byte blocks): 1 operation in 939 cycles (128 bytes)
tcrypt: test 3 (128 bit key, 256 byte blocks): 1 operation in 1341 cycles (256 bytes)
tcrypt: test 4 (128 bit key, 1024 byte blocks): 1 operation in 3786 cycles (1024 bytes)
tcrypt: test 5 (128 bit key, 1424 byte blocks): 1 operation in 5064 cycles (1424 bytes)
tcrypt: test 6 (128 bit key, 4096 byte blocks): 1 operation in 13889 cycles (4096 bytes)
tcrypt: test 7 (192 bit key, 16 byte blocks): 1 operation in 236 cycles (16 bytes)
rcu: INFO: rcu_preempt detected expedited stalls on CPUs/tasks: {1 operation in 745 cycles (64 bytes)
The stack trace prints interspersed with the next 26 test prints,
as it moved into:
tcrypt: testing decryption speed of sync skcipher cts(cbc(aes)) using cts(cbc(aes-aesni))
It proceeded through the next two with no problem:
tcrypt: testing encryption speed of sync skcipher ctr(aes) using ctr(aes-aesni)
tcrypt: testing decryption speed of sync skcipher ctr(aes) using ctr(aes-aesni)
and then reported two stalls during cfb encryption and cfb decryption:
tcrypt: testing encryption speed of sync skcipher cfb(aes) using cfb(aes-aesni)
tcrypt: test 0 (128 bit key, 16 byte blocks): 1 operation in 182 cycles (16 bytes)
tcrypt: test 1 (128 bit key, 64 byte blocks): 1 operation in 347 cycles (64 bytes)
tcrypt: test 2 (128 bit key, 128 byte blocks): 1 operation in 542 cycles (128 bytes)
tcrypt: test 3 (128 bit key, 256 byte blocks): 1 operation in 963 cycles (256 bytes)
rcu: INFO: rcu_preempt detected expedited stalls on CPUs/tasks: {1 operation in 3465 cycles (1024 bytes)
tcrypt: test 5 (128 bit key, 1420 byte blocks): 1 operation in 4808 cycles (1420 bytes)
tcrypt: test 6 (128 bit key, 4096 byte blocks): 1 operation in 13631 cycles (4096 bytes)
tcrypt: test 7 (192 bit key, 16 byte blocks): 1 operation in 190 cycles (16 bytes)
tcrypt: test 8 (192 bit key, 64 byte blocks): 1 operation in 360 cycles (64 bytes)
tcrypt: test 9 (192 bit key, 128 byte blocks): 1 operation in 570 cycles (128 bytes)
tcrypt: test 10 (192 bit key, 256 byte blocks): 1 operation in 1010 cycles (256 bytes)
tcrypt: test 11 (192 bit key, 1024 byte blocks): 1 operation in 3660 cycles (1024 bytes)
tcrypt: test 12 (192 bit key, 1420 byte blocks): 1 operation in 5088 cycles (1420 bytes)
tcrypt: test 13 (192 bit key, 4096 byte blocks): 1 operation in 14781 cycles (4096 bytes)
tcrypt: test 14 (256 bit key, 16 byte blocks): 1 operation in 195 cycles (16 bytes)
tcrypt: test 15 (256 bit key, 64 byte blocks): 1 operation in 363 cycles (64 bytes)
tcrypt: test 16 (256 bit key, 128 byte blocks): 1 operation in 592 cycles (128 bytes) 1 operation in 1046 cycles (256 bytes)
tcrypt: test 18 (256 bit key, 1024 byte blocks): 1 operation in 3796 cycles (1024 bytes)
tcrypt: test 19 (256 bit key, 1420 byte blocks): 1 operation in 5283 cycles (1420 bytes)
tcrypt: test 20 (256 bit key, 4096 byte blocks): 1 operation in 14925 cycles (4096 bytes)
tcrypt: testing decryption speed of sync skcipher cfb(aes) using cfb(aes-aesni) 1 operation in 184 cycles (16 bytes)
1 operation in 415 cycles (64 bytes)
tcrypt: test 2 (128 bit key, 128 byte blocks): 1 operation in 661 cycles (128 bytes)
rcu: INFO: rcu_preempt detected expedited stalls on CPUs/tasks: {1 operation in 1215 cycles (256 bytes)
None of the other modes (from 0 to 999) had problems.
mode=200 ran from 18:55:06 to 18:55:09, one of the longer durations, but not unique (e.g., mode 300 took 4 sec, mode 400 took 5 sec).
Perhaps the cycles mode needs to call cond_resched() too?
Herbert Xu
maintainerQuoted 2 lines · Robert (Servers)
Perhaps the cycles mode needs to call
cond_resched()too?
Yes, just make the cond_resched unconditional. Having a few too many rescheds shouldn't be an issue.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Quoted 12 lines
-----Original Message-----
From: Herbert Xu <[email protected]>
Sent: Sunday, October 9, 2022 1:20 AM
To: Elliott, Robert (Servers) <[email protected]>
Cc: Ard Biesheuvel <[email protected]>; Taehee Yoo <[email protected]>; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] crypto: x86: Do not acquire fpu context for too long
Quoted 6 lines
Quoted 2 lines · Robert (Servers)
Perhaps the cycles mode needs to call
cond_resched()too?Yes, just make the
cond_reschedunconditional. Having a few too many rescheds shouldn't be an issue.
This looks promising. I was able to trigger a lot of rcu stalls by setting:
echo 2 > /sys/module/rcupdate/parameters/rcu_cpu_stall_timeout echo 200 > /sys/module/rcupdate/parameters/rcu_exp_cpu_stall_timeout
and running these concurrently:
watch -n 0 modprobe tcrypt=200
watch -n 0 module tcrypt=0 through 999
After changing tcrypt to call cond_resched in both cases, I don't see any more rcu stalls.
I am getting miscompares from the extended self-test for crc32 and crct10dif, and will investigate those further.
BTW, the way tcrypt always refuses to load leads to an ever-growing list in the Call Traces:
kernel: Unloaded tainted modules: tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 t crypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():
1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt() :1 tcrypt():1 tcrypt():1 tcrypt():1
Quoted 3 lines
Quoted 3 lines
From: Herbert Xu <[email protected]>
...
Quoted 11 lines
Quoted 6 lines · Robert (Servers)
Quoted 2 lines
Perhaps the cycles mode needs to call
cond_resched()too?Yes, just make the
cond_reschedunconditional. Having a few too many rescheds shouldn't be an issue.This looks promising. I was able to trigger a lot of rcu stalls by setting:
echo 2 >/sys/module/rcupdate/parameters/rcu_cpu_stall_timeoutecho 200 >/sys/module/rcupdate/parameters/rcu_exp_cpu_stall_timeout
Quoted 6 lines
and running these concurrently:
watch -n 0 modprobe tcrypt=200
watch -n 0 module tcrypt=0 through 999I am getting miscompares from the extended self-test for crc32 and crct10dif, and will investigate those further.
The assembly functions for those two do not handle small sizes (unlike crc32c, which handles all sizes), so the new do/while loop needs to use larger steps and call the generic function for any leftover bytes. I've corrected that patch.
Quoted 2 lines
After changing tcrypt to call
cond_reschedin both cases, I don't see any more rcu stalls.
This ran cleanly with a few nights of testing with
CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
which compares the accelerated implementation to the generic implementation under randomly generated test vectors - a very helpful test suite. It only works with data sizes up to 2 * PAGE_SIZE, though.
With the cond_resched changes, rcu stalls are gone with one exception. If printing of Call Traces is triggered for any reason, either by a test failing (e.g., because of the CRC loop bugs) or by using a SysRq trigger like:
echo l > /proc/sysrq-trigger
then the very act of generating and printing the Call Traces can trigger rcu stall messages. Examples:
Oct 09 14:56:39 kernel: cryptomgr: alg: shash: crct10dif-pclmul test failed (wrong result) on test vector "random: psize=4303 ksize=0", cfg="random: use_final src_divs=[<flush>95.43%@+4, <flush>4.57%@+6]" Oct 09 14:56:39 kernel: cryptomgr: alg: self-tests for crct10dif using crct10dif failed (rc=-22) Oct 09 14:56:39 kernel: ------------[ cut here ]------------ Oct 09 14:56:39 kernel: alg: self-tests for crct10dif using crct10dif failed (rc=-22) Oct 09 14:56:39 kernel: WARNING: CPU: 55 PID: 10553 at crypto/testmgr.c:5837 alg_test.cold+0x3e/0x124 ... Oct 09 14:56:42 kernel: R10: 0000000000000003 R11: 0000000000000246 R12: 0000000000040000 Oct 09 14:56:42 kernel: rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
and:
Oct 10 19:45:02 kernel: sysrq: Show backtrace of all active CPUs ... Oct 10 19:45:03 kernel: </TASK> ... Oct 10 19:45:06 kernel: rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
If there are no WARNs or call traces, then there are no rcu stalls.
So, we should be aware that commit 09a5ef9644bc0e1 ("crypto: testmgr - WARN on test failure") might introduce additional errors.
Quoted 19 lines
BTW, the way tcrypt always refuses to load leads to an ever-growing list in the Call Traces:
kernel: Unloaded tainted modules: tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 t
crypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1tcrypt():
1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1tcrypt()
:1 tcrypt():1 tcrypt():1 tcrypt():1
That was already noticed and a fix has already been posted for 6.1 as commit 47cc75aa9283 ("module: tracking: Keep a record of tainted unloaded modules only")
Hi Elliott, Robert
2022. 10. 10. 오전 4:58에 Elliott, Robert (Servers) 이(가) 쓴 글:
Quoted 36 lines
Quoted 12 lines
-----Original Message-----
From: Herbert Xu <[email protected]>
Sent: Sunday, October 9, 2022 1:20 AM
To: Elliott, Robert (Servers) <[email protected]>
Cc: Ard Biesheuvel <[email protected]>; Taehee Yoo <[email protected]>; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] crypto: x86: Do not acquire fpu context for too longQuoted 6 lines
Quoted 2 lines · Robert (Servers)
Perhaps the cycles mode needs to call
cond_resched()too?Yes, just make the
cond_reschedunconditional. Having a few too many rescheds shouldn't be an issue.This looks promising. I was able to trigger a lot of rcu stalls by setting:
echo 2 >/sys/module/rcupdate/parameters/rcu_cpu_stall_timeoutecho 200 >/sys/module/rcupdate/parameters/rcu_exp_cpu_stall_timeoutand running these concurrently:
watch -n 0 modprobe tcrypt=200
watch -n 0 module tcrypt=0 through 999After changing tcrypt to call
cond_reschedin both cases, I don't see any more rcu stalls.I am getting miscompares from the extended self-test for crc32 and crct10dif, and will investigate those further.
BTW, the way tcrypt always refuses to load leads to an ever-growing list in the Call Traces:
Quoted 9 lines
kernel: Unloaded tainted modules: tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 t crypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1tcrypt():
1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
I tested mb_aead as well.
I can find rcu stalls easily while testing gcm(aes-generic) with the below commands. #shell1
while :
do
modprobe tcrypt mode=215 num_mb=1024
done
#shell2
while :
do
modprobe tcrypt mode=0
done
Then, I added cond_resched() as you mentioned.
crypto/tcrypt.c +1 -0
@@ -415,6 +415,7 @@ static void test_mb_aead_speed(const char *algo, int enc, int secs, }else { ret =test_mb_aead_cycles (data, enc, bs, num_mb); +cond_resched (); }
I can't see rcu stalls anymore, I think it works well.