Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752332AbcK1RhY (ORCPT ); Mon, 28 Nov 2016 12:37:24 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:58076 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751373AbcK1RhO (ORCPT ); Mon, 28 Nov 2016 12:37:14 -0500 Subject: Re: [PATCH v3] crypto: add virtio-crypto driver To: Gonglei References: <1480334903-6672-1-git-send-email-arei.gonglei@huawei.com> <1480334903-6672-2-git-send-email-arei.gonglei@huawei.com> <20161128162055.GB11196@stefanha-x1.localdomain> <20161128185747-mutt-send-email-mst@kernel.org> Cc: "Michael S. Tsirkin" , Stefan Hajnoczi , linux-kernel@vger.kernel.org, qemu-devel@nongnu.org, virtio-dev@lists.oasis-open.org, virtualization@lists.linux-foundation.org, linux-crypto@vger.kernel.org, luonengjun@huawei.com, weidong.huang@huawei.com, wu.wubin@huawei.com, xin.zeng@intel.com, claudio.fontana@huawei.com, herbert@gondor.apana.org.au, davem@davemloft.net, jianjay.zhou@huawei.com, hanweidong@huawei.com, arei.gonglei@hotmail.com, cornelia.huck@de.ibm.com, xuquan8@huawei.com, longpeng2@huawei.com, salvatore.benedetto@intel.com From: Halil Pasic Date: Mon, 28 Nov 2016 18:37:01 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161128185747-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16112817-0008-0000-0000-000003B31931 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16112817-0009-0000-0000-00001B957C54 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-11-28_14:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1611280287 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4281 Lines: 124 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; >>> > > +} >>> > > +