> -----Original Message-----
> From: zhenwei pi [mailto:[email protected]]
> Sent: Thursday, May 5, 2022 5:24 PM
> To: Gonglei (Arei) <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; kernel test robot
> <[email protected]>; Dan Carpenter <[email protected]>
> Subject: [PATCH v5 2/5] virtio-crypto: use private buffer for control request
>
> Originally, all of the control requests share a single buffer( ctrl & input &
> ctrl_status fields in struct virtio_crypto), this allows queue depth 1 only, the
> performance of control queue gets limited by this design.
>
> In this patch, each request allocates request buffer dynamically, and free buffer
> after request, so the scope protected by ctrl_lock also get optimized here.
> It's possible to optimize control queue depth in the next step.
>
> A necessary comment is already in code, still describe it again:
> /*
> * Note: there are padding fields in request, clear them to zero before
> * sending to host to avoid to divulge any information.
> * Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48]
> */
> So use kzalloc to allocate buffer of struct virtio_crypto_ctrl_request.
>
> Potentially dereferencing uninitialized variables:
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> Cc: Michael S. Tsirkin <[email protected]>
> Cc: Jason Wang <[email protected]>
> Cc: Gonglei <[email protected]>
> Signed-off-by: zhenwei pi <[email protected]>
> ---
> .../virtio/virtio_crypto_akcipher_algs.c | 57 ++++++++++++-------
> drivers/crypto/virtio/virtio_crypto_common.h | 17 ++++--
> .../virtio/virtio_crypto_skcipher_algs.c | 50 ++++++++++------
> 3 files changed, 79 insertions(+), 45 deletions(-)
>
Reviewed-by: Gonglei <[email protected]>
Regards,
-Gonglei
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index 20901a263fc8..698ea57e2649 100644
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -108,16 +108,22 @@ static int
> virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
> unsigned int num_out = 0, num_in = 0;
> struct virtio_crypto_op_ctrl_req *ctrl;
> struct virtio_crypto_session_input *input;
> + struct virtio_crypto_ctrl_request *vc_ctrl_req;
>
> pkey = kmemdup(key, keylen, GFP_ATOMIC);
> if (!pkey)
> return -ENOMEM;
>
> - spin_lock(&vcrypto->ctrl_lock);
> - ctrl = &vcrypto->ctrl;
> + vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
> + if (!vc_ctrl_req) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + ctrl = &vc_ctrl_req->ctrl;
> memcpy(&ctrl->header, header, sizeof(ctrl->header));
> memcpy(&ctrl->u, para, sizeof(ctrl->u));
> - input = &vcrypto->input;
> + input = &vc_ctrl_req->input;
> input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>
> sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl)); @@ -129,16 +135,22 @@
> static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
> sg_init_one(&inhdr_sg, input, sizeof(*input));
> sgs[num_out + num_in++] = &inhdr_sg;
>
> + spin_lock(&vcrypto->ctrl_lock);
> err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto,
> GFP_ATOMIC);
> - if (err < 0)
> + if (err < 0) {
> + spin_unlock(&vcrypto->ctrl_lock);
> goto out;
> + }
>
> virtqueue_kick(vcrypto->ctrl_vq);
> while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
> !virtqueue_is_broken(vcrypto->ctrl_vq))
> cpu_relax();
> + spin_unlock(&vcrypto->ctrl_lock);
>
> if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
> + pr_err("virtio_crypto: Create session failed status: %u\n",
> + le32_to_cpu(input->status));
> err = -EINVAL;
> goto out;
> }
> @@ -148,13 +160,9 @@ static int
> virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
> err = 0;
>
> out:
> - spin_unlock(&vcrypto->ctrl_lock);
> + kfree(vc_ctrl_req);
> kfree_sensitive(pkey);
>
> - if (err < 0)
> - pr_err("virtio_crypto: Create session failed status: %u\n",
> - le32_to_cpu(input->status));
> -
> return err;
> }
>
> @@ -167,15 +175,18 @@ static int
> virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
> int err;
> struct virtio_crypto_op_ctrl_req *ctrl;
> struct virtio_crypto_inhdr *ctrl_status;
> + struct virtio_crypto_ctrl_request *vc_ctrl_req;
>
> - spin_lock(&vcrypto->ctrl_lock);
> - if (!ctx->session_valid) {
> - err = 0;
> - goto out;
> - }
> - ctrl_status = &vcrypto->ctrl_status;
> + if (!ctx->session_valid)
> + return 0;
> +
> + vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
> + if (!vc_ctrl_req)
> + return -ENOMEM;
> +
> + ctrl_status = &vc_ctrl_req->ctrl_status;
> ctrl_status->status = VIRTIO_CRYPTO_ERR;
> - ctrl = &vcrypto->ctrl;
> + ctrl = &vc_ctrl_req->ctrl;
> ctrl->header.opcode =
> cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
> ctrl->header.queue_id = 0;
>
> @@ -188,16 +199,22 @@ static int
> virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
> sg_init_one(&inhdr_sg, &ctrl_status->status, sizeof(ctrl_status->status));
> sgs[num_out + num_in++] = &inhdr_sg;
>
> + spin_lock(&vcrypto->ctrl_lock);
> err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto,
> GFP_ATOMIC);
> - if (err < 0)
> + if (err < 0) {
> + spin_unlock(&vcrypto->ctrl_lock);
> goto out;
> + }
>
> virtqueue_kick(vcrypto->ctrl_vq);
> while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
> !virtqueue_is_broken(vcrypto->ctrl_vq))
> cpu_relax();
> + spin_unlock(&vcrypto->ctrl_lock);
>
> if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
> + pr_err("virtio_crypto: Close session failed status: %u, session_id:
> 0x%llx\n",
> + ctrl_status->status, destroy_session->session_id);
> err = -EINVAL;
> goto out;
> }
> @@ -206,11 +223,7 @@ static int
> virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
> ctx->session_valid = false;
>
> out:
> - spin_unlock(&vcrypto->ctrl_lock);
> - if (err < 0) {
> - pr_err("virtio_crypto: Close session failed status: %u, session_id:
> 0x%llx\n",
> - ctrl_status->status, destroy_session->session_id);
> - }
> + kfree(vc_ctrl_req);
>
> return err;
> }
> diff --git a/drivers/crypto/virtio/virtio_crypto_common.h
> b/drivers/crypto/virtio/virtio_crypto_common.h
> index e693d4ee83a6..2422237ec4e6 100644
> --- a/drivers/crypto/virtio/virtio_crypto_common.h
> +++ b/drivers/crypto/virtio/virtio_crypto_common.h
> @@ -13,6 +13,7 @@
> #include <crypto/aead.h>
> #include <crypto/aes.h>
> #include <crypto/engine.h>
> +#include <uapi/linux/virtio_crypto.h>
>
>
> /* Internal representation of a data virtqueue */ @@ -65,11 +66,6 @@ struct
> virtio_crypto {
> /* Maximum size of per request */
> u64 max_size;
>
> - /* Control VQ buffers: protected by the ctrl_lock */
> - struct virtio_crypto_op_ctrl_req ctrl;
> - struct virtio_crypto_session_input input;
> - struct virtio_crypto_inhdr ctrl_status;
> -
> unsigned long status;
> atomic_t ref_count;
> struct list_head list;
> @@ -85,6 +81,17 @@ struct virtio_crypto_sym_session_info {
> __u64 session_id;
> };
>
> +/*
> + * Note: there are padding fields in request, clear them to zero before
> + * sending to host to avoid to divulge any information.
> + * Ex,
> +virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48]
> + */
> +struct virtio_crypto_ctrl_request {
> + struct virtio_crypto_op_ctrl_req ctrl;
> + struct virtio_crypto_session_input input;
> + struct virtio_crypto_inhdr ctrl_status; };
> +
> struct virtio_crypto_request;
> typedef void (*virtio_crypto_data_callback)
> (struct virtio_crypto_request *vc_req, int len); diff --git
> a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> index e3c5bc8d6112..6aaf0869b211 100644
> --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> @@ -126,6 +126,7 @@ static int virtio_crypto_alg_skcipher_init_session(
> struct virtio_crypto_op_ctrl_req *ctrl;
> struct virtio_crypto_session_input *input;
> struct virtio_crypto_sym_create_session_req *sym_create_session;
> + struct virtio_crypto_ctrl_request *vc_ctrl_req;
>
> /*
> * Avoid to do DMA from the stack, switch to using @@ -136,15 +137,20
> @@ static int virtio_crypto_alg_skcipher_init_session(
> if (!cipher_key)
> return -ENOMEM;
>
> - spin_lock(&vcrypto->ctrl_lock);
> + vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
> + if (!vc_ctrl_req) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> /* Pad ctrl header */
> - ctrl = &vcrypto->ctrl;
> + ctrl = &vc_ctrl_req->ctrl;
> ctrl->header.opcode =
> cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
> ctrl->header.algo = cpu_to_le32(alg);
> /* Set the default dataqueue id to 0 */
> ctrl->header.queue_id = 0;
>
> - input = &vcrypto->input;
> + input = &vc_ctrl_req->input;
> input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
> /* Pad cipher's parameters */
> sym_create_session = &ctrl->u.sym_create_session; @@ -164,12
> +170,12 @@ static int virtio_crypto_alg_skcipher_init_session(
> sg_init_one(&inhdr, input, sizeof(*input));
> sgs[num_out + num_in++] = &inhdr;
>
> + spin_lock(&vcrypto->ctrl_lock);
> err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
> num_in, vcrypto, GFP_ATOMIC);
> if (err < 0) {
> spin_unlock(&vcrypto->ctrl_lock);
> - kfree_sensitive(cipher_key);
> - return err;
> + goto out;
> }
> virtqueue_kick(vcrypto->ctrl_vq);
>
> @@ -180,13 +186,13 @@ static int virtio_crypto_alg_skcipher_init_session(
> while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
> !virtqueue_is_broken(vcrypto->ctrl_vq))
> cpu_relax();
> + spin_unlock(&vcrypto->ctrl_lock);
>
> if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
> - spin_unlock(&vcrypto->ctrl_lock);
> pr_err("virtio_crypto: Create session failed status: %u\n",
> le32_to_cpu(input->status));
> - kfree_sensitive(cipher_key);
> - return -EINVAL;
> + err = -EINVAL;
> + goto out;
> }
>
> if (encrypt)
> @@ -194,10 +200,11 @@ static int virtio_crypto_alg_skcipher_init_session(
> else
> ctx->dec_sess_info.session_id = le64_to_cpu(input->session_id);
>
> - spin_unlock(&vcrypto->ctrl_lock);
> -
> + err = 0;
> +out:
> + kfree(vc_ctrl_req);
> kfree_sensitive(cipher_key);
> - return 0;
> + return err;
> }
>
> static int virtio_crypto_alg_skcipher_close_session(
> @@ -212,12 +219,16 @@ static int virtio_crypto_alg_skcipher_close_session(
> unsigned int num_out = 0, num_in = 0;
> struct virtio_crypto_op_ctrl_req *ctrl;
> struct virtio_crypto_inhdr *ctrl_status;
> + struct virtio_crypto_ctrl_request *vc_ctrl_req;
>
> - spin_lock(&vcrypto->ctrl_lock);
> - ctrl_status = &vcrypto->ctrl_status;
> + vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
> + if (!vc_ctrl_req)
> + return -ENOMEM;
> +
> + ctrl_status = &vc_ctrl_req->ctrl_status;
> ctrl_status->status = VIRTIO_CRYPTO_ERR;
> /* Pad ctrl header */
> - ctrl = &vcrypto->ctrl;
> + ctrl = &vc_ctrl_req->ctrl;
> ctrl->header.opcode =
> cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION);
> /* Set the default virtqueue id to 0 */
> ctrl->header.queue_id = 0;
> @@ -236,28 +247,31 @@ static int virtio_crypto_alg_skcipher_close_session(
> sg_init_one(&status_sg, &ctrl_status->status,
> sizeof(ctrl_status->status));
> sgs[num_out + num_in++] = &status_sg;
>
> + spin_lock(&vcrypto->ctrl_lock);
> err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
> num_in, vcrypto, GFP_ATOMIC);
> if (err < 0) {
> spin_unlock(&vcrypto->ctrl_lock);
> - return err;
> + goto out;
> }
> virtqueue_kick(vcrypto->ctrl_vq);
>
> while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
> !virtqueue_is_broken(vcrypto->ctrl_vq))
> cpu_relax();
> + spin_unlock(&vcrypto->ctrl_lock);
>
> if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
> - spin_unlock(&vcrypto->ctrl_lock);
> pr_err("virtio_crypto: Close session failed status: %u, session_id:
> 0x%llx\n",
> ctrl_status->status, destroy_session->session_id);
>
> return -EINVAL;
> }
> - spin_unlock(&vcrypto->ctrl_lock);
>
> - return 0;
> + err = 0;
> +out:
> + kfree(vc_ctrl_req);
> + return err;
> }
>
> static int virtio_crypto_alg_skcipher_init_sessions(
> --
> 2.20.1