On Tue, Jun 14 2022 at 12:10P -0400,
Benjamin Marzinski <bmarz...@redhat.com> wrote:

> After commit 82f6cdcc3676c ("dm: switch dm_io booleans over to proper
> flags") dm_start_io_acct stopped atomically checking and setting
> was_accounted, which turned into the DM_IO_ACCOUNTED flag. This opened
> the possibility for a race where IO accounting is started twice for
> duplicate bios. To remove the race, check the flag while holding the
> io->lock.
> 
> Fixes: 82f6cdcc3676c ("dm: switch dm_io booleans over to proper flags")
> Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com>
> ---
>  drivers/md/dm.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index d8f16183bf27..c7d2dbf03ccb 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -544,17 +544,20 @@ static void dm_start_io_acct(struct dm_io *io, struct 
> bio *clone)
>  {
>       /*
>        * Ensure IO accounting is only ever started once.
> +      * Expect no possibility for race unless DM_TIO_IS_DUPLICATE_BIO.
>        */
> -     if (dm_io_flagged(io, DM_IO_ACCOUNTED))
> -             return;
> -

This check ^ is best left at the top since it avoids needless locking
in the DM_TIO_IS_DUPLICATE_BIO case.

> -     /* Expect no possibility for race unless DM_TIO_IS_DUPLICATE_BIO. */
>       if (!clone || likely(dm_tio_is_normal(clone_to_tio(clone)))) {
> +             if (WARN_ON_ONCE(dm_io_flagged(io, DM_IO_ACCOUNTED)))
> +                     return;

So this can be dropped ^

>               dm_io_set_flag(io, DM_IO_ACCOUNTED);
>       } else {
>               unsigned long flags;
>               /* Can afford locking given DM_TIO_IS_DUPLICATE_BIO */
>               spin_lock_irqsave(&io->lock, flags);
> +             if (dm_io_flagged(io, DM_IO_ACCOUNTED)) {
> +                     spin_unlock_irqrestore(&io->lock, flags);
> +                     return;
> +             }

Leaving only this part ^ as the necessary change to fix the race.

>               dm_io_set_flag(io, DM_IO_ACCOUNTED);
>               spin_unlock_irqrestore(&io->lock, flags);
>       }

Now applied and staged in linux-next for v5.19-rc3 (or later).  I
won't rebase so this commit will remain valid:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.19&id=10eb3a0d517fcc83eeea4242c149461205675eb4

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Reply via email to