Received-SPF: pass (google.com: domain of linux-crypto+bounces-4580-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 30 May 2024 23:13:48 -0700
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
Message-ID: <20240531061348.GG6505@sol.localdomain>
References: <20240507002343.239552-7-ebiggers@kernel.org>
 <ZllXDOJKW2pHWBTz@gondor.apana.org.au>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZllXDOJKW2pHWBTz@gondor.apana.org.au>

On Fri, May 31, 2024 at 12:50:20PM +0800, Herbert Xu wrote:
> Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > +               if (multibuffer) {
> > +                       if (ctx->pending_data) {
> > +                               /* Hash and verify two data blocks. */
> > +                               err = fsverity_hash_2_blocks(params,
> > +                                                            inode,
> > +                                                            ctx->pending_data,
> > +                                                            data,
> > +                                                            ctx->hash1,
> > +                                                            ctx->hash2);
> > +                               kunmap_local(data);
> > +                               kunmap_local(ctx->pending_data);
> > +                               ctx->pending_data = NULL;
> > +                               if (err != 0 ||
> > +                                   !verify_data_block(inode, vi, ctx->hash1,
> > +                                                      ctx->pending_pos,
> > +                                                      ctx->max_ra_pages) ||
> > +                                   !verify_data_block(inode, vi, ctx->hash2,
> > +                                                      pos, ctx->max_ra_pages))
> > +                                       return false;
> > +                       } else {
> > +                               /* Wait and see if there's another block. */
> > +                               ctx->pending_data = data;
> > +                               ctx->pending_pos = pos;
> > +                       }
> > +               } else {
> > +                       /* Hash and verify one data block. */
> > +                       err = fsverity_hash_block(params, inode, data,
> > +                                                 ctx->hash1);
> > +                       kunmap_local(data);
> > +                       if (err != 0 ||
> > +                           !verify_data_block(inode, vi, ctx->hash1,
> > +                                              pos, ctx->max_ra_pages))
> > +                               return false;
> > +               }
> > +               pos += block_size;
> 
> I think this complexity is gross.  Look at how we did GSO in
> networking.  There should be a unified code-path for aggregated
> data and simple data, not an aggregated path versus a simple path.
> 
> I think ultimately it stems from the fact that this code went from
> ahash to shash.  What were the issues back then? If it's just vmalloc
> we should fix ahash to support that, rather than making users of the
> Crypto API go through contortions like this.

It can't be asynchronous, period.  As I've explained, that would be far too
complex, and it would also defeat the purpose because it would make performance
worse.  Messages *must* be queued up and hashed in the caller's context.

What could make sense would be some helper functions and an associated struct
for queueing up messages for a particular crypto_shash, up to its mb_max_msgs
value, and then flushing them and retrieving the digests.  These would be
provided by the crypto API.

I think this would address your concern, in that the users (fsverity and
dm-verity) would have a unified code path for multiple vs. single blocks.

I didn't think it would be worthwhile to go there yet, given that fsverity and
dm-verity just want 2x or 1x, and it ends up being simpler and more efficient to
handle those cases directly.  But we could go with the more general queueing
helper functions instead if you feel they should be included from the start.

- Eric