Re: [v2 PATCH 00/13] Architecture-optimized SHA-256 library API

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Apr 27, 2025 at 05:56:43AM -0700, Eric Biggers wrote:
> On Sun, Apr 27, 2025 at 08:41:38PM +0800, Herbert Xu wrote:
> > On Sun, Apr 27, 2025 at 05:35:14AM -0700, Eric Biggers wrote:
> > >
> > > Well, barely a day and you've already ruined my patch series.  Now instead of a
> > > clean design where the crypto_shash API is built on top of the normal library
> > > API (sha256_update() etc.), there's now a special low-level API
> > > "sha256_choose_blocks()" just for shash that it's built on top of instead, for
> > > no good reason.  You're also still pushing your broken BLOCK_HASH_UPDATE_BLOCKS
> > > macro that doesn't work with size_t, and putting my name on your broken code
> > > that uses it.
> > 
> > Your design is unacceptable because you're forcing the partial block
> > handling on shash where it's not needed,
> 
> Excuse me?  It's the other way around.  In my version the partial block handling
> is only in the library, not shash.  In your version you've forced it into the
> shash layer, even though the library does it already.  I understand that you've
> added support for partial block handling to crypto/shash.c and you want to feel
> like your work is useful, but in this case it's not, since the libray has to
> handle arbitrary-length inputs anyway.
> 
> > just as you're forcing the hardirq support on everything.
> 
> If you want crypto_shash to warn on hardirq usage you should just put a
> WARN_ON(in_hardirq()) in crypto_shash_*(), which will actually achieve that.
> Not add a shash-specific non-hardirq-safe low-level API to the library that can
> silently corrupt random tasks' SIMD registers on production systems.

By the way, as I mentioned in my cover letter:

    For now the SHA-256 library is well-covered by the crypto_shash
    self-tests, but I plan to add a test for the library directly later.

But due to your gratuitous changes where crypto_shash is no longer built on top
of the normal SHA-256 library API, that's no longer the case.

So while I do still plan to add a SHA-256 library test anyway, I don't see the
reason for not also making crypto_shash just do the right thing.

- Eric