> -----Original Message-----
> From: Taehee Yoo <ap420073@xxxxxxxxx>
> Sent: Tuesday, October 4, 2022 1:02 AM
> To: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-crypto@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; tglx@xxxxxxxxxxxxx;
> mingo@xxxxxxxxxx; bp@xxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx;
> hpa@xxxxxxxxx; ardb@xxxxxxxxxx; ebiggers@xxxxxxxxxx
> Subject: Re: [PATCH] crypto: x86: Do not acquire fpu context for too long
>
> Hi Herbert,
> Thanks a lot for your review!
>
> On 10/4/22 13:52, Herbert Xu wrote:
> > On Tue, Oct 04, 2022 at 04:49:12AM +0000, Taehee Yoo wrote:
> >>
> >> #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)
> >
> > 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
> 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 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.
