On Mon, Apr 07, 2025 at 07:16:38PM -0400, Sean Anderson wrote:
>
> [    2.731344] refcount_t: decrement hit 0; leaking memory.

...

> [    2.731496] caam_rsp_fq_dqrr_cb (include/linux/refcount.h:336 include/linux/refcount.h:351 drivers/crypto/caam/qi.c:593) 
> [    2.731502] qman_p_poll_dqrr (drivers/soc/fsl/qbman/qman.c:1652 drivers/soc/fsl/qbman/qman.c:1759) 
> [    2.731510] caam_qi_poll (drivers/crypto/caam/qi.c:491) 
> [    2.731514] __napi_poll (net/core/dev.c:7328) 
> [    2.731520] net_rx_action (net/core/dev.c:7394 net/core/dev.c:7514) 
> [    2.731524] handle_softirqs (arch/arm64/include/asm/jump_label.h:36 include/trace/events/irq.h:142 kernel/softirq.c:562) 
> [    2.731530] __do_softirq (kernel/softirq.c:596) 
> [    2.731533] ____do_softirq (arch/arm64/kernel/irq.c:82) 
> [    2.731538] call_on_irq_stack (arch/arm64/kernel/entry.S:897) 
> [    2.731542] do_softirq_own_stack (arch/arm64/kernel/irq.c:87) 
> [    2.731547] __irq_exit_rcu (kernel/softirq.c:442 kernel/softirq.c:662) 
> [    2.731550] irq_exit_rcu (kernel/softirq.c:681) 
> [    2.731554] el1_interrupt (arch/arm64/kernel/entry-common.c:565 arch/arm64/kernel/entry-common.c:575) 
> [    2.731561] el1h_64_irq_handler (arch/arm64/kernel/entry-common.c:581) 
> [    2.731567] el1h_64_irq (arch/arm64/kernel/entry.S:596) 
> [    2.731570] qman_enqueue (drivers/soc/fsl/qbman/qman.c:2354) (P)
> [    2.731576] caam_qi_enqueue (drivers/crypto/caam/qi.c:125) 

So caam_qi_enqueue hasn't had a chance to increment the refcount
and the IRQ already came in to decrement it.  Lesson is that you
should always increment your refcount before you give it away.

---8<---
Ensure refcount is raised before request is enqueued since it could
be dequeued before the call returns.

