[dm-devel] snapshot-origin with no snapshot may lead to BUG() in bio_split()

[Cédric Delmas]

Le 29/07/2019 à 16:38, Mike Snitzer a écrit :
> On Sat, Jul 20 2019 at  5:26am -0400,
> Cédric Delmas <cedricde at 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
> 

I didn't know that this usecase is unsupported, 
Documentation/device-mapper/snapshot.txt lets me think that even if the 
origin device should have one or more snapshots based on it, it is not 
mandatory. If this configuration is not supported, you are absolutely 
right, it is better to raise an error.

I think it could be nice to be able to permanently use a snapshot-origin 
device and to create snapshots only on demand (without forgetting to 
suspend the origin device during snapshot creation) however any 
correction or error notification is OK for me.

Cédric