Re: [PATCH] crypto: api - Fix generic algorithm self-test races

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Oct 06, 2024 at 08:53:28AM +0800, Herbert Xu wrote:
> On Sat, Oct 05, 2024 at 03:24:48PM -0700, Eric Biggers wrote:
> >
> > The tests are still failing on upstream:
> > 
> > [    0.343845] alg: self-tests for rfc4106(gcm(aes)) using rfc4106(gcm_base(ctr(aes-generic),ghash-generic)) failed (rc=-2)
> 
> You're right.  I only disabled the warnings at the point of
> allocation, the overall self-test warning is still there.  Let
> me get rid of them too.
> 
> > Besides the test failures, it looks like there's no longer any guarantee that
> > algorithms are actually available now that their module is loaded.
> 
> That would indeed be a bug.  But I haven't seen it in practice.
> Although the s390 folks were reporting some weird errors with
> dm-crypt, they have recently disappeared.
> 
> If you do see an actual failure please report it and then I'll
> consider reverting it until it's fixed.
> 
> > E.g. consider if someone does 'modprobe aesni-intel' and then immediately
> > creates a dm-crypt device.  Now it sounds like the AES-NI algorithms might not
> > have finished being tested yet and the generic algorithms can be used instead,
> > resulting in a performance regression.
> 
> That is not the case.  After modprobe returns, the algorithm is
> guaranteed to have been registered.  Yes it is untested, but that
> should not be a problem because a test larval will have been created
> and all users looking for that algorithm will be waiting on that
> test larval.

I'm not sure about that, since the code that looks up algorithms only looks for
algorithms that already have the CRYPTO_ALG_TESTED flag.

> > I understand that you want to try to fix the edge cases in "fallback" ciphers.
> > But "fallback" ciphers have always seemed like a bad design due to how they use
> > the crypto API recursively.  I think the algorithms that use them should
> > generally be migrated off of them, e.g. as I did in commit f235bc11cc95
> > ("crypto: arm/aes-neonbs - go back to using aes-arm directly").  That fixed the
> > problem in aes-neonbs that seems to have triggered this work in the first place.
> 
> Yes getting rid of fallbacks is nice, but this it not the reason why
> we're making self-test asynchronous.  The primary issue with synchronous
> self-tests is the modprobe dead-lock.

That problem is caused by the use of fallback ciphers, though.

- Eric