Re: [PATCH v1 1/1] dm-integrity: Implement asynch digest support

[Harald Freudenberger <freude@linux.ibm.com>]

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@linux.ibm.com>
>> >
>> > 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 :-)