| Message ID | Z_XpfyPaoZ6Y8u6z@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: api - Allow delayed algorithm destruction | expand |
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,
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);