Re: [GIT PULL] Crypto Update for 6.15

[Herbert Xu <herbert@gondor.apana.org.au>]

On Tue, Mar 25, 2025 at 08:25:41AM -0700, Eric Biggers wrote:
> 
> 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's see how your version is so much better:

https://lore.kernel.org/all/20250212154718.44255-6-ebiggers@kernel.org/

-	/* Up to 1 + FS_VERITY_MAX_LEVELS pages may be mapped at once */
-	BUILD_BUG_ON(1 + FS_VERITY_MAX_LEVELS > KM_MAX_IDX);
+	/*
+	 * Up to FS_VERITY_MAX_PENDING_DATA_BLOCKS + FS_VERITY_MAX_LEVELS pages
+	 * may be mapped at once.
+	 */
+	BUILD_BUG_ON(FS_VERITY_MAX_PENDING_DATA_BLOCKS +
+		     FS_VERITY_MAX_LEVELS > KM_MAX_IDX);

This arbitrary limit is a direct result of your welded-on commitment
to an API that supports virtually mapped addresses only.  Make no
mistake, virtual addresses are simple and easy to use, but the kernel
added more complicated constructs for real reasons.

I've gone through your use-case in fsverity/dm-verity, and they
never touch the data at all so the only reason for it to kmap the
data at all is to feed it to the Crypto API, which is capable of
doing its own kmap but you elected not to use that because you
hate the interface.

In fact it's a recurring theme, the zswap code jumps through multiple
hoops to map the data they're working on so that they can feed it to
the Crypto API as a virtually mapped pointer, even though they never
touch the mapped data at all.  The same thing also happened in ubifs,
which I managed to simplify by switching away from kmapped pointers:

https://patchwork.kernel.org/project/linux-crypto/patch/99ae6a15afc1478bab201949dc3dbb2c7634b687.1742034499.git.herbert@gondor.apana.org.au/

-		addr += UBIFS_BLOCK_SIZE;
-		if (folio_test_highmem(folio) && (offset_in_page(addr) == 0)) {
-			kunmap_local(addr - UBIFS_BLOCK_SIZE);
-			addr = kmap_local_folio(folio, i * UBIFS_BLOCK_SIZE);
-		}
+		offset += UBIFS_BLOCK_SIZE;

All this complexity was added because the legacy compression
interface only supported virtually mapped addresses.

Sure the ahash/acomp interface was suboptimal for *only* supporting
SG lists, which is what this pull request addresses by adding virtual
address (and folio) support.

> Let me reiterate why "request chaining" is a bad idea and is going to cause
> problems.

I'm more than willing to discuss with you the implementation details
of how the chaining is done and improving it.  However, if you proceed
to only issue blanket nacks without providing any constructive feedback,
then the only thing I can do is ignore you.

> 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.

Everybody wants to sratch their itch but my job as the maintainer is
to ensure that the subsystem doesn't collapse into an unmaintainable
hodgepodge of individual contributions.

Yes I get that batching is useful for you, but your use-case is not
unique at all.  The compression people are proposing pretty much the
same thing

https://patchwork.kernel.org/project/linux-crypto/patch/20250303084724.6490-15-kanchana.p.sridhar@intel.com/

I don't want to be pressured by you into committing to an interface
that works for you only.
 
> Nacked-by: Eric Biggers <ebiggers@kernel.org>

This pull request doesn't even contain the meat of the hash changes
since I've been busy with the compression work.  So this is simply
a pre-emptive strike to stop further work from rendering your patches
obsolete.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt