Virtualization - Sep 2023 - [PATCH] crypto: virtio-crypto: call finalize with bh disabled

Doing ipsec produces a spinlock recursion warning.
This is due to not disabling BH during crypto completion function.

Fixes: 59ca6c93387d3 ("virtio-crypto: implement RSA algorithm")
Reported-by: Halil Pasic <pasic at linux.ibm.com>
Signed-off-by: Gonglei <arei.gonglei at huawei.com>
---
 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 3 ++-
 drivers/crypto/virtio/virtio_crypto_skcipher_algs.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 2621ff8a9376..19e2898977d3 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -61,8 +61,9 @@ static void virtio_crypto_akcipher_finalize_req(
 	vc_akcipher_req->src_buf = NULL;
 	vc_akcipher_req->dst_buf = NULL;
 	virtcrypto_clear_request(&vc_akcipher_req->base);
-
+	local_bh_disable();
 	crypto_finalize_akcipher_request(vc_akcipher_req->base.dataq->engine,
req, err);
+	local_bh_enable();
 }
 
 static void virtio_crypto_dataq_akcipher_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 23c41d87d835..661c1102583e 100644
--- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
@@ -566,9 +566,10 @@ static void virtio_crypto_skcipher_finalize_req(
 					 AES_BLOCK_SIZE, 0);
 	kfree_sensitive(vc_sym_req->iv);
 	virtcrypto_clear_request(&vc_sym_req->base);
-
+	local_bh_disable();
 	crypto_finalize_skcipher_request(vc_sym_req->base.dataq->engine,
 					   req, err);
+	local_bh_enable();
 }
 
 static struct virtio_crypto_algo virtio_crypto_algs[] = { {
-- 
2.23.0

[..]
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -61,8 +61,9 @@ static void virtio_crypto_akcipher_finalize_req(
>  	vc_akcipher_req->src_buf = NULL;
>  	vc_akcipher_req->dst_buf = NULL;
>  	virtcrypto_clear_request(&vc_akcipher_req->base);
> -
> +	local_bh_disable();
>  
crypto_finalize_akcipher_request(vc_akcipher_req->base.dataq->engine, req,
err);
> +	local_bh_enable();
Thanks Gonglei!

I did this a quick spin, and it does not seem to be sufficient on s390x.
Which does not come as a surprise to me, because 

#define lockdep_assert_in_softirq()                                     \
do {                                                                    \
        WARN_ON_ONCE(__lockdep_enabled                  &&             
\
                     (!in_softirq() || in_irq() || in_nmi()));          \
} while (0)

will still warn because  in_irq() still evaluates to true (your patch
addresses the !in_softirq() part).

I don't have any results on x86 yet. My current understanding is that the
virtio-pci transport code disables interrupts locally somewhere in the
call chain (actually in vp_vring_interrupt() via spin_lock_irqsave())
and then x86 would be fine. But I will get that verified.

On the other hand virtio_airq_handler() calls vring_interrupt() with
interrupts enabled. (While vring_interrupt() is called in a (read)
critical section in virtio_airq_handler() we use read_lock() and
not read_lock_irqsave() to grab the lock. Whether that is correct in
it self (i.e. disregarding the crypto problem) or not I'm not sure right
now. Will think some more about it tomorrow.) If the way to go forward
is disabling interrupts in virtio-ccw before vring_interrupt() is
called, I would be glad to spin a patch for that.

Copying Conny, as she may have an opinion on this (if I'm not wrong she
authored that code).

Regards,
Halil