Reported-by: Sean Anderson <sean.anderson@linux.dev>
Cc: <stable@vger.kernel.org>
Fixes: 11144416a755 ("crypto: caam/qi - optimize frame queue cleanup")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/caam/qi.c b/drivers/crypto/caam/qi.c
index 7701d00bcb3a..b6e7c0b29d4e 100644
--- a/drivers/crypto/caam/qi.c
+++ b/drivers/crypto/caam/qi.c
@@ -122,12 +122,12 @@ int caam_qi_enqueue(struct device *qidev, struct caam_drv_req *req)
 	qm_fd_addr_set64(&fd, addr);
 
 	do {
+		refcount_inc(&req->drv_ctx->refcnt);
 		ret = qman_enqueue(req->drv_ctx->req_fq, &fd);
-		if (likely(!ret)) {
-			refcount_inc(&req->drv_ctx->refcnt);
+		if (likely(!ret))
 			return 0;
-		}
 
+		refcount_dec(&req->drv_ctx->refcnt);
 		if (ret != -EBUSY)
 			break;
 		num_retries++;
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On 4/8/25 01:17, Herbert Xu wrote:
> On Mon, Apr 07, 2025 at 07:16:38PM -0400, Sean Anderson wrote:
>>
>> [    2.731344] refcount_t: decrement hit 0; leaking memory.
> 
> ...
> 
>> [    2.731496] caam_rsp_fq_dqrr_cb (include/linux/refcount.h:336 include/linux/refcount.h:351 drivers/crypto/caam/qi.c:593) 
>> [    2.731502] qman_p_poll_dqrr (drivers/soc/fsl/qbman/qman.c:1652 drivers/soc/fsl/qbman/qman.c:1759) 
>> [    2.731510] caam_qi_poll (drivers/crypto/caam/qi.c:491) 
>> [    2.731514] __napi_poll (net/core/dev.c:7328) 
>> [    2.731520] net_rx_action (net/core/dev.c:7394 net/core/dev.c:7514) 
>> [    2.731524] handle_softirqs (arch/arm64/include/asm/jump_label.h:36 include/trace/events/irq.h:142 kernel/softirq.c:562) 
>> [    2.731530] __do_softirq (kernel/softirq.c:596) 
>> [    2.731533] ____do_softirq (arch/arm64/kernel/irq.c:82) 
>> [    2.731538] call_on_irq_stack (arch/arm64/kernel/entry.S:897) 
>> [    2.731542] do_softirq_own_stack (arch/arm64/kernel/irq.c:87) 
>> [    2.731547] __irq_exit_rcu (kernel/softirq.c:442 kernel/softirq.c:662) 
>> [    2.731550] irq_exit_rcu (kernel/softirq.c:681) 
>> [    2.731554] el1_interrupt (arch/arm64/kernel/entry-common.c:565 arch/arm64/kernel/entry-common.c:575) 
>> [    2.731561] el1h_64_irq_handler (arch/arm64/kernel/entry-common.c:581) 
>> [    2.731567] el1h_64_irq (arch/arm64/kernel/entry.S:596) 
>> [    2.731570] qman_enqueue (drivers/soc/fsl/qbman/qman.c:2354) (P)
>> [    2.731576] caam_qi_enqueue (drivers/crypto/caam/qi.c:125) 
> 
> So caam_qi_enqueue hasn't had a chance to increment the refcount
> and the IRQ already came in to decrement it.  Lesson is that you
> should always increment your refcount before you give it away.
> 
> ---8<---
> Ensure refcount is raised before request is enqueued since it could
> be dequeued before the call returns.
> 
> Reported-by: Sean Anderson <sean.anderson@linux.dev>
> Cc: <stable@vger.kernel.org>
> Fixes: 11144416a755 ("crypto: caam/qi - optimize frame queue cleanup")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/drivers/crypto/caam/qi.c b/drivers/crypto/caam/qi.c
> index 7701d00bcb3a..b6e7c0b29d4e 100644
> --- a/drivers/crypto/caam/qi.c
> +++ b/drivers/crypto/caam/qi.c
> @@ -122,12 +122,12 @@ int caam_qi_enqueue(struct device *qidev, struct caam_drv_req *req)
>  	qm_fd_addr_set64(&fd, addr);
>  
>  	do {
> +		refcount_inc(&req->drv_ctx->refcnt);
>  		ret = qman_enqueue(req->drv_ctx->req_fq, &fd);
> -		if (likely(!ret)) {
> -			refcount_inc(&req->drv_ctx->refcnt);
> +		if (likely(!ret))
>  			return 0;
> -		}
>  
> +		refcount_dec(&req->drv_ctx->refcnt);
>  		if (ret != -EBUSY)
>  			break;
>  		num_retries++;

Tested-by: Sean Anderson <sean.anderson@linux.dev>

However, note that the following error is still present:

[    3.294978] alg: sig: sign test failed: invalid output
[    3.300128] 00000000: 68 e4 b7 d5 2e 12 d4 9b ca a5 a7 fc 50 2f d2 f6
[    3.306573] 00000010: 6b 01 58 1e b7 72 2c 0f 66 a4 c9 1e b9 49 21 a7
[    3.313016] 00000020: 53 62 26 6d bb 69 62 a2 7e 3b 8d 80 6f 2d 21 9d
[    3.319458] 00000030: cd 6f 92 e5 a9 03 e4 b3 b0 fc e7 4f ec a7 23 5c
[    3.325902] 00000040: 1d dc 08 13 c3 2e 8b d9 fa ab e0 6f 2b a0 9d 2b
[    3.332345] 00000050: 32 47 d5 ad ec 65 5a ce b3 13 35 95 37 0e f1 b0
[    3.338788] 00000060: c3 8e 22 0d c9 a2 b3 31 58 ea 74 18 18 d2 9f 7e
[    3.345230] 00000070: 04 fe 3b f1 5e 40 6c 39 3f 38 a2 71 58 fa da 1a
[    3.351672] 00000080: ed 02 70 7c 9e 72 93 a3 66 25 f2 35 e9 82 00 49
[    3.358114] 00000090: 8e 09 b9 f0 52 76 b2 9e b4 06 b0 60 f2 15 a1 78
[    3.364556] 000000a0: 74 3c 9b 87 17 f6 e8 ff ca fe 41 ed 73 c8 28 70
[    3.370999] 000000b0: 33 cc a2 de d6 04 7a b5 85 81 a3 46 16 46 66 af
[    3.377442] 000000c0: 26 84 bc 9e 70 68 7c b3 68 44 73 4c c2 a4 70 45
[    3.383887] 000000d0: a5 af a3 b3 9c cb b5 c8 bf 7d 8a 17 97 f6 33 0c
[    3.390329] 000000e0: 94 cc d3 fd ee e7 ba 8b dc 6f c7 3b 3b 67 14 ae
[    3.396771] 000000f0: 6a 00 33 90 df c5 f9 a2 6c b8 93 f7 cf 6b 0d 0d
[    3.403214] alg: sig: test 1 failed for pkcs1(rsa-caam,sha256): err -22
[    3.409833] alg: self-tests for pkcs1(rsa,sha256) using pkcs1(rsa-caam,sha256) failed (rc=-22)
[    3.409836] ------------[ cut here ]------------
[    3.423067] alg: self-tests for pkcs1(rsa,sha256) using pkcs1(rsa-caam,sha256) failed (rc=-22)
[    3.423094] WARNING: CPU: 3 PID: 337 at crypto/testmgr.c:6012 alg_test (crypto/testmgr.c:6012 (discriminator 1)) 
[    3.439282] Modules linked in:
[    3.442334] CPU: 3 UID: 0 PID: 337 Comm: cryptomgr_test Not tainted 6.14.0-seco+ #163 NONE
[    3.450687] Hardware name: LS1046A RDB Board (DT)
[    3.455387] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    3.462349] pc : alg_test (crypto/testmgr.c:6012 (discriminator 1)) 
[    3.466008] lr : alg_test (crypto/testmgr.c:6012 (discriminator 1)) 
[    3.469667] sp : ffffffc086ebbd40
[    3.472974] x29: ffffffc086ebbd40 x28: 0000000000000000 x27: 0000000000000000
[    3.480114] x26: 00000000ffffffea x25: 00000000ffffffff x24: ffffffc082066470
[    3.487253] x23: 0000000000000159 x22: 0000000000000807 x21: ffffff8801ce2a80
[    3.494392] x20: ffffff8801ce2a00 x19: ffffffc081290df0 x18: 0000000000000000
[    3.501531] x17: 35326168732c6d61 x16: 61632d6173722831 x15: 000000000000002d
[    3.508670] x14: 0000000000000000 x13: 00000000fffffffe x12: 65742d666c657320
[    3.515808] x11: 656820747563205b x10: ffffffc081dc0640 x9 : ffffffc081d91ef8
[    3.522946] x8 : ffffffc086ebbb28 x7 : ffffffc081d91ef8 x6 : 00000000fffff7ff
[    3.530085] x5 : 00000000000001a4 x4 : 0000000000000000 x3 : 0000000000000000
[    3.537224] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff88023fc200
[    3.544362] Call trace:
[    3.546801] alg_test (crypto/testmgr.c:6012 (discriminator 1)) (P)
[    3.550461] cryptomgr_test (crypto/algboss.c:181) 
[    3.554122] kthread (kernel/kthread.c:464) 
[    3.557261] ret_from_fork (arch/arm64/kernel/entry.S:863) 
[    3.560833] ---[ end trace 0000000000000000 ]---

as well as another error on reboot (present before but I forgot to post it):

[   98.514208] ------------[ cut here ]------------
[   98.518823] WARNING: CPU: 3 PID: 1 at crypto/algapi.c:464 crypto_unregister_alg (crypto/algapi.c:464 (discriminator 1)) 
[   98.527100] Modules linked in:
[   98.530153] CPU: 3 UID: 0 PID: 1 Comm: systemd-shutdow Tainted: G        W          6.14.0-seco+ #163 NONE
[   98.539899] Tainted: [W]=WARN
[   98.542859] Hardware name: LS1046A RDB Board (DT)
[   98.547559] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   98.554520] pc : crypto_unregister_alg (crypto/algapi.c:464 (discriminator 1)) 
[   98.559224] lr : crypto_unregister_alg (include/linux/atomic/atomic-arch-fallback.h:457 include/linux/atomic/atomic-instrumented.h:33 include/linux/refcount.h:136 crypto/algapi.c:464) 
[   98.563926] sp : ffffffc0820dbb80
[   98.567234] x29: ffffffc0820dbb80 x28: ffffff8800160000 x27: ffffff8801787090
[   98.574374] x26: 0000000000000000 x25: ffffffc082022030 x24: 0000000000000000
[   98.581513] x23: 0000000000000001 x22: ffffffc081efa4e0 x21: ffffffc0820dbbb8
[   98.588652] x20: ffffffc081e80cc8 x19: ffffffc081f095c0 x18: 0000000000000000
[   98.595791] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000001
[   98.602929] x14: ffffff8800160080 x13: 00000000000001b6 x12: 0000000000000001
[   98.610069] x11: 0000000000000000 x10: 000000000000042c x9 : 0000000000000001
[   98.617207] x8 : ffffffc0820dbb68 x7 : ffffffc081f095d0 x6 : ffffffc0820dbb58
[   98.624346] x5 : ffffffc081f095c0 x4 : 0000000000000000 x3 : 0000000000000000
[   98.631484] x2 : ffffffffffffffff x1 : 0000000000000001 x0 : 0000000000000002
[   98.638623] Call trace:
[   98.641062] crypto_unregister_alg (crypto/algapi.c:464 (discriminator 1)) (P)
[   98.645765] crypto_unregister_rng (crypto/rng.c:188) 
[   98.650035] caam_prng_unregister (drivers/crypto/caam/caamprng.c:207) 
[   98.654216] caam_jr_remove (drivers/crypto/caam/jr.c:59 drivers/crypto/caam/jr.c:207) 
[   98.657963] platform_shutdown (drivers/base/platform.c:1439) 
[   98.661883] device_shutdown (include/linux/device.h:937 drivers/base/core.c:4823) 
[   98.665804] kernel_restart (kernel/reboot.c:271 kernel/reboot.c:285) 
[   98.669465] __do_sys_reboot (kernel/reboot.c:725) 
[   98.673384] __arm64_sys_reboot (kernel/reboot.c:723) 
[   98.677390] invoke_syscall (arch/arm64/include/asm/current.h:19 arch/arm64/kernel/syscall.c:54) 
[   98.681137] el0_svc_common.constprop.0 (include/linux/thread_info.h:135 (discriminator 2) arch/arm64/kernel/syscall.c:140 (discriminator 2)) 
[   98.685841] do_el0_svc (arch/arm64/kernel/syscall.c:152) 
[   98.689152] el0_svc (arch/arm64/include/asm/irqflags.h:55 arch/arm64/include/asm/irqflags.h:76 arch/arm64/kernel/entry-common.c:165 arch/arm64/kernel/entry-common.c:178 arch/arm64/kernel/entry-common.c:745) 
[   98.692204] el0t_64_sync_handler (arch/arm64/kernel/entry-common.c:763) 
[   98.696559] el0t_64_sync (arch/arm64/kernel/entry.S:600) 
[   98.700217] ---[ end trace 0000000000000000 ]---

