| Message ID | ZMt6gH4FSHsDetkU@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: api - Use work queue in crypto_destroy_instance | expand |
On Thu, Aug 3, 2023 at 11:59 AM Herbert Xu <herbert@gondor.apana.org.au> 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 <revest@chromium.org> Acked-by: Florent Revest <revest@chromium.org> > ---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 <revest@chromium.org> > Reported-by: syzbot+d769eed29cc42d75e2a3@syzkaller.appspotmail.com > Reported-by: syzbot+610ec0671f51e838436e@syzkaller.appspotmail.com > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > 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; > };
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; };