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

From: Gonglei (Arei)
Date: Wed Nov 30 2016 - 21:28:06 EST


Hi Stefan,

>
> On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> b/drivers/crypto/virtio/virtio_crypto_algs.c
> > new file mode 100644
> > index 0000000..08b077f
> > --- /dev/null
> > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> > @@ -0,0 +1,518 @@
> > + /* Algorithms supported by virtio crypto device
> > + *
> > + * Authors: Gonglei <arei.gonglei@xxxxxxxxxx>
> > + *
> > + * Copyright 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/scatterlist.h>
> > +#include <crypto/algapi.h>
> > +#include <linux/err.h>
> > +#include <crypto/scatterwalk.h>
> > +#include <linux/atomic.h>
> > +
> > +#include <uapi/linux/virtio_crypto.h>
> > +#include "virtio_crypto_common.h"
> > +
> > +static DEFINE_MUTEX(algs_lock);
>
> Did you run checkpatch.pl? I think it encourages you to document what
> the lock protects.
>
Sure. Basically I run checkpatch.py each time. :)

# ./scripts/checkpatch.pl 0001-crypto-add-virtio-crypto-driver.patch
total: 0 errors, 0 warnings, 1873 lines checked

0001-crypto-add-virtio-crypto-driver.patch has no obvious style problems and is ready for submission.

> > +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.

Regards,
-Gonglei