On 2025-01-16 09:03, Eric Biggers wrote:
On Thu, Jan 16, 2025 at 08:33:46AM +0100, Harald Freudenberger wrote:
On 2025-01-15 18:37, Eric Biggers wrote:
> On Wed, Jan 15, 2025 at 05:46:57PM +0100, Harald Freudenberger wrote:
> > Use the async digest in-kernel crypto API instead of the
> > synchronous digest API. This has the advantage of being able
> > to use synchronous as well as asynchronous digest implementations
> > as the in-kernel API has an automatic wrapping mechanism
> > to provide all synchronous digests via the asynch API.
> >
> > Tested with crc32, sha256, hmac-sha256 and the s390 specific
> > implementations for hmac-sha256 and protected key phmac-sha256.
> >
> > Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>
>
> As Mikulas mentioned, this reduces performance for everyone else, which
> is not
> great. It also makes the code more complicated.
>
> I also see that you aren't actually using the algorithm in an async
> manner, but
> rather waiting for it synchronously each time. Thus the ability to
> operate
> asynchronously provides no benefit in this case, and this change is
> purely about
> allowing a particular driver to be used, presumably the s390 phmac one
> from your
> recent patchset. Since s390 phmac seems to be new code, and furthermore
> it is
> CPU-based and thus uses virtual addresses (which makes the use of
> scatterlists
> entirely pointless), wouldn't it be easier to just make it implement
> shash
> instead of ahash, moving any wait that may be necessary into the driver
> itself?
>
> - Eric
Thanks for this feedback. I'll give it a try with some performance
measurements.
And I totally agree that a synchronous implementation of phmac whould
have
solved
this also. But maybe you can see that this is not an option according
to
Herbert Xu's feedback about my first posts with implementing phmac as
an
shash.
The thing is that we have to derive a hardware based key (pkey) from
the
given key material and that may be a sleeping call which a shash must
not
invoke.
So finally the phmac implementation is now an ahash digest
implementation
as suggested by Herbert.
You are right, my patch is not really asynchronous. Or at least
waiting for
completion at the end of each function. However, opposed to the ahash
invocation
where there have been some update() calls this is now done in just one
digest()
giving the backing algorithm a chance to hash all this in one step
(well it
still
needs to walk the scatterlist).
Is there a way to have dm-integrity accept both, a ahash() or a
shash()
digest?
To properly support async algorithms, the users (e.g. dm-integrity and
dm-verity) really would need to have separate code paths anyway. The
computation models are just too different.
But in this case, it seems you simply want it to be synchronous and use
virtual
addresses. The quirks of ahash, including its need for per-request
allocations
and scatterlists, make it a poor match here. The only thing you are
getting
with it is, ironically, that it allows you to wait synchronously. That
could be
done with shash too if it was fixed to support algorithms that aren't
atomic.
E.g. there could be a new CRYPTO_ALG_MAY_SLEEP flag that could be set
in struct
shash_alg to indicate that the algorithm doesn't support atomic
context, and a
flag could be passed to crypto_alloc_shash() to allow such an algorithm
to be
selected (if the particular user never uses it in atomic context).
That would be faster and simpler than the proposed ahash based version.
- Eric
Eric your idea was whirling around in my head for at least 3 month now.
It would be great to have a way to offer an shash() implementation which
clearly states that it is not for use in atomic context. E.g. like as
you
wrote with a new flag. I'll work out something in this direction and
push
it onto the crypto mailing list :-)
