Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230802170923.1151605-1-revest@chromium.org> <ZMt6gH4FSHsDetkU@gondor.apana.org.au>
In-Reply-To: <ZMt6gH4FSHsDetkU@gondor.apana.org.au>
From:   Florent Revest <revest@chromium.org>
Date:   Thu, 3 Aug 2023 14:13:05 +0200
Message-ID: <CABRcYmLWYr19MVsnrw2OEkmkpJLUNjPULgwooKxYO=R2J2mLww@mail.gmail.com>
Subject: Re: crypto: api - Use work queue in crypto_destroy_instance
To:     Herbert Xu <herbert@gondor.apana.org.au>
Cc:     linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-sctp@vger.kernel.org, davem@davemloft.net,
        hillf.zj@alibaba-inc.com, marcelo.leitner@gmail.com,
        lucien.xin@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Aug 3, 2023 at 11:59=E2=80=AFAM Herbert Xu <herbert@gondor.apana.or=
g.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_instanc=
e
> 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_instan=
ce *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 =3D (void *)alg;
> +       struct crypto_instance *inst =3D container_of(w, struct crypto_in=
stance,
> +                                                   free_work);
>         struct crypto_template *tmpl =3D inst->tmpl;
>
>         crypto_free_instance(inst);
>         crypto_tmpl_put(tmpl);
>  }
>
> +static void crypto_destroy_instance(struct crypto_alg *alg)
> +{
> +       struct crypto_instance *inst =3D container_of(alg,
> +                                                   struct crypto_instanc=
e,
> +                                                   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;
>  };