Re: snapshot-origin with no snapshot may lead to BUG() in bio_split()

[Mike Snitzer <snitzer@redhat.com>]

On Sat, Jul 20 2019 at  5:26am -0400,
Cédric Delmas <cedricde@outlook.fr> wrote:

> Hello,
> 
> I encountered a bug while working with DM snapshot targets: having a
> snapshot-origin target with all snapshots removed may lead to
> BUG_ON(sectors <= 0) in function bio_split() (file block/bio.c).

...
 
> Steps to reproduce:
> truncate -s 500M origin.bin
> truncate -s 50M snapshot.bin
> losetup /dev/loop0 origin.bin
> losetup /dev/loop1 snapshot.bin
> mkfs.ext4 /dev/loop0
> dmsetup create snap --table "0 $(blockdev --getsz /dev/loop0) snapshot /dev/loop0 /dev/loop1 N 256"
> dmsetup create orig --table "0 $(blockdev --getsz /dev/loop0) snapshot-origin /dev/loop0"
> # use /dev/mapper/snap and /dev/mapper/orig then unmount them
> dmsetup suspend orig
> dmsetup remove snap
> dmsetup resume orig
> e2fsck /dev/mapper/orig
> # BUG in bio_split()
> 
> Steps to reproduce (the express way):
> truncate -s 500M origin.bin
> losetup /dev/loop0 origin.bin
> mkfs.ext4 /dev/loop0
> dmsetup create orig --table "0 $(blockdev --getsz /dev/loop0) snapshot-origin /dev/loop0"
> e2fsck /dev/mapper/orig
> # BUG in bio_split()
> 
> 
> I looked at the code and to my opinion the problem comes from function origin_map (file drivers/md/dm-snap.c). In the following code:
> 
> static int origin_map(struct dm_target *ti, struct bio *bio)
> {
> 	struct dm_origin *o = ti->private;
> 	unsigned available_sectors;
> ...
> 	available_sectors = o->split_boundary -
> 		((unsigned)bio->bi_iter.bi_sector & (o->split_boundary - 1));
> 
> 	if (bio_sectors(bio) > available_sectors)
> 		dm_accept_partial_bio(bio, available_sectors);
> ...
> 
> when there is no snapshot, split_boundary is 0 so available_sectors gets an invalid value.
> The problem no more appears if the function origin_map early exits using the following patch:
> --- a/drivers/md/dm-snap.c      2019-07-14 08:11:23.000000000 +0200
> +++ b/drivers/md/dm-snap.c      2019-07-19 17:50:15.876000000 +0200
> @@ -2328,6 +2328,9 @@ static int origin_map(struct dm_target *
>         if (bio_data_dir(bio) != WRITE)
>                 return DM_MAPIO_REMAPPED;
>  
> +       if (unlikely(!o->split_boundary))
> +               return do_origin(o->dev, bio);
> +
>         available_sectors = o->split_boundary -
>                 ((unsigned)bio->bi_iter.bi_sector & (o->split_boundary - 1));
>  

When there is no snapshot snapshot-origin shouldn't be used.

So your patch may fix the BUG() you hit but it doesn't go far enough
with warning the user that they've entered "unsupported" territory.

Rather than call do_origin() I'm inclined to
DMERR_LIMIT("... unsupported ...") and error the IO.

What are your reasons for wanting to silently allow this unsupported
usecase?

Mike