--Sean

On Tue, Apr 08, 2025 at 11:44:27AM -0400, Sean Anderson wrote:
>
> However, note that the following error is still present:
> 
> [    3.294978] alg: sig: sign test failed: invalid output

That just looks like a bug in the driver.  I'll leave it in the
caam maintainers' capable hands :)
 
> as well as another error on reboot (present before but I forgot to post it):
> 
> [   98.514208] ------------[ cut here ]------------
> [   98.518823] WARNING: CPU: 3 PID: 1 at crypto/algapi.c:464 crypto_unregister_alg (crypto/algapi.c:464 (discriminator 1)) 
> [   98.527100] Modules linked in:
> [   98.530153] CPU: 3 UID: 0 PID: 1 Comm: systemd-shutdow Tainted: G        W          6.14.0-seco+ #163 NONE
> [   98.539899] Tainted: [W]=WARN
> [   98.542859] Hardware name: LS1046A RDB Board (DT)
> [   98.547559] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [   98.554520] pc : crypto_unregister_alg (crypto/algapi.c:464 (discriminator 1)) 
> [   98.559224] lr : crypto_unregister_alg (include/linux/atomic/atomic-arch-fallback.h:457 include/linux/atomic/atomic-instrumented.h:33 include/linux/refcount.h:136 crypto/algapi.c:464) 

