Re: [PATCH v4 1/1] crypto: add virtio-crypto driver

[Stefan Hajnoczi]

On Thu, Dec 01, 2016 at 02:27:19AM +0000, Gonglei (Arei) wrote:
> > On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > > +static int virtio_crypto_alg_ablkcipher_init_session(
> > > +		struct virtio_crypto_ablkcipher_ctx *ctx,
> > > +		uint32_t alg, const uint8_t *key,
> > > +		unsigned int keylen,
> > > +		int encrypt)
> > > +{
> > > +	struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> > > +	unsigned int tmp;
> > > +	struct virtio_crypto *vcrypto = ctx->vcrypto;
> > > +	int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT :
> > VIRTIO_CRYPTO_OP_DECRYPT;
> > > +	int err;
> > > +	unsigned int num_out = 0, num_in = 0;
> > > +
> > > +	/*
> > > +	 * Avoid to do DMA from the stack, switch to using
> > > +	 * dynamically-allocated for the key
> > > +	 */
> > > +	uint8_t *cipher_key = kmalloc(keylen, GFP_ATOMIC);
> > > +
> > > +	if (!cipher_key)
> > > +		return -ENOMEM;
> > > +
> > > +	memcpy(cipher_key, key, keylen);
> > 
> > Are there any rules on handling key material in the kernel?  This buffer
> > is just kfreed later.  Do you need to zero it out before freeing it?
> > 
> Good questions. For kernel crypto core, each cipher request should be freed
> by skcipher_request_free(): zeroize and free request data structure.
> 
> I need to use kzfree() for key as well. I'll also check other stuffs. Thanks. 
> 
> > > +
> > > +	spin_lock(&vcrypto->ctrl_lock);
> > 
> > The QAT accelerator driver doesn't spin while talking to the device in
> > virtio_crypto_alg_ablkcipher_init_session().  I didn't find any other
> > driver examples in the kernel tree, but this function seems like a
> > weakness in the virtio-crypto device.
> > 
> The control queues of virtio-net and virtio-console are also be locked
> Please see:
>  __send_control_msg() in virtio_console.c and virtio-net's control queue
> protected by rtnl lock.
> 
> I didn't want to protect session creations but the virtqueue's operations
> like what other virtio devices do.
> 
> > While QEMU is servicing the create session command this vcpu is blocked.
> > The QEMU global mutex is held so no other vcpu can enter QEMU and the
> > QMP monitor is also blocked.
> > 
> > This is a scalability and performance problem.  Can you look at how QAT
> > avoids this synchronous session setup?
> 
> For QAT driver, the session creation is synchronous as well because it's a
> plain software operation which can be completed ASAP.

I'm mentioning the vmexit and wait for request completion in a spinlock
because the same type of issue has been a performance bottleneck with
virtio guest driver in the past.

If there is a way to avoid spinning then that would be preferred.  It's
basically a known anti-pattern for virtio guest drivers.

Could you initialize the session on the host side when the first
asynchronous data is submitted for encryption/decryption instead of
during init_session()?

Stefan

Attachment: signature.asc
Description: PGP signature