Message-ID: <CAMj1kXFM29ZM5ncyZLrAbX35V7y8=C-n8KT0y1SZhafK_+5aHg@mail.gmail.com>
Date: Tue, 25 Mar 2025 17:59:39 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Eric Biggers <ebiggers@...nel.org>, Herbert Xu <herbert@...dor.apana.org.au>, 
	Linus Torvalds <torvalds@...ux-foundation.org>
Cc: "David S. Miller" <davem@...emloft.net>, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, 
	Linux Crypto Mailing List <linux-crypto@...r.kernel.org>
Subject: Re: [GIT PULL] Crypto Update for 6.15

On Tue, 25 Mar 2025 at 16:25, Eric Biggers <ebiggers@...nel.org> wrote:
>
> On Tue, Mar 25, 2025 at 01:53:28PM +0800, Herbert Xu wrote:
> >
> >       crypto: hash - Add request chaining API
>
> Herbert didn't mention that I have nacked this patch, which he is insisting on
> pushing for some reason instead of my original version that is much better.
>
> Let me reiterate why "request chaining" is a bad idea and is going to cause
> problems.
>
> It makes it so that now a single hash request can now actually be a list of hash
> requests.  It makes some of the crypto code operate on the whole list.  However,
> most code still operates only on the first request in the list.  It's
> undocumented and inconsistent which code is doing which, which is going to cause
> bugs.  The first request in the list is also being treated specially in
> undocumented ways, so submitting a list of requests is not necessarily
> equivalent to submitting them all individually.  Another recipe for bugs.
>
> Each hash request can also contain an entire scatterlist.  It's overkill for
> what is actually needed for multibuffer hashing, which is a simple API that
> hashes two buffers specified by virtual address.  Herbert's API creates lots of
> unnecessary edge cases, most of which lack any testing.  It continues many of
> the worst practices of the crypto API that we *know* are not working, like
> requiring per-request memory allocations and optimizing for legacy hardware
> offload rather than the CPU-based crypto that almost everyone actually uses.
>
> In contrast, my patchset
> https://lore.kernel.org/r/20250212154718.44255-1-ebiggers@kernel.org/ supports
> multibuffer hashing in a much better way and has been ready for a year already.
> It actually works; it has a smaller diffstat; it is faster; it has a much
> simpler API; and it actually includes all needed pieces including x86 and arm64
> support, dm-verity and fs-verity support, and full documentation and tests.
>
> I've been spending a lot of time fixing the kernel's crypto code over the years.
> I'm not looking forward to having another set of major issues to fix.
>
> And this latest set of issues will be totally unnecessary.
>
> We can do better than this, especially for cryptography code.
>
> Nacked-by: Eric Biggers <ebiggers@...nel.org>
>

It's sad that it is coming to this, but I have to second Eric here:
for CPU based crypto, the flexibility of Herbert's approach has no
added value. SHA CPU instructions can be interleaved at the
instruction level to get almost 2x speedup in some cases, and this
works very well when operating on equal sized inputs. However,
generalizing this to arbitrary request chains to accommodate async h/w
offload introduces a lot of complexity for use cases that are only
imaginary.

Given Eric's track record as a contributor to the crypto subsystem and
as a maintainer of subsystems that are closely tied to it, I would
expect Herbert to take his opinion more seriously, but it is just
being ignored. Instead, a lightly tested alternative with no
integration into existing users has been merged in its place, with
very little input from the community.

So Herbert, please withdraw this pull request, and work with Eric and
the rest of us to converge on something that we can all get behind.

Powered by blists - more mailing lists