Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Feb 12, 2025 at 12:17:46PM +0100, Harald Freudenberger wrote:
> On 2025-02-09 17:34, Eric Biggers wrote:
> > On Sun, Feb 09, 2025 at 04:47:57PM +0800, Herbert Xu wrote:
> > > On Wed, Jan 15, 2025 at 05:22:28PM +0100, Harald Freudenberger wrote:
> > > >
> > > > +static int s390_phmac_init(struct ahash_request *req)
> > > > +{
> > > > +	struct s390_phmac_req_ctx *req_ctx = ahash_request_ctx(req);
> > > > +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> > > > +	struct s390_kmac_sha2_ctx *ctx = &req_ctx->sha2_ctx;
> > > > +	int rc;
> > > > +
> > > > +	/*
> > > > +	 * First try synchronous. If this fails for any reason
> > > > +	 * schedule this request asynchronous via workqueue.
> > > > +	 */
> > > > +
> > > > +	rc = phmac_init(tfm, ctx, false);
> > > > +	if (!rc)
> > > > +		goto out;
> > > > +
> > > > +	req_ctx->req = req;
> > > > +	INIT_DELAYED_WORK(&req_ctx->work, phmac_wq_init_fn);
> > > > +	schedule_delayed_work(&req_ctx->work, 0);
> > > > +	rc = -EINPROGRESS;
> > > 
> > > This creates a resource problem because there is no limit on how
> > > many requests that can be delayed in this manner for a given tfm.
> > > 
> > > When we hit this case, I presume this is a system-wide issue and
> > > all requests would go pending? If that is the case, I suggest
> > > allocating a system-wide queue through crypto_engine and using
> > > that to limit how many requests that can become EINPROGRESS.
> > 
> > Or just make it synchronous which would be way easier, and the calling
> > code uses
> > it synchronously anyway.
> > 
> > - Eric
> 
> A word about synchronous vs asynchronous...
> 
> As a synchronous hash (or chipher or whatever) MUST NOT sleep I can't
> really implement the pkey stuff in a synchronous way:

As I said at
https://lore.kernel.org/dm-devel/20250116080324.GA3910@sol.localdomain/, shash
could fairly easily be fixed to support sleepable algorithms (e.g.
CRYPTO_ALG_SLEEPABLE).

This would be *much* simpler than doing it with ahash.

You even had it as a shash already in the first version of your patchset, just
missing the bits that add the support for sleepable algorithms.

I am trying to help you by suggesting an approach that would be much easier.
There is no need to shoehorn CPU-based crypto into ahash, which is designed for
off-CPU offload.

> The issue with pkey (We call it "protected key") is that it is some kind
> of hardware based key. As such it needs some special preparation action
> to be done upfront in the hardware/firmware to use such a pkey.
> Now think about KVM live guest migration where a guest suddenly awakes
> (Well the guest is not even aware of this) on a new machine with another
> hardware. So out of the sudden a hardware based crypto operation fails
> with an indication that the hardware/firmware can't deal with this
> key object and needs re-preparation. Usually this preparation step is
> some kind of asynchronous operation (write some pci registers or run
> some DMA sequences or refresh the working key material via an HSM
> communication...) and as such may take some time and involve even
> sleeping on a mutex or completion until another kernel thread is done.
> Please note this is not unique to pkey on system z but may apply
> to all kinds of hardware/firmware based keys in situations like
> KVM live guest migration or suspend/resume.

I.e. it already uses a kernel thread that does the operation synchronously (as
opposed to being actual hardware offload that does not consume a thread and
signals completion to the CPU via an interrupt), in addition to the caller's
thread which also waits synchronously via crypto_wait_req().  There is really no
need to make it async, nor to use scatterlists.

- Eric