crypto virtio cleanups

PATCH In review Lukas Wunner 3 participants 8 messages Feb 3, 2025 – Feb 16, 2025

Subsystems (3)

Crypto API virtio core virtio crypto driver

lore

Share & tools

lore

Lukas Wunner

author

2025-02-03 13:39 UTC #0 lore raw

cover crypto virtio cleanups

Here's an assortment of trivial crypto virtio cleanups which I accumulated while working on commit 5b553e06b321 ("crypto: virtio - Drop sign/verify operations").

I've used qemu + libgcrypt backend to ascertain that all boot-time crypto selftests still pass after these changes. I've also verified that a KEYCTL_PKEY_ENCRYPT operation using virtio-pkcs1-rsa produces correct output.

Thanks!

Lukas Wunner (5):
crypto: virtio - Fix kernel-doc of virtcrypto_dev_stop()
crypto: virtio - Simplify RSA key size caching
crypto: virtio - Drop superfluous ctx->tfm backpointer
crypto: virtio - Drop superfluous [as]kcipher_ctx pointer
crypto: virtio - Drop superfluous [as]kcipher_req pointer

 .../virtio/virtio_crypto_akcipher_algs.c      | 41 ++++++++-----------
 drivers/crypto/virtio/virtio_crypto_mgr.c     |  2 +-
 .../virtio/virtio_crypto_skcipher_algs.c      | 17 ++------
 3 files changed, 21 insertions(+), 39 deletions(-)

Signature · 1 line

2.43.0

Lukas Wunner

author

2025-02-03 13:44 UTC #1 lore raw

PATCH 1/5 crypto: virtio - Fix kernel-doc of virtcrypto_dev_stop()

It seems the kernel-doc of virtcrypto_dev_start() was copied verbatim to virtcrypto_dev_stop(). Fix it.

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/crypto/virtio/virtio_crypto_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/crypto/virtio/virtio_crypto_mgr.c +1 -1

index 70e778aac0f2..bddbd8ebfebe 100644
@@ -256,7 +256,7 @@ int virtcrypto_dev_start(struct virtio_crypto *vcrypto)
  * @vcrypto:    Pointer to virtio crypto device.
  *
  * Function notifies all the registered services that the virtio crypto device
- * is ready to be used.
+ * shall no longer be used.
  * To be used by virtio crypto device specific drivers.
  *
  * Return: void

Signature · 1 line

2.43.0

Lukas Wunner

author

2025-02-03 13:47 UTC #2 lore raw

PATCH 2/5 crypto: virtio - Simplify RSA key size caching

In reply to Lukas Wunner

When setting a public or private RSA key, the integer n is cached in the transform context virtio_crypto_akcipher_ctx -- with the sole purpose of calculating the key size from it in virtio_crypto_rsa_max_size(). It looks like this was copy-pasted from crypto/rsa.c.

Cache the key size directly instead of the integer n, thus simplifying the code and reducing the memory footprint.

Signed-off-by Lukas Wunner <[email protected]>

---
 .../virtio/virtio_crypto_akcipher_algs.c      | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

drivers/crypto/virtio/virtio_crypto_akcipher_algs.c +8 ~~-11~~

