Re: dm crypt: initialize tasklet in crypt_io_init()

[Ignat Korchagin]

On Wed, Mar 8, 2023 at 8:27 PM Mike Snitzer <snitzer@xxxxxxxxxx> wrote:
>
> On Wed, Mar 08 2023 at  2:19P -0500,
> Mike Snitzer <snitzer@xxxxxxxxxx> wrote:
>
> > On Wed, Mar 08 2023 at  8:55P -0500,
> > Ignat Korchagin <ignat@xxxxxxxxxxxxxx> wrote:
> >
> > > Perhaps instead we can just pass an additional flag from
> > > tasklet_schedule to indicate to the function that we're running in a
> > > tasklet. I originally have chosen the tasklet_trylock/unlock hack to
> > > avoid passing an extra flag. But unitialized memory makes sense as
> > > well as the desire to avoid calling tasklet_init unconditionally. So
> > > an extra member in dm_crypt_io might be the most straightforward here.
> >
> > ... I think we should certainly evaluate the use of an extra flag.
> >
> > Ignat: I'll have a look at implementing it but if you have a patch
> > already developed please do share.
>
> I've staged the following in linux-next for 6.3 via the linux-dm.git,
> but if you see anything wrong with it I can obviously fix:
>
> From: Mike Snitzer <snitzer@xxxxxxxxxx>
> Date: Wed, 8 Mar 2023 14:39:54 -0500
> Subject: [PATCH] dm crypt: avoid accessing uninitialized tasklet
>
> When neither "no_read_workqueue" nor "no_write_workqueue" are enabled,
> tasklet_trylock() in crypt_dec_pending() may still return false due to
> an uninitialized state, and dm-crypt will unnecessarily do io completion
> in io_queue workqueue instead of current context.
>
> Fix this by adding an 'in_tasklet' flag to dm_crypt_io struct and
> initialize it to false in crypt_io_init(). Set this flag to true in
> kcryptd_queue_crypt() before calling tasklet_schedule(). If set
> crypt_dec_pending() will punt io completion to a workqueue.
>
> This also nicely avoids the tasklet_trylock/unlock hack when tasklets
> aren't in use.
>
> Fixes: 8e14f610159d ("dm crypt: do not call bio_endio() from the dm-crypt tasklet")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Hou Tao <houtao1@xxxxxxxxxx>
> Suggested-by: Ignat Korchagin <ignat@xxxxxxxxxxxxxx>
> Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> ---
>  drivers/md/dm-crypt.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index faba1be572f9..de08ff4f7c98 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -72,7 +72,9 @@ struct dm_crypt_io {
>         struct crypt_config *cc;
>         struct bio *base_bio;
>         u8 *integrity_metadata;
> -       bool integrity_metadata_from_pool;
> +       bool integrity_metadata_from_pool:1;
> +       bool in_tasklet:1;
> +
>         struct work_struct work;
>         struct tasklet_struct tasklet;
>
> @@ -1731,6 +1733,7 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc,
>         io->ctx.r.req = NULL;
>         io->integrity_metadata = NULL;
>         io->integrity_metadata_from_pool = false;
> +       io->in_tasklet = false;
>         atomic_set(&io->io_pending, 0);
>  }
>
> @@ -1777,8 +1780,7 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
>          * our tasklet. In this case we need to delay bio_endio()
>          * execution to after the tasklet is done and dequeued.
>          */
> -       if (tasklet_trylock(&io->tasklet)) {
> -               tasklet_unlock(&io->tasklet);
> +       if (!io->in_tasklet) {

nitpick: maybe invert the logic here for better readability? (so it
becomes "if (in_tasklet) queue..." else just falls through bio_endio()
)

>                 bio_endio(base_bio);
>                 return;
>         }
> @@ -2233,6 +2235,7 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>                  * it is being executed with irqs disabled.
>                  */
>                 if (in_hardirq() || irqs_disabled()) {
> +                       io->in_tasklet = true;
>                         tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
>                         tasklet_schedule(&io->tasklet);
>                         return;
> --
> 2.37.1 (Apple Git-137.1)
>

Reviewed-by: Ignat Korchagin <ignat@xxxxxxxxxxxxxx>