Yes that's a long-standing design fault in the crypto_unregister
mechanism for drivers.

The unregister mechanism was designed for software crypto, so it
relied on module refcounts to prevent unregistering something
that's still in use.  For hardware that follows the device model
where things get unplugged at random, this obviously doesn't work.

What I'll do is make the crypto_unregister call wait for the users
to go away.  That matches how the network device unregistration works
and hopefully should solve this problem.  But keep your eyes for
dead locks that used to plague netdev unregistration :)

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Wed, Apr 09, 2025 at 10:58:04AM +0800, Herbert Xu wrote:
>
> What I'll do is make the crypto_unregister call wait for the users
> to go away.  That matches how the network device unregistration works
> and hopefully should solve this problem.  But keep your eyes for
> dead locks that used to plague netdev unregistration :)

That was a dumb idea.  All it would do is make the shutdown hang.
So here is a different tack.  Let the algorithms stick around,
by allocating them dynamically instead.  Then we could simply
kfree them when the user finally disappears (if ever).

Note to make this work, caam needs to be modified to allocate the
algorithms dynamically (kmemdup should work), and add a cra_destroy
function to kfree the memory.

---8<---
The current algorithm unregistration mechanism originated from
software crypto.  The code relies on module reference counts to
stop in-use algorithms from being unregistered.  Therefore if
the unregistration function is reached, it is assumed that the
module reference count has hit zero and thus the algorithm reference
count should be exactly 1.

This is completely broken for hardware devices, which can be
unplugged at random.

Fix this by allowing algorithms to be destroyed later if a destroy
callback is provided.

Reported-by: Sean Anderson <sean.anderson@linux.dev>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 5b8a4c787387..f368c0dc0d6d 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -481,10 +481,10 @@ void crypto_unregister_alg(struct crypto_alg *alg)
 	if (WARN(ret, "Algorithm %s is not registered", alg->cra_driver_name))
 		return;
 
