Re: [PATCH v3 6/8] fsverity: improve performance by using multibuffer hashing

From: Eric Biggers <ebiggers@kernel.org>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: linux-crypto@vger.kernel.org, fsverity@lists.linux.dev,
	dm-devel@lists.linux.dev, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, ardb@kernel.org,
	samitolvanen@google.com, bvanassche@acm.org
Subject: Re: [PATCH v3 6/8] fsverity: improve performance by using multibuffer hashing
Date: Mon, 3 Jun 2024 11:50:02 -0700	[thread overview]
Message-ID: <20240603185002.GA35358@sol.localdomain> (raw)
In-Reply-To: <20240531185126.GA1153@sol.localdomain>

On Fri, May 31, 2024 at 11:51:26AM -0700, Eric Biggers wrote:
> On Fri, May 31, 2024 at 04:49:28PM +0800, Herbert Xu wrote:
> > On Thu, May 30, 2024 at 11:52:58PM -0700, Eric Biggers wrote:
> > >
> > > Looking at it again a bit more closely, both fsverity and dm-verity have
> > > per-block information that they need to keep track of in the queue in addition
> > > to the data buffers and hashes: the block number, and in dm-verity's case also a
> > > bvec_iter pointing to that block.
> > 
> > Again I'm not asking you to make this API asynchronous at all.
> 
> What exactly are you suggesting, then?  It seems that you want multibuffer
> hashing to be supported by the existing ahash API.  However, that only works if
> it's made asynchronous, given how the messages would have to be queued up on a
> global queue.  That has a huge number of issues which I've already explained.
> (And it was even tried before, and it failed.)
> 
> > I was just commenting on the added complexity in fsverify due to
> > the use of the linear shash API instead of the page-based ahash API.
> 
> It's the other way around.  The shash version is much simpler.  Just look at the
> diff of commit 8fcd94add6c5 that changed from ahash to shash:
> 
>     4 files changed, 71 insertions(+), 200 deletions(-)
> 
> > This complexity was then compounded by the multi-buffer support.
> 
> fsverity and dm-verity will have to be updated to use multibuffer hashing
> anyway, given that transparently supporting it in the existing API is not
> viable.  If your concern is that in my current patchset fsverity and dm-verity
> have separate code paths for multibuffer vs. single-buffer, as I mentioned I can
> address that by restructuring them to operate on arrays (similar to what I
> already did with the crypto API part).
> 
> > I think this would look a lot simpler if it moved back to ahash.
> > 
> > The original commit mentioned that ahash was bad for fsverify
> > because of vmalloc.  But the only use of linear pointers in fsverify
> > seems to be from kmalloc.  Where is the vmalloc coming from?
> 
> XFS is working on adding support for fsverity, and XFS was planning to provide
> their Merkle tree blocks in vmalloced buffers.  Their plans have shifted several
> times, and I think their latest plan no longer uses vmalloced buffers.  But in
> any case it's still very convenient and much simpler to be able to just use
> virtual addresses without having to worry about what type of memory it is.
> 

I've now unified the code paths for single-block and multi-block processing in
fsverity and dm-verity, which I think addresses your remaining concern that's
feasible to address.  Your feedback has been conflating different issues, and it
only comes in once every few weeks, so it's a bit hard to follow.  But I think
the remaining thing that you're asking for is to make the API to be part of
"ahash" and use the existing async completion callback.  That's not actually
feasible, for the reasons that I've explained in detail.  See in particular
https://lore.kernel.org/linux-crypto/20240503152810.GA1132@sol.localdomain/.

Thanks,

- Eric

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel