On Wed, Aug 02, 2023 at 07:09:22PM +0200, Florent Revest wrote:
>
> I found that the following program reliably reproduces a "BUG: sleeping function
> called from invalid context" backtrace in crypto code:
Great detective work! And thanks for cc'ing me :)
This is definitely a bug in the Crypto API. Although it's hard to
trigger because you need to unregister the instance before the last
user frees it in atomic context. The fact that it triggers for your
test program probably means that we're not creating the template
correctly and it gets unregistered as soon as it's created.
As to the fix I think we should move the work into crypto_destroy_instance
since that's the function that is being called from atomic context
and then does something that should only be done from process context.
So here's my patch based on your work:
---8<---
The function crypto_drop_spawn expects to be called in process
context. However, when an instance is unregistered while it still
has active users, the last user may cause the instance to be freed
in atomic context.
Fix this by delaying the freeing to a work queue.
Fixes: 6bfd48096ff8 ("[CRYPTO] api: Added spawns")
Reported-by: Florent Revest <[email protected]>
Reported-by: [email protected]
Reported-by: [email protected]
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/crypto/algapi.c b/crypto/algapi.c
index 5e7cd603d489..4fe95c448047 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -17,6 +17,7 @@
#include <linux/rtnetlink.h>
#include <linux/slab.h>
#include <linux/string.h>
+#include <linux/workqueue.h>
#include "internal.h"
@@ -74,15 +75,26 @@ static void crypto_free_instance(struct crypto_instance *inst)
inst->alg.cra_type->free(inst);
}
-static void crypto_destroy_instance(struct crypto_alg *alg)
+static void crypto_destroy_instance_workfn(struct work_struct *w)
{
- struct crypto_instance *inst = (void *)alg;
+ struct crypto_instance *inst = container_of(w, struct crypto_instance,
+ free_work);
struct crypto_template *tmpl = inst->tmpl;
crypto_free_instance(inst);
crypto_tmpl_put(tmpl);
}
+static void crypto_destroy_instance(struct crypto_alg *alg)
+{
+ struct crypto_instance *inst = container_of(alg,
+ struct crypto_instance,
+ alg);
+
+ INIT_WORK(&inst->free_work, crypto_destroy_instance_workfn);
+ schedule_work(&inst->free_work);
+}
+
/*
* This function adds a spawn to the list secondary_spawns which
* will be used at the end of crypto_remove_spawns to unregister
diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
index 6156161b181f..ca86f4c6ba43 100644
--- a/include/crypto/algapi.h
+++ b/include/crypto/algapi.h
@@ -12,6 +12,7 @@
#include <linux/cache.h>
#include <linux/crypto.h>
#include <linux/types.h>
+#include <linux/workqueue.h>
/*
* Maximum values for blocksize and alignmask, used to allocate
@@ -82,6 +83,8 @@ struct crypto_instance {
struct crypto_spawn *spawns;
};
+ struct work_struct free_work;
+
void *__ctx[] CRYPTO_MINALIGN_ATTR;
};
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, Aug 3, 2023 at 11:59 AM Herbert Xu <[email protected]> wrote:
>
> On Wed, Aug 02, 2023 at 07:09:22PM +0200, Florent Revest wrote:
> >
> > I found that the following program reliably reproduces a "BUG: sleeping function
> > called from invalid context" backtrace in crypto code:
>
> Great detective work! And thanks for cc'ing me :)
Thank you!
> This is definitely a bug in the Crypto API. Although it's hard to
> trigger because you need to unregister the instance before the last
> user frees it in atomic context. The fact that it triggers for your
> test program probably means that we're not creating the template
> correctly and it gets unregistered as soon as it's created.
>
> As to the fix I think we should move the work into crypto_destroy_instance
> since that's the function that is being called from atomic context
> and then does something that should only be done from process context.
Sounds good to me :)
> So here's my patch based on your work:
FWIW,
Tested-by: Florent Revest <[email protected]>
Acked-by: Florent Revest <[email protected]>
> ---8<---
> The function crypto_drop_spawn expects to be called in process
> context. However, when an instance is unregistered while it still
> has active users, the last user may cause the instance to be freed
> in atomic context.
>
> Fix this by delaying the freeing to a work queue.
>
> Fixes: 6bfd48096ff8 ("[CRYPTO] api: Added spawns")
> Reported-by: Florent Revest <[email protected]>
> Reported-by: [email protected]
> Reported-by: [email protected]
> Signed-off-by: Herbert Xu <[email protected]>
>
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 5e7cd603d489..4fe95c448047 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -17,6 +17,7 @@
> #include <linux/rtnetlink.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> +#include <linux/workqueue.h>
>
> #include "internal.h"
>
> @@ -74,15 +75,26 @@ static void crypto_free_instance(struct crypto_instance *inst)
> inst->alg.cra_type->free(inst);
> }
>
> -static void crypto_destroy_instance(struct crypto_alg *alg)
> +static void crypto_destroy_instance_workfn(struct work_struct *w)
> {
> - struct crypto_instance *inst = (void *)alg;
> + struct crypto_instance *inst = container_of(w, struct crypto_instance,
> + free_work);
> struct crypto_template *tmpl = inst->tmpl;
>
> crypto_free_instance(inst);
> crypto_tmpl_put(tmpl);
> }
>
> +static void crypto_destroy_instance(struct crypto_alg *alg)
> +{
> + struct crypto_instance *inst = container_of(alg,
> + struct crypto_instance,
> + alg);
> +
> + INIT_WORK(&inst->free_work, crypto_destroy_instance_workfn);
> + schedule_work(&inst->free_work);
> +}
> +
> /*
> * This function adds a spawn to the list secondary_spawns which
> * will be used at the end of crypto_remove_spawns to unregister
> diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
> index 6156161b181f..ca86f4c6ba43 100644
> --- a/include/crypto/algapi.h
> +++ b/include/crypto/algapi.h
> @@ -12,6 +12,7 @@
> #include <linux/cache.h>
> #include <linux/crypto.h>
> #include <linux/types.h>
> +#include <linux/workqueue.h>
>
> /*
> * Maximum values for blocksize and alignmask, used to allocate
> @@ -82,6 +83,8 @@ struct crypto_instance {
> struct crypto_spawn *spawns;
> };
>
> + struct work_struct free_work;
> +
> void *__ctx[] CRYPTO_MINALIGN_ATTR;
> };