-	if (WARN_ON(refcount_read(&alg->cra_refcnt) != 1))
-		return;
-
-	if (alg->cra_type && alg->cra_type->destroy)
+	if (alg->cra_destroy)
+		crypto_alg_put(alg);
+	else if (!WARN_ON(refcount_read(&alg->cra_refcnt) != 1) &&
+		 alg->cra_type && alg->cra_type->destroy)
 		alg->cra_type->destroy(alg);
 
 	crypto_remove_final(&list);
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On 4/8/25 23:29, Herbert Xu wrote:
> On Wed, Apr 09, 2025 at 10:58:04AM +0800, Herbert Xu wrote:
>>
>> What I'll do is make the crypto_unregister call wait for the users
>> to go away.  That matches how the network device unregistration works
>> and hopefully should solve this problem.  But keep your eyes for
>> dead locks that used to plague netdev unregistration :)
> 
> That was a dumb idea.  All it would do is make the shutdown hang.
> So here is a different tack.  Let the algorithms stick around,
> by allocating them dynamically instead.  Then we could simply
> kfree them when the user finally disappears (if ever).
> 
> Note to make this work, caam needs to be modified to allocate the
> algorithms dynamically (kmemdup should work), and add a cra_destroy
> function to kfree the memory.
> 
> ---8<---
> The current algorithm unregistration mechanism originated from
> software crypto.  The code relies on module reference counts to
> stop in-use algorithms from being unregistered.  Therefore if
> the unregistration function is reached, it is assumed that the
> module reference count has hit zero and thus the algorithm reference
> count should be exactly 1.
> 
> This is completely broken for hardware devices, which can be
> unplugged at random.
> 
> Fix this by allowing algorithms to be destroyed later if a destroy
> callback is provided.
> 
> Reported-by: Sean Anderson <sean.anderson@linux.dev>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 5b8a4c787387..f368c0dc0d6d 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -481,10 +481,10 @@ void crypto_unregister_alg(struct crypto_alg *alg)
>  	if (WARN(ret, "Algorithm %s is not registered", alg->cra_driver_name))
>  		return;
>  
> -	if (WARN_ON(refcount_read(&alg->cra_refcnt) != 1))
> -		return;
> -
> -	if (alg->cra_type && alg->cra_type->destroy)
> +	if (alg->cra_destroy)
> +		crypto_alg_put(alg);
> +	else if (!WARN_ON(refcount_read(&alg->cra_refcnt) != 1) &&
> +		 alg->cra_type && alg->cra_type->destroy)
>  		alg->cra_type->destroy(alg);
>  
>  	crypto_remove_final(&list);

The above patch didn't apply cleanly. I seem to be missing cra_type. What
tree should I test with?

--Sean

On Thu, Apr 10, 2025 at 07:24:25PM -0400, Sean Anderson wrote:
>
> The above patch didn't apply cleanly. I seem to be missing cra_type. What
> tree should I test with?

The patch goes on top of cryptodev.  But it won't do anything
without a corresponding patch to caam that moves the algorithm
data structures into dynamically allocated memory, and adds a
cra_destroy hook to free that memory.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Fri, Apr 11, 2025 at 09:36:27AM +0800, Herbert Xu wrote:
> 
> The patch goes on top of cryptodev.  But it won't do anything
> without a corresponding patch to caam that moves the algorithm
> data structures into dynamically allocated memory, and adds a
> cra_destroy hook to free that memory.

Here's a patch on top that allows drivers to do this easily.
Unfortunately it still won't help caam because it embeds the
algorithm in a bigger structure, so the duplication needs to
be done by hand.

---8<---
If the bit CRYPTO_ALG_DUP_FIRST is set, an algorithm will be
duplicated by kmemdup before registration.  This is inteded for
hardware-based algorithms that may be unplugged at will.

Do not use this if the algorithm data structure is embedded in a
bigger data structure.  Perform the duplication in the driver
instead.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/acompress.c     |  1 +
 crypto/aead.c          |  1 +
 crypto/ahash.c         |  1 +
 crypto/akcipher.c      |  1 +
 crypto/algapi.c        | 41 ++++++++++++++++++++++++++++-------------
 crypto/api.c           |  9 +++++++++
 crypto/internal.h      |  5 ++++-
 crypto/kpp.c           |  1 +
 crypto/lskcipher.c     |  1 +
 crypto/rng.c           |  1 +
 crypto/scompress.c     |  1 +
 crypto/shash.c         |  1 +
 crypto/sig.c           |  1 +
 crypto/skcipher.c      |  1 +
 include/linux/crypto.h |  9 +++++++++
 15 files changed, 61 insertions(+), 14 deletions(-)

