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
