Re: [PATCH v2 0/9] crypto: x86 - stop using the SIMD helper

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Apr 03, 2025 at 09:25:37AM +0800, Herbert Xu wrote:
> On Wed, Apr 02, 2025 at 10:19:30AM -0700, Eric Biggers wrote:
> >
> > This seems premature.  crypto_shash is documented to be usable in any context.
> > See the "Context:" comments in include/crypto/hash.h.  Similarly, developers
> > expect lib/ functions to be available in any context unless otherwise
> > documented.
> 
> Doing slow computations in a hard IRQ is a bad idea.  The whole
> point of a hard IRQ handler is to set a flag and defer everything
> to a different context.
> 
> Please show me one good reason why we should allow crypto in
> a hard IRQ.
>  
> > IMO, doing it for lib/ too would be going too far though.  The lib/ functions
> > should be easy to use and not have random requirements on the calling context.
> > And since they're just functions, it's easy for them to fall back to the generic
> > functions when needed.  Also note that for very short inputs it can actually be
> > faster to use no-SIMD code, as that avoids the overhead of a kernel-mode SIMD
> > section.  So the fallback sometimes exists anyway for that.
> 
> We already disallow SIMD in hard IRQs anyway (may_use_simd is
> always false in that context).  The only thing you could use
> is the generic implementation.
> 
> So making this change in lib/crypto does not take any functionality
> away.  You could still invoke the generic lib/crypto code directly.
> 
> It does mean that we take away a completely useless check for
> people who are actually doing crypto because crypto work should
> never be done in a hard IRQ.

It's not the 90s anymore.  Crypto is fast now, and used ubiquitously.

And "crypto" doesn't necessarily mean a large operation.  It can be hashing just
a few bytes of data, for example.

Also as you know, the crypto API includes some non-cryptographic algorithms too.

BTW, x86 does allow SIMD in hardirq context in some cases.

Certainly agreed that crypto in hardirqs is something to be avoided in general,
though.

So maybe your proposal is okay, if it's done properly.

The thing I actually have more of a problem with is that you tend to start
making random API changes without any of the necessary prerequisites like
updating documentation, or adding debug assertions to catch violations of new
requirements.  You've already started removing the fallbacks from shash (commit
3846c01d42526bc31), but neither of those things have been done.  So we're
currently in a weird state where the shash API is explicitly documented to work
in all contexts, but you've broken that.

- Eric