The crypto_unregister_alg() function expects callers to ensure that any algorithm that is unregistered has a refcnt of exactly 1, and issues a BUG_ON() if this is not the case. However, there are in fact drivers that will call crypto_unregister_alg() without ensuring that the refcnt has been lowered first, most notably on system shutdown. This causes the BUG_ON() to trigger, which prevents a clean shutdown and hangs the system.
To avoid such hangs on shutdown, demote the BUG_ON() to WARN_ON() in crypto_unregister_alg(). Cc stable because this problem was observed on a 6.2 kernel, cf the link below.
Link: https://lore.kernel.org/r/87r0tyq8ph.fsf@toke.dk Cc: stable@vger.kernel.org Signed-off-by: Toke Høiland-Jørgensen toke@redhat.com --- crypto/algapi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crypto/algapi.c b/crypto/algapi.c index d08f864f08be..e9954fcb61be 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -493,7 +493,7 @@ void crypto_unregister_alg(struct crypto_alg *alg) if (WARN(ret, "Algorithm %s is not registered", alg->cra_driver_name)) return;
- BUG_ON(refcount_read(&alg->cra_refcnt) != 1); + WARN_ON(refcount_read(&alg->cra_refcnt) != 1); if (alg->cra_destroy) alg->cra_destroy(alg);
On Sat, Mar 11, 2023 at 05:25:12PM +0100, Toke Høiland-Jørgensen wrote:
diff --git a/crypto/algapi.c b/crypto/algapi.c index d08f864f08be..e9954fcb61be 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -493,7 +493,7 @@ void crypto_unregister_alg(struct crypto_alg *alg) if (WARN(ret, "Algorithm %s is not registered", alg->cra_driver_name)) return;
- BUG_ON(refcount_read(&alg->cra_refcnt) != 1);
- WARN_ON(refcount_read(&alg->cra_refcnt) != 1);
I think we should return here instead of continuing to destroy the algorithm since we know that it's still in use.
Thanks,
On Sun, Mar 12, 2023 at 03:27:10PM +0800, Herbert Xu wrote:
On Sat, Mar 11, 2023 at 05:25:12PM +0100, Toke Høiland-Jørgensen wrote:
diff --git a/crypto/algapi.c b/crypto/algapi.c index d08f864f08be..e9954fcb61be 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -493,7 +493,7 @@ void crypto_unregister_alg(struct crypto_alg *alg) if (WARN(ret, "Algorithm %s is not registered", alg->cra_driver_name)) return;
- BUG_ON(refcount_read(&alg->cra_refcnt) != 1);
- WARN_ON(refcount_read(&alg->cra_refcnt) != 1);
I think we should return here instead of continuing to destroy the algorithm since we know that it's still in use.
Also, this still panics a box with panic-on-warn enabled, so if this can be handled, returning an error is good.
thanks,
greg k-h
On Sun, Mar 12, 2023 at 12:10:38PM +0100, Greg KH wrote:
Also, this still panics a box with panic-on-warn enabled, so if this can be handled, returning an error is good.
Well it can't really be handled because the driver is trying to unregister an in-use algorithm so it's going to die one way or another.
But returning here at least means that if this is happening through shutdown then the system at least gets a chance to reboot.
Cheers,
linux-stable-mirror@lists.linaro.org