index 48fee07b7e51..7fdf32c79909 100644
@@ -21,7 +21,7 @@
 #include "virtio_crypto_common.h"
 
 struct virtio_crypto_rsa_ctx {
-	MPI n;
+	unsigned int key_size;
 };
 
 struct virtio_crypto_akcipher_ctx {

@@ -352,10 +352,7 @@ static int virtio_crypto_rsa_set_key(struct crypto_akcipher *tfm,
 	int node = virtio_crypto_get_current_node();
 	uint32_t keytype;
 	int ret;
-
-	/* mpi_free will test n, just free it. */
-	mpi_free(rsa_ctx->n);
-	rsa_ctx->n = NULL;
+	MPI n;
 
 	if (private) {
 		keytype = VIRTIO_CRYPTO_AKCIPHER_KEY_TYPE_PRIVATE;

@@ -368,10 +365,13 @@ static int virtio_crypto_rsa_set_key(struct crypto_akcipher *tfm,
 	if (ret)
 		return ret;
 
-	rsa_ctx->n = mpi_read_raw_data(rsa_key.n, rsa_key.n_sz);
-	if (!rsa_ctx->n)
+	n = mpi_read_raw_data(rsa_key.n, rsa_key.n_sz);
+	if (!n)
 		return -ENOMEM;
 
+	rsa_ctx->key_size = mpi_get_size(n);
+	mpi_free(n);
+
 	if (!ctx->vcrypto) {
 		vcrypto = virtcrypto_get_dev_node(node, VIRTIO_CRYPTO_SERVICE_AKCIPHER,
 						VIRTIO_CRYPTO_AKCIPHER_RSA);

@@ -442,7 +442,7 @@ static unsigned int virtio_crypto_rsa_max_size(struct crypto_akcipher *tfm)
 	struct virtio_crypto_akcipher_ctx *ctx = akcipher_tfm_ctx(tfm);
 	struct virtio_crypto_rsa_ctx *rsa_ctx = &ctx->rsa_ctx;
 
-	return mpi_get_size(rsa_ctx->n);
+	return rsa_ctx->key_size;
 }
 
 static int virtio_crypto_rsa_init_tfm(struct crypto_akcipher *tfm)

@@ -460,12 +460,9 @@ static int virtio_crypto_rsa_init_tfm(struct crypto_akcipher *tfm)
 static void virtio_crypto_rsa_exit_tfm(struct crypto_akcipher *tfm)
 {
 	struct virtio_crypto_akcipher_ctx *ctx = akcipher_tfm_ctx(tfm);
-	struct virtio_crypto_rsa_ctx *rsa_ctx = &ctx->rsa_ctx;
 
 	virtio_crypto_alg_akcipher_close_session(ctx);
 	virtcrypto_dev_put(ctx->vcrypto);
-	mpi_free(rsa_ctx->n);
-	rsa_ctx->n = NULL;
 }
 
 static struct virtio_crypto_akcipher_algo virtio_crypto_akcipher_algs[] = {

Signature · 1 line

2.43.0

Lukas Wunner

author

2025-02-03 13:48 UTC #3 lore raw

PATCH 3/5 crypto: virtio - Drop superfluous ctx->tfm backpointer

In reply to Lukas Wunner

struct virtio_crypto_[as]kcipher_ctx contains a backpointer to struct crypto_[as]kcipher which is superfluous in two ways:

First, it's not used anywhere. Second, the context is embedded into struct crypto_tfm, so one could just use container_of() to get from the context to crypto_tfm and from there to crypto_[as]kcipher.

Drop the superfluous backpointer.

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 5 -----
 drivers/crypto/virtio/virtio_crypto_skcipher_algs.c | 4 ----
 2 files changed, 9 deletions(-)

drivers/crypto/virtio/virtio_crypto_akcipher_algs.c +0 -5

index 7fdf32c79909..aa8255786d6c 100644
@@ -26,7 +26,6 @@ struct virtio_crypto_rsa_ctx {
 
 struct virtio_crypto_akcipher_ctx {
 	struct virtio_crypto *vcrypto;
-	struct crypto_akcipher *tfm;
 	bool session_valid;
 	__u64 session_id;
 	union {

@@ -447,10 +446,6 @@ static unsigned int virtio_crypto_rsa_max_size(struct crypto_akcipher *tfm)
 
 static int virtio_crypto_rsa_init_tfm(struct crypto_akcipher *tfm)
 {
-	struct virtio_crypto_akcipher_ctx *ctx = akcipher_tfm_ctx(tfm);
-
-	ctx->tfm = tfm;
-
 	akcipher_set_reqsize(tfm,
 			     sizeof(struct virtio_crypto_akcipher_request));

drivers/crypto/virtio/virtio_crypto_skcipher_algs.c +0 -4

index 23c41d87d835..495fc655a51c 100644
@@ -17,7 +17,6 @@
 
 struct virtio_crypto_skcipher_ctx {
 	struct virtio_crypto *vcrypto;
-	struct crypto_skcipher *tfm;
 
 	struct virtio_crypto_sym_session_info enc_sess_info;
 	struct virtio_crypto_sym_session_info dec_sess_info;

@@ -515,10 +514,7 @@ static int virtio_crypto_skcipher_decrypt(struct skcipher_request *req)
 
 static int virtio_crypto_skcipher_init(struct crypto_skcipher *tfm)
 {
-	struct virtio_crypto_skcipher_ctx *ctx = crypto_skcipher_ctx(tfm);
-
 	crypto_skcipher_set_reqsize(tfm, sizeof(struct virtio_crypto_sym_request));
-	ctx->tfm = tfm;
 
 	return 0;
 }

Signature · 1 line

2.43.0

Lukas Wunner

author

2025-02-03 13:50 UTC #4 lore raw

PATCH 4/5 crypto: virtio - Drop superfluous [as]kcipher_ctx pointer

In reply to Lukas Wunner

The request context virtio_crypto_{akcipher,sym}_request contains a pointer to the transform context virtio_crypto_[as]kcipher_ctx.

The pointer is superfluous as it can be calculated with the cheap crypto_akcipher_reqtfm() + akcipher_tfm_ctx() and crypto_skcipher_reqtfm() + crypto_skcipher_ctx() combos.

Drop the superfluous pointer.

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 8 ++++----
 drivers/crypto/virtio/virtio_crypto_skcipher_algs.c | 5 +----
 2 files changed, 5 insertions(+), 8 deletions(-)

drivers/crypto/virtio/virtio_crypto_akcipher_algs.c +4 -4

index aa8255786d6c..ac8eb3d07c93 100644
@@ -35,7 +35,6 @@ struct virtio_crypto_akcipher_ctx {
 
 struct virtio_crypto_akcipher_request {
 	struct virtio_crypto_request base;
-	struct virtio_crypto_akcipher_ctx *akcipher_ctx;
 	struct akcipher_request *akcipher_req;
 	void *src_buf;
 	void *dst_buf;

@@ -212,7 +211,8 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
 static int __virtio_crypto_akcipher_do_req(struct virtio_crypto_akcipher_request *vc_akcipher_req,
 		struct akcipher_request *req, struct data_queue *data_vq)
 {
-	struct virtio_crypto_akcipher_ctx *ctx = vc_akcipher_req->akcipher_ctx;
+	struct crypto_akcipher *atfm = crypto_akcipher_reqtfm(req);
+	struct virtio_crypto_akcipher_ctx *ctx = akcipher_tfm_ctx(atfm);
 	struct virtio_crypto_request *vc_req = &vc_akcipher_req->base;
 	struct virtio_crypto *vcrypto = ctx->vcrypto;
 	struct virtio_crypto_op_data_req *req_data = vc_req->req_data;

@@ -272,7 +272,8 @@ static int virtio_crypto_rsa_do_req(struct crypto_engine *engine, void *vreq)
 	struct akcipher_request *req = container_of(vreq, struct akcipher_request, base);
 	struct virtio_crypto_akcipher_request *vc_akcipher_req = akcipher_request_ctx(req);
 	struct virtio_crypto_request *vc_req = &vc_akcipher_req->base;
-	struct virtio_crypto_akcipher_ctx *ctx = vc_akcipher_req->akcipher_ctx;
+	struct crypto_akcipher *atfm = crypto_akcipher_reqtfm(req);
+	struct virtio_crypto_akcipher_ctx *ctx = akcipher_tfm_ctx(atfm);
 	struct virtio_crypto *vcrypto = ctx->vcrypto;
 	struct data_queue *data_vq = vc_req->dataq;
 	struct virtio_crypto_op_header *header;

@@ -318,7 +319,6 @@ static int virtio_crypto_rsa_req(struct akcipher_request *req, uint32_t opcode)
 
 	vc_req->dataq = data_vq;
 	vc_req->alg_cb = virtio_crypto_dataq_akcipher_callback;
-	vc_akcipher_req->akcipher_ctx = ctx;
 	vc_akcipher_req->akcipher_req = req;
 	vc_akcipher_req->opcode = opcode;

drivers/crypto/virtio/virtio_crypto_skcipher_algs.c +1 -4

index 495fc655a51c..e2a481a29b77 100644
@@ -27,7 +27,6 @@ struct virtio_crypto_sym_request {
 
 	/* Cipher or aead */
 	uint32_t type;
-	struct virtio_crypto_skcipher_ctx *skcipher_ctx;
 	struct skcipher_request *skcipher_req;
 	uint8_t *iv;
 	/* Encryption? */

@@ -324,7 +323,7 @@ __virtio_crypto_skcipher_do_req(struct virtio_crypto_sym_request *vc_sym_req,
 		struct data_queue *data_vq)
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
-	struct virtio_crypto_skcipher_ctx *ctx = vc_sym_req->skcipher_ctx;
+	struct virtio_crypto_skcipher_ctx *ctx = crypto_skcipher_ctx(tfm);
 	struct virtio_crypto_request *vc_req = &vc_sym_req->base;
 	unsigned int ivsize = crypto_skcipher_ivsize(tfm);
 	struct virtio_crypto *vcrypto = ctx->vcrypto;

@@ -480,7 +479,6 @@ static int virtio_crypto_skcipher_encrypt(struct skcipher_request *req)
 
 	vc_req->dataq = data_vq;
 	vc_req->alg_cb = virtio_crypto_dataq_sym_callback;
-	vc_sym_req->skcipher_ctx = ctx;
 	vc_sym_req->skcipher_req = req;
 	vc_sym_req->encrypt = true;

@@ -505,7 +503,6 @@ static int virtio_crypto_skcipher_decrypt(struct skcipher_request *req)
 
 	vc_req->dataq = data_vq;
 	vc_req->alg_cb = virtio_crypto_dataq_sym_callback;
-	vc_sym_req->skcipher_ctx = ctx;
 	vc_sym_req->skcipher_req = req;
 	vc_sym_req->encrypt = false;

Signature · 1 line

2.43.0

Lukas Wunner

author

2025-02-03 13:53 UTC #5 lore raw

PATCH 5/5 crypto: virtio - Drop superfluous [as]kcipher_req pointer

In reply to Lukas Wunner

The request context virtio_crypto_{akcipher,sym}_request contains a pointer to the [as]kcipher_request itself.

The pointer is superfluous as it can be calculated with container_of().

Drop the superfluous pointer.

Signed-off-by Lukas Wunner <[email protected]>

---
I've considered introducing a static inline to <crypto/[as]kcipher.h> to get from the request context to the request, but these two seem to be the only occurrences in the tree which would need it, so I figured it's probably not worth it. If anyone disagrees, please speak up.

 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 9 ++++-----
 drivers/crypto/virtio/virtio_crypto_skcipher_algs.c | 8 +++-----
 2 files changed, 7 insertions(+), 10 deletions(-)

drivers/crypto/virtio/virtio_crypto_akcipher_algs.c +4 -5

index ac8eb3d07c93..2e44915c9f23 100644
@@ -35,7 +35,6 @@ struct virtio_crypto_akcipher_ctx {
 
 struct virtio_crypto_akcipher_request {
 	struct virtio_crypto_request base;
-	struct akcipher_request *akcipher_req;
 	void *src_buf;
 	void *dst_buf;
 	uint32_t opcode;

@@ -67,7 +66,9 @@ static void virtio_crypto_dataq_akcipher_callback(struct virtio_crypto_request *
 {
 	struct virtio_crypto_akcipher_request *vc_akcipher_req =
 		container_of(vc_req, struct virtio_crypto_akcipher_request, base);
-	struct akcipher_request *akcipher_req;
+	struct akcipher_request *akcipher_req =
+		container_of((void *)vc_akcipher_req, struct akcipher_request,
+			     __ctx);
 	int error;
 
 	switch (vc_req->status) {

@@ -86,8 +87,7 @@ static void virtio_crypto_dataq_akcipher_callback(struct virtio_crypto_request *
 		break;
 	}
 
-	akcipher_req = vc_akcipher_req->akcipher_req;
-	/* actual length maybe less than dst buffer */
+	/* actual length may be less than dst buffer */
 	akcipher_req->dst_len = len - sizeof(vc_req->status);
 	sg_copy_from_buffer(akcipher_req->dst, sg_nents(akcipher_req->dst),
 			    vc_akcipher_req->dst_buf, akcipher_req->dst_len);

@@ -319,7 +319,6 @@ static int virtio_crypto_rsa_req(struct akcipher_request *req, uint32_t opcode)
 
 	vc_req->dataq = data_vq;
 	vc_req->alg_cb = virtio_crypto_dataq_akcipher_callback;
-	vc_akcipher_req->akcipher_req = req;
 	vc_akcipher_req->opcode = opcode;
 
 	return crypto_transfer_akcipher_request_to_engine(data_vq->engine, req);

drivers/crypto/virtio/virtio_crypto_skcipher_algs.c +3 -5

index e2a481a29b77..1b3fb21a2a7d 100644
@@ -27,7 +27,6 @@ struct virtio_crypto_sym_request {
 
 	/* Cipher or aead */
 	uint32_t type;
-	struct skcipher_request *skcipher_req;
 	uint8_t *iv;
 	/* Encryption? */
 	bool encrypt;

@@ -55,7 +54,9 @@ static void virtio_crypto_dataq_sym_callback
 {
 	struct virtio_crypto_sym_request *vc_sym_req =
 		container_of(vc_req, struct virtio_crypto_sym_request, base);
-	struct skcipher_request *ablk_req;
+	struct skcipher_request *ablk_req =
+		container_of((void *)vc_sym_req, struct skcipher_request,
+			     __ctx);
 	int error;
 
 	/* Finish the encrypt or decrypt process */

@@ -75,7 +76,6 @@ static void virtio_crypto_dataq_sym_callback
 			error = -EIO;
 			break;
 		}
-		ablk_req = vc_sym_req->skcipher_req;
 		virtio_crypto_skcipher_finalize_req(vc_sym_req,
 							ablk_req, error);
 	}

@@ -479,7 +479,6 @@ static int virtio_crypto_skcipher_encrypt(struct skcipher_request *req)
 
 	vc_req->dataq = data_vq;
 	vc_req->alg_cb = virtio_crypto_dataq_sym_callback;
-	vc_sym_req->skcipher_req = req;
 	vc_sym_req->encrypt = true;
 
 	return crypto_transfer_skcipher_request_to_engine(data_vq->engine, req);

@@ -503,7 +502,6 @@ static int virtio_crypto_skcipher_decrypt(struct skcipher_request *req)
 
 	vc_req->dataq = data_vq;
 	vc_req->alg_cb = virtio_crypto_dataq_sym_callback;
-	vc_sym_req->skcipher_req = req;
 	vc_sym_req->encrypt = false;
 
 	return crypto_transfer_skcipher_request_to_engine(data_vq->engine, req);

Signature · 1 line

2.43.0

zhenwei pi

Reviewed-by on the v1 series

2025-02-05 07:52 UTC #6 lore raw

In reply to Lukas Wunner

This series looks good to me, thanks!

Reviewed-by zhenwei pi <[email protected]>

Quoted 23 lines · Lukas Wunner

Here's an assortment of trivial crypto virtio cleanups which I accumulated while working on commit 5b553e06b321 ("crypto: virtio - Drop sign/verify operations").
I've used qemu + libgcrypt backend to ascertain that all boot-time crypto selftests still pass after these changes. I've also verified that a KEYCTL_PKEY_ENCRYPT operation using virtio-pkcs1-rsa produces correct output.
Thanks!
Lukas Wunner (5):
crypto: virtio - Fix kernel-doc of virtcrypto_dev_stop()
crypto: virtio - Simplify RSA key size caching
crypto: virtio - Drop superfluous ctx->tfm backpointer
crypto: virtio - Drop superfluous [as]kcipher_ctx pointer
crypto: virtio - Drop superfluous [as]kcipher_req pointer
  .../virtio/virtio_crypto_akcipher_algs.c      | 41 ++++++++-----------
  drivers/crypto/virtio/virtio_crypto_mgr.c     |  2 +-
  .../virtio/virtio_crypto_skcipher_algs.c      | 17 ++------
  3 files changed, 21 insertions(+), 39 deletions(-)

Herbert Xu

maintainer

2025-02-16 05:25 UTC #7 lore raw

In reply to Lukas Wunner

Quoted 18 lines · Lukas Wunner

Here's an assortment of trivial crypto virtio cleanups which I accumulated while working on commit 5b553e06b321 ("crypto: virtio - Drop sign/verify operations").
I've used qemu + libgcrypt backend to ascertain that all boot-time crypto selftests still pass after these changes. I've also verified that a KEYCTL_PKEY_ENCRYPT operation using virtio-pkcs1-rsa produces correct output.
Thanks!
Lukas Wunner (5):
crypto: virtio - Fix kernel-doc of virtcrypto_dev_stop()
crypto: virtio - Simplify RSA key size caching
crypto: virtio - Drop superfluous ctx->tfm backpointer
crypto: virtio - Drop superfluous [as]kcipher_ctx pointer
crypto: virtio - Drop superfluous [as]kcipher_req pointer

Quoted 7 lines

 .../virtio/virtio_crypto_akcipher_algs.c      | 41 ++++++++-----------
 drivers/crypto/virtio/virtio_crypto_mgr.c     |  2 +-
 .../virtio/virtio_crypto_skcipher_algs.c      | 17 ++------
 3 files changed, 21 insertions(+), 39 deletions(-)

Signature · 1 line

2.43.0

All applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt