On Wed, Feb 05, 2025 at 09:23:18PM +0100, Mikulas Patocka wrote:
> Introduce ahash support for the "internal hash" algorithm.
>
> Rework the dm-integrity code to be able to run the "internal hash"
> either with a synchronous ("shash") or asynchronous ("ahash") hash
> algorithm implementation.
>
> The get_mac() function now tries to decide which of the digest
> implemenations to use if there is a choice:
> - If an ahash and shash tfm is available and both are backed by the
> same driver name it is assumed that the shash is the faster
> implementation and thus the shash tfm is delivered to the caller.
> - If an ahash and shash tfm is available but the backing device driver
> divers (different driver names) it is assumed that the ahash
> implementation is a "better" hardware based implementation and thus
> the ahash tfm is delivered to the caller.
> - If there is no choice, for example only an ahash or an shash
> implementation is available then this tfm is delivered to the
> caller. Especially in cases where only an ahash implementation is
> available this is now used instead of failing.
> - The caller can steer this choice by passing a NULL to the ahash or
> shash parameter, thus enforcing to only allocate an algorithm of the
> remaining possibility.
>
> The function integrity_sector_checksum() is now only a dispatcher
> function calling one of the two new functions
> integrity_ahash_sector_checksum() or integrity_shash_sector_checksum()
> based on which tfm is allocated based on the two new fields
> internal_shash and internal_ahash in struct dm_integrity_c.
>
> Together with this comes some slight rework around availability and
> digest size of the internal hash in use.
>
> Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>
> Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
> ---
> drivers/md/dm-integrity.c | 350 +++++++++++++++++++++++++++++++++-------------
> 1 file changed, 252 insertions(+), 98 deletions(-)
Thanks. This keeps the questionable ahash stuff from affecting the common case.
I do still want to emphasize that the use of ahash here is artificial, given
that the proposed code waits synchronously for each request to complete, and the
underlying "phmac" algorithm that Harald is trying to add
(https://lore.kernel.org/r/20250115162231.83516-1-freude@xxxxxxxxxxxxx) uses CPU
instructions (s390 CPACF) and therefore wants virtual addresses. IMO, the way
that things should be is that subsystems like dm-integrity, dm-verity,
fs-verity, etc. should implement ahash support *only* if they are trying to
support old-school, fully off-CPU hardware hash accelerators *and* it is used
properly with an async callback other than crypto_req_done in order to actually
utilize the hardware properly. That does not apply here, where the underlying
algorithm that Harald is adding could just be made an shash instead, provided
that shash was improved to support sleepable algorithms.
So, that is my opinion. But in practice it may be necessary to use ahash for
other use cases too if that is what Herbert insists on.
- Eric
