Re: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver
On 12/14/2016 12:50 PM, Gonglei wrote:
> diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> b/drivers/crypto/virtio/virtio_crypto_core.c
> new file mode 100644
> index 000..c0854a1
> --- /dev/null
> +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> @@ -0,0 +1,474 @@
[..]
> +
> +static void virtcrypto_dataq_callback(struct virtqueue *vq)
> +{
> + struct virtio_crypto *vcrypto = vq->vdev->priv;
> + struct virtio_crypto_request *vc_req;
> + unsigned long flags;
> + unsigned int len;
> + struct ablkcipher_request *ablk_req;
> + int error;
> +
> + spin_lock_irqsave(&vcrypto->lock, flags);
Would it make sense to use a per virtqueue lock
like in virtio_blk for example instead of locking on the whole
device? OK, it seems you use only one dataqueue, so it
may not be that relevant.
> + do {
> + virtqueue_disable_cb(vq);
> + while ((vc_req = virtqueue_get_buf(vq, &len)) != NULL) {
> + if (vc_req->type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> + switch (vc_req->status) {
> + case VIRTIO_CRYPTO_OK:
> + error = 0;
> + break;
> + case VIRTIO_CRYPTO_INVSESS:
> + case VIRTIO_CRYPTO_ERR:
> + error = -EINVAL;
> + break;
> + case VIRTIO_CRYPTO_BADMSG:
> + error = -EBADMSG;
> + break;
> + default:
> + error = -EIO;
> + break;
> + }
> + ablk_req = vc_req->ablkcipher_req;
> + virtcrypto_clear_request(vc_req);
> +
> + spin_unlock_irqrestore(&vcrypto->lock, flags);
> + /* Finish the encrypt or decrypt process */
> + ablk_req->base.complete(&ablk_req->base, error);
> + spin_lock_irqsave(&vcrypto->lock, flags);
> + }
> + }
> + } while (!virtqueue_enable_cb(vq));
> + spin_unlock_irqrestore(&vcrypto->lock, flags);
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/2] crypto: add virtio-crypto driver
On 12/12/2016 11:05 PM, Michael S. Tsirkin wrote: > On Mon, Dec 12, 2016 at 06:54:07PM +0800, Herbert Xu wrote: >> On Mon, Dec 12, 2016 at 06:25:12AM +, Gonglei (Arei) wrote: >>> Hi, Michael & Herbert >>> >>> Because the virtio-crypto device emulation had been in QEMU 2.8, >>> would you please merge the virtio-crypto driver for 4.10 if no other >>> comments? If so, Miachel pls ack and/or review the patch, then >>> Herbert will take it (I asked him last week). Thank you! >>> >>> Ps: Note on 4.10 merge window timing from Linus >>> https://lkml.org/lkml/2016/12/7/506 >>> >>> Dec 23rd is the deadline for 4.10 merge window. >> >> Sorry but it's too late for 4.10. It needed to have been in my >> tree before the merge window opened to make it for this cycle. >> >> Cheers, > > > Objections to me merging this? I'm preparing my tree right now. Got this when testing the most recent version on s390x [ 20.391074] test 0 (128 bit key, 16 byte blocks): [ 20.391078] BUG: using smp_processor_id() in preemptible [] code: insmod/97 [ 20.391082] caller is virtio_crypto_ablkcipher_setkey+0x44/0x198 [ 20.391085] CPU: 0 PID: 97 Comm: insmod Not tainted 4.9.0-02683-gb62a1ab #46 [ 20.391088] Hardware name: IBM 2964 NC9 704 (KVM) [ 20.391405] Stack: [ 20.391407]0c0eb6d0 0c0eb760 0003 [ 20.391414]0c0eb800 0c0eb778 0c0eb778 0020 [ 20.391420] 000a 0020 03ff000a [ 20.391426]03ff000c 0c0eb7c8 [ 20.391432]07c173c8 001126ba 0c0eb760 0c0eb7b8 [ 20.391439] Call Trace: [ 20.391442] ([<0011259e>] show_trace+0x8e/0xe0) [ 20.391446] [<00112670>] show_stack+0x80/0xd8 [ 20.391449] [<00753ab6>] dump_stack+0x96/0xd8 [ 20.391453] [<007872e6>] check_preemption_disabled+0xfe/0x128 [ 20.391456] [<00839cc4>] virtio_crypto_ablkcipher_setkey+0x44/0x198 [ 20.391459] [<00705a40>] skcipher_setkey_ablkcipher+0x50/0x70 [ 20.391476] [<03ff80002a48>] test_skcipher_speed+0x328/0xb98 [tcrypt] [ 20.391492] [<03ff800063dc>] do_test+0x1c24/0x28e0 [tcrypt] [ 20.391509] [<03ff8001006a>] tcrypt_mod_init+0x6a/0x1000 [tcrypt] [ 20.391512] [<001002cc>] do_one_initcall+0xb4/0x148 [ 20.391515] [<00298632>] do_init_module+0x7a/0x228 [ 20.391519] [<001fd380>] load_module+0x2428/0x2de0 [ 20.391522] [<001fde8a>] SyS_init_module+0x152/0x160 [ 20.391526] [<009f1306>] system_call+0xd6/0x270 [ 20.391528] no locks held by insmod/97. Gonglei, any idea? Did not look into it myself yet. Halil > >> -- >> Email: Herbert Xu >> Home Page: http://gondor.apana.org.au/~herbert/ >> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] crypto: add virtio-crypto driver
On 11/28/2016 06:19 PM, Michael S. Tsirkin 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);
>>> > > +
>>> > > + spin_lock(&vcrypto->ctrl_lock);
>>> > > + /* Pad ctrl header */
>>> > > + vcrypto->ctrl.header.opcode =
>>> > > + cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
>>> > > + vcrypto->ctrl.header.algo = cpu_to_le32(alg);
>>> > > + /* Set the default dataqueue id to 0 */
>>> > > + vcrypto->ctrl.header.queue_id = 0;
>>> > > +
>>> > > + vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>>> > > + /* Pad cipher's parameters */
>>> > > + vcrypto->ctrl.u.sym_create_session.op_type =
>>> > > + cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
>>> > > + vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo =
>>> > > + vcrypto->ctrl.header.algo;
>>> > > + vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen =
>>> > > + cpu_to_le32(keylen);
>>> > > + vcrypto->ctrl.u.sym_create_session.u.cipher.para.op =
>>> > > + cpu_to_le32(op);
>>> > > +
>>> > > + sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
>>> > > + sgs[num_out++] = &outhdr;
>>> > > +
>>> > > + /* Set key */
>>> > > + sg_init_one(&key_sg, cipher_key, keylen);
>>> > > + sgs[num_out++] = &key_sg;
>>> > > +
>>> > > + /* Return status and session id back */
>>> > > + sg_init_one(&inhdr, &vcrypto->input, sizeof(vcrypto->input));
>>> > > + sgs[num_out + num_in++] = &inhdr;
>>> > > +
>>> > > + err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
>>> > > + num_in, vcrypto, GFP_ATOMIC);
>>> > > + if (err < 0) {
>>> > > + spin_unlock(&vcrypto->ctrl_lock);
>>> > > + kfree(cipher_key);
>>> > > + return err;
>>> > > + }
>>> > > + virtqueue_kick(vcrypto->ctrl_vq);
>>> > > +
>>> > > + /*
>>> > > +* Spin for a response, the kick causes an ioport write,
>>> > > trapping
>>> > > +* into the hypervisor, so the request should be handled
>>> > > immediately.
>>> > > +*/
I have my doubts about this comment (and about the code below too). Is
'kick causes an ioport write' true for every transport/architecture?
If we relay on this property maybe the documentation of notify should
mention it.
I know we have the same message in virtio-net.
>>> > > + while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
>>> > > + !virtqueue_is_broken(vcrypto->ctrl_vq))
>>> > > + cpu_relax();
> this spin under lock is kind of ugly.
> Why do we need to hold it while spinning?
> to prevent submitting more than one request?
> Isn't there a way to control this within crypto core?
>
> unlock
> relax
> lock
>
> would be better.
>
>>> > > +
>>> > > + if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) {
>>> > > + spin_unlock(&vcrypto->ctrl_lock);
>>> > > + pr_err("virtio_crypto: Create session failed status:
>>> > > %u\n",
>>> > > + le32_to_cpu(vcrypto->input.status));
>>> > > + kfree(cipher_key);
>>> > > + return -EINVAL;
>>> > > + }
>>> > > + spin_unlock(&vcrypto->ctrl_lock);
>>> > > +
> You drop lock here. If someone is trying to submit multiple
> requests, then the below will be racy as it might overwrite
> new result with previous data.
>
Was going to object on this too but Michael was faster.
Halil
>>> > > + spin_lock(&ctx->lock);
>>> > > + if (encrypt)
>>> > > + ctx->enc_sess_info.session_id =
>>> > > + le64_to_cpu(vcrypto->input.session_id);
>>> > > + else
>>> > > + ctx->dec_sess_info.session_id =
>>> > > + le64_to_cpu(vcrypto->input.session_id);
>>> > > + spin_unlock(&ctx->lock);
>>> > > +
>>> > > + kfree(cipher_key);
>>> > > + return 0;
>>> > > +}
>>> > > +
--
To unsubscribe from
