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