diff --git a/crypto/acompress.c b/crypto/acompress.c
index 5d0b8b8b84f6..4763524732ba 100644
--- a/crypto/acompress.c
+++ b/crypto/acompress.c
@@ -152,6 +152,7 @@ static const struct crypto_type crypto_acomp_type = {
 	.maskset = CRYPTO_ALG_TYPE_ACOMPRESS_MASK,
 	.type = CRYPTO_ALG_TYPE_ACOMPRESS,
 	.tfmsize = offsetof(struct crypto_acomp, base),
+	.algsize = offsetof(struct acomp_alg, base),
 };
 
 struct crypto_acomp *crypto_alloc_acomp(const char *alg_name, u32 type,
diff --git a/crypto/aead.c b/crypto/aead.c
index 12f5b42171af..5d14b775036e 100644
--- a/crypto/aead.c
+++ b/crypto/aead.c
@@ -186,6 +186,7 @@ static const struct crypto_type crypto_aead_type = {
 	.maskset = CRYPTO_ALG_TYPE_MASK,
 	.type = CRYPTO_ALG_TYPE_AEAD,
 	.tfmsize = offsetof(struct crypto_aead, base),
+	.algsize = offsetof(struct aead_alg, base),
 };
 
 int crypto_grab_aead(struct crypto_aead_spawn *spawn,
diff --git a/crypto/ahash.c b/crypto/ahash.c
index 2d9eec2b2b1c..7ed88f8bedeb 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -897,6 +897,7 @@ static const struct crypto_type crypto_ahash_type = {
 	.maskset = CRYPTO_ALG_TYPE_AHASH_MASK,
 	.type = CRYPTO_ALG_TYPE_AHASH,
 	.tfmsize = offsetof(struct crypto_ahash, base),
+	.algsize = offsetof(struct ahash_alg, halg.base),
 };
 
 int crypto_grab_ahash(struct crypto_ahash_spawn *spawn,
diff --git a/crypto/akcipher.c b/crypto/akcipher.c
index 72c82d9aa077..a36f50c83827 100644
--- a/crypto/akcipher.c
+++ b/crypto/akcipher.c
@@ -97,6 +97,7 @@ static const struct crypto_type crypto_akcipher_type = {
 	.maskset = CRYPTO_ALG_TYPE_AHASH_MASK,
 	.type = CRYPTO_ALG_TYPE_AKCIPHER,
 	.tfmsize = offsetof(struct crypto_akcipher, base),
+	.algsize = offsetof(struct akcipher_alg, base),
 };
 
 int crypto_grab_akcipher(struct crypto_akcipher_spawn *spawn,
diff --git a/crypto/algapi.c b/crypto/algapi.c
index f368c0dc0d6d..532d3efc3c7d 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -66,13 +66,7 @@ static int crypto_check_alg(struct crypto_alg *alg)
 
 static void crypto_free_instance(struct crypto_instance *inst)
 {
-	struct crypto_alg *alg = &inst->alg;
-	const struct crypto_type *type;
-
-	type = alg->cra_type;
-	if (type->destroy)
-		type->destroy(alg);
-	type->free(inst);
+	inst->alg.cra_type->free(inst);
 }
 
 static void crypto_destroy_instance_workfn(struct work_struct *w)
@@ -424,6 +418,15 @@ void crypto_remove_final(struct list_head *list)
 }
 EXPORT_SYMBOL_GPL(crypto_remove_final);
 
+static void crypto_free_alg(struct crypto_alg *alg)
+{
+	unsigned int algsize = alg->cra_type->algsize;
+	u8 *p = (u8 *)alg - algsize;
+
+	crypto_destroy_alg(alg);
+	kfree(p);
+}
+
 int crypto_register_alg(struct crypto_alg *alg)
 {
 	struct crypto_larval *larval;
@@ -436,6 +439,19 @@ int crypto_register_alg(struct crypto_alg *alg)
 	if (err)
 		return err;
 
+	if (alg->cra_flags & CRYPTO_ALG_DUP_FIRST &&
+	    !WARN_ON_ONCE(alg->cra_destroy)) {
+		unsigned int algsize = alg->cra_type->algsize;
+		u8 *p = (u8 *)alg - algsize;
+
+		p = kmemdup(p, algsize + sizeof(*alg), GFP_KERNEL);
+		if (!p)
+			return -ENOMEM;
+
+		alg = (void *)(p + algsize);
+		alg->cra_destroy = crypto_free_alg;
+	}
+
 	down_write(&crypto_alg_sem);
 	larval = __crypto_register_alg(alg, &algs_to_put);
 	if (!IS_ERR_OR_NULL(larval)) {
@@ -444,8 +460,10 @@ int crypto_register_alg(struct crypto_alg *alg)
 	}
 	up_write(&crypto_alg_sem);
 
-	if (IS_ERR(larval))
+	if (IS_ERR(larval)) {
+		crypto_alg_put(alg);
 		return PTR_ERR(larval);
+	}
 
 	if (test_started)
 		crypto_schedule_test(larval);
@@ -481,12 +499,9 @@ void crypto_unregister_alg(struct crypto_alg *alg)
 	if (WARN(ret, "Algorithm %s is not registered", alg->cra_driver_name))
 		return;
 
-	if (alg->cra_destroy)
-		crypto_alg_put(alg);
-	else if (!WARN_ON(refcount_read(&alg->cra_refcnt) != 1) &&
-		 alg->cra_type && alg->cra_type->destroy)
-		alg->cra_type->destroy(alg);
+	WARN_ON(!alg->cra_destroy && refcount_read(&alg->cra_refcnt) != 1);
 
+	list_add(&alg->cra_list, &list);
 	crypto_remove_final(&list);
 }
 EXPORT_SYMBOL_GPL(crypto_unregister_alg);
diff --git a/crypto/api.c b/crypto/api.c
index 2880aa04bb99..e427cc5662b5 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -703,5 +703,14 @@ void crypto_req_done(void *data, int err)
 }
 EXPORT_SYMBOL_GPL(crypto_req_done);
 
+void crypto_destroy_alg(struct crypto_alg *alg)
+{
+	if (alg->cra_type && alg->cra_type->destroy)
+		alg->cra_type->destroy(alg);
+	if (alg->cra_destroy)
+		alg->cra_destroy(alg);
+}
+EXPORT_SYMBOL_GPL(crypto_destroy_alg);
+
 MODULE_DESCRIPTION("Cryptographic core API");
 MODULE_LICENSE("GPL");
diff --git a/crypto/internal.h b/crypto/internal.h
index 2edefb546ad4..2ed79bf208ca 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -46,6 +46,7 @@ struct crypto_type {
 	unsigned int maskclear;
 	unsigned int maskset;
 	unsigned int tfmsize;
+	unsigned int algsize;
 };
 
 enum {
@@ -162,10 +163,12 @@ static inline struct crypto_alg *crypto_alg_get(struct crypto_alg *alg)
 	return alg;
 }
 
+void crypto_destroy_alg(struct crypto_alg *alg);
+
 static inline void crypto_alg_put(struct crypto_alg *alg)
 {
 	if (refcount_dec_and_test(&alg->cra_refcnt))
-		alg->cra_destroy(alg);
+		crypto_destroy_alg(alg);
 }
 
 static inline int crypto_tmpl_get(struct crypto_template *tmpl)
diff --git a/crypto/kpp.c b/crypto/kpp.c
index ecc63a1a948d..2e0cefe7a25f 100644
--- a/crypto/kpp.c
+++ b/crypto/kpp.c
@@ -80,6 +80,7 @@ static const struct crypto_type crypto_kpp_type = {
 	.maskset = CRYPTO_ALG_TYPE_MASK,
 	.type = CRYPTO_ALG_TYPE_KPP,
 	.tfmsize = offsetof(struct crypto_kpp, base),
+	.algsize = offsetof(struct kpp_alg, base),
 };
 
 struct crypto_kpp *crypto_alloc_kpp(const char *alg_name, u32 type, u32 mask)
diff --git a/crypto/lskcipher.c b/crypto/lskcipher.c
index cdb4897c63e6..c2e2c38b5aa8 100644
--- a/crypto/lskcipher.c
+++ b/crypto/lskcipher.c
@@ -294,6 +294,7 @@ static const struct crypto_type crypto_lskcipher_type = {
 	.maskset = CRYPTO_ALG_TYPE_MASK,
 	.type = CRYPTO_ALG_TYPE_LSKCIPHER,
 	.tfmsize = offsetof(struct crypto_lskcipher, base),
+	.algsize = offsetof(struct lskcipher_alg, co.base),
 };
 
 static void crypto_lskcipher_exit_tfm_sg(struct crypto_tfm *tfm)
diff --git a/crypto/rng.c b/crypto/rng.c
index 9d8804e46422..b8ae6ebc091d 100644
--- a/crypto/rng.c
+++ b/crypto/rng.c
@@ -98,6 +98,7 @@ static const struct crypto_type crypto_rng_type = {
 	.maskset = CRYPTO_ALG_TYPE_MASK,
 	.type = CRYPTO_ALG_TYPE_RNG,
 	.tfmsize = offsetof(struct crypto_rng, base),
+	.algsize = offsetof(struct rng_alg, base),
 };
 
 struct crypto_rng *crypto_alloc_rng(const char *alg_name, u32 type, u32 mask)
diff --git a/crypto/scompress.c b/crypto/scompress.c
index 4a2b3933aa95..19f1312d7dd7 100644
--- a/crypto/scompress.c
+++ b/crypto/scompress.c
@@ -367,6 +367,7 @@ static const struct crypto_type crypto_scomp_type = {
 	.maskset = CRYPTO_ALG_TYPE_MASK,
 	.type = CRYPTO_ALG_TYPE_SCOMPRESS,
 	.tfmsize = offsetof(struct crypto_scomp, base),
+	.algsize = offsetof(struct scomp_alg, base),
 };
 
 static void scomp_prepare_alg(struct scomp_alg *alg)
diff --git a/crypto/shash.c b/crypto/shash.c
index 301ab42bf849..a2a7d6609172 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -227,6 +227,7 @@ const struct crypto_type crypto_shash_type = {
 	.maskset = CRYPTO_ALG_TYPE_MASK,
 	.type = CRYPTO_ALG_TYPE_SHASH,
 	.tfmsize = offsetof(struct crypto_shash, base),
+	.algsize = offsetof(struct shash_alg, base),
 };
 
 int crypto_grab_shash(struct crypto_shash_spawn *spawn,
diff --git a/crypto/sig.c b/crypto/sig.c
index dfc7cae90802..f2df943b96e1 100644
--- a/crypto/sig.c
+++ b/crypto/sig.c
@@ -74,6 +74,7 @@ static const struct crypto_type crypto_sig_type = {
 	.maskset = CRYPTO_ALG_TYPE_MASK,
 	.type = CRYPTO_ALG_TYPE_SIG,
 	.tfmsize = offsetof(struct crypto_sig, base),
+	.algsize = offsetof(struct sig_alg, base),
 };
 
 struct crypto_sig *crypto_alloc_sig(const char *alg_name, u32 type, u32 mask)
diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 132075a905d9..319215cfded5 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -620,6 +620,7 @@ static const struct crypto_type crypto_skcipher_type = {
 	.maskset = CRYPTO_ALG_TYPE_SKCIPHER_MASK,
 	.type = CRYPTO_ALG_TYPE_SKCIPHER,
 	.tfmsize = offsetof(struct crypto_skcipher, base),
+	.algsize = offsetof(struct skcipher_alg, base),
 };
 
 int crypto_grab_skcipher(struct crypto_skcipher_spawn *spawn,
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 1e3809d28abd..f87f124ddc18 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -50,6 +50,15 @@
  */
 #define CRYPTO_ALG_NEED_FALLBACK	0x00000100
 
+/*
+ * Set if the algorithm data structure should be duplicated into
+ * kmalloc memory before registration.  This is useful for hardware
+ * that can be disconnected at will.  Do not use this if the data
+ * structure is embedded into a bigger one.  Duplicate the overall
+ * data structure in the driver in that case.
+ */
+#define CRYPTO_ALG_DUP_FIRST		0x00000200
+
 /*
  * Set if the algorithm has passed automated run-time testing.  Note that
  * if there is no run-time testing for a given algorithm it is considered
-- 
2.39.5

-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Sat, Apr 12, 2025 at 01:16:43PM +0800, Herbert Xu wrote:
> On Fri, Apr 11, 2025 at 09:36:27AM +0800, Herbert Xu wrote:
> > 
> > The patch goes on top of cryptodev.  But it won't do anything
> > without a corresponding patch to caam that moves the algorithm
> > data structures into dynamically allocated memory, and adds a
> > cra_destroy hook to free that memory.
> 
> Here's a patch on top that allows drivers to do this easily.
> Unfortunately it still won't help caam because it embeds the
> algorithm in a bigger structure, so the duplication needs to
> be done by hand.
> 
> ---8<---
> If the bit CRYPTO_ALG_DUP_FIRST is set, an algorithm will be
> duplicated by kmemdup before registration.  This is inteded for
> hardware-based algorithms that may be unplugged at will.
> 
> Do not use this if the algorithm data structure is embedded in a
> bigger data structure.  Perform the duplication in the driver
> instead.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Why does this make any sense?  The lifetime of the algorithm struct memory
should be the same as that of the owning module, and drivers should ensure that.
In which case hacks like this are not needed.

- Eric

On Sun, Apr 13, 2025 at 09:03:04AM -0700, Eric Biggers wrote:
>
> Why does this make any sense?  The lifetime of the algorithm struct memory
> should be the same as that of the owning module, and drivers should ensure that.
> In which case hacks like this are not needed.

Hardware can be unplugged at any time.  Once all hardware backing
a driver has been removed, you need to unregister that algorithm.

Please read my explanation for the prior patch:

https://patchwork.kernel.org/project/linux-crypto/patch/Z_XpfyPaoZ6Y8u6z@gondor.apana.org.au/

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On 4/8/2025 8:17 AM, Herbert Xu wrote:
> Ensure refcount is raised before request is enqueued since it could
> be dequeued before the call returns.
> 
> Reported-by: Sean Anderson <sean.anderson@linux.dev>
> Cc: <stable@vger.kernel.org>
> Fixes: 11144416a755 ("crypto: caam/qi - optimize frame queue cleanup")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>

Thanks!
Horia