| Message ID | 20190213142602.20436-1-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: ensure that a DUP block group has exactly two stripes | expand |
On 13.02.19 г. 16:26 ч., Johannes Thumshirn wrote: > We recently had a customer issue with a corrupted filesystem. When trying > to mount this image btrfs panicked with a division by zero in > calc_stripe_length(). > > The corrupt chunk had a 'num_stripes' value of 1. calc_stripe_length() > takes this value and divides it by the number of copies the RAID profile is > expected to have to calculate the amount of data stripes. As a DUP profile > is expected to have 2 copies this division resulted in 1/2 = 0. Later then > the 'data_stripes' variable is used as a divisor in the stripe length > calculation which results in a division by 0 and thus a kernel panic. > > When encountering a filesystem with a DUP block group and a 'num_stripes' > value unequal to 2, refuse mounting as the image is corrupted and will lead > to unexpected behaviour. > > Fixes: e06cd3dd7cea ("Btrfs: add validadtion checks for chunk loading") > Cc: Liu Bo <obuil.liubo@gmail.com> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/volumes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 03f223aa7194..b40cc7c830f4 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -6794,7 +6794,7 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, > (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || > (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) || > (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) || > - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) || > + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) || > ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 && > num_stripes != 1)) { > btrfs_err(fs_info, >
On 2019/2/13 下午10:26, Johannes Thumshirn wrote: > We recently had a customer issue with a corrupted filesystem. When trying > to mount this image btrfs panicked with a division by zero in > calc_stripe_length(). > > The corrupt chunk had a 'num_stripes' value of 1. calc_stripe_length() > takes this value and divides it by the number of copies the RAID profile is > expected to have to calculate the amount of data stripes. As a DUP profile > is expected to have 2 copies this division resulted in 1/2 = 0. Later then > the 'data_stripes' variable is used as a divisor in the stripe length > calculation which results in a division by 0 and thus a kernel panic. > > When encountering a filesystem with a DUP block group and a 'num_stripes' > value unequal to 2, refuse mounting as the image is corrupted and will lead > to unexpected behaviour. > > Fixes: e06cd3dd7cea ("Btrfs: add validadtion checks for chunk loading") > Cc: Liu Bo <obuil.liubo@gmail.com> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/volumes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 03f223aa7194..b40cc7c830f4 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -6794,7 +6794,7 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, > (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || > (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) || > (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) || > - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) || > + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) || > ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 && > num_stripes != 1)) { > btrfs_err(fs_info, >
On 2/13/19 3:26 PM, Johannes Thumshirn wrote: > We recently had a customer issue with a corrupted filesystem. When trying > to mount this image btrfs panicked with a division by zero in > calc_stripe_length(). > > The corrupt chunk had a 'num_stripes' value of 1. calc_stripe_length() > takes this value and divides it by the number of copies the RAID profile is > expected to have to calculate the amount of data stripes. As a DUP profile > is expected to have 2 copies this division resulted in 1/2 = 0. Later then > the 'data_stripes' variable is used as a divisor in the stripe length > calculation which results in a division by 0 and thus a kernel panic. > > When encountering a filesystem with a DUP block group and a 'num_stripes' > value unequal to 2, refuse mounting as the image is corrupted and will lead > to unexpected behaviour. > > Fixes: e06cd3dd7cea ("Btrfs: add validadtion checks for chunk loading") > Cc: Liu Bo <obuil.liubo@gmail.com> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > --- > fs/btrfs/volumes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 03f223aa7194..b40cc7c830f4 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -6794,7 +6794,7 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, > (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || > (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) || > (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) || > - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) || > + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) || > ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 && > num_stripes != 1)) { > btrfs_err(fs_info, > It looks like the RAID1 check has a similar problem. Shouldn't that check also be != 2 ? Hans
On 13/02/2019 15:32, Hans van Kranenburg wrote: [...] >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 03f223aa7194..b40cc7c830f4 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -6794,7 +6794,7 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, >> (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || >> (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) || >> (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) || >> - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) || >> + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) || >> ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 && >> num_stripes != 1)) { >> btrfs_err(fs_info, >> > > It looks like the RAID1 check has a similar problem. Shouldn't that > check also be != 2 ? Hmm I guess a degraded RAID1 can have only 1 stripe, doesn't it?
On 13.02.19 г. 16:37 ч., Johannes Thumshirn wrote: > On 13/02/2019 15:32, Hans van Kranenburg wrote: > [...] > >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index 03f223aa7194..b40cc7c830f4 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -6794,7 +6794,7 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, >>> (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || >>> (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) || >>> (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) || >>> - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) || >>> + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) || >>> ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 && >>> num_stripes != 1)) { >>> btrfs_err(fs_info, >>> >> >> It looks like the RAID1 check has a similar problem. Shouldn't that >> check also be != 2 ? > > Hmm I guess a degraded RAID1 can have only 1 stripe, doesn't it? It depends on the POV. root@ubuntu-virtual:~# fallocate -l 2g /media/scratch/disk1.img root@ubuntu-virtual:~# fallocate -l 2g /media/scratch/disk2.img root@ubuntu-virtual:~# losetup -f /media/scratch/disk1.img root@ubuntu-virtual:~# losetup -f /media/scratch/disk2.img root@ubuntu-virtual:~# mkfs.btrfs -draid1 /dev/loop0 /dev/loop1 root@ubuntu-virtual:~# losetup -d /dev/loop1 root@ubuntu-virtual:~# mount -odegraded /dev/loop0 /media/test/ btrfs inspect-internal dump-tree -t chunk /dev/loop0 | grep -B1 -A5 DATA\|RAID1 item 3 key (FIRST_CHUNK_TREE CHUNK_ITEM 30408704) itemoff 15863 itemsize 112 length 214695936 owner 2 stripe_len 65536 type METADATA|RAID1 io_align 65536 io_width 65536 sector_size 4096 num_stripes 2 sub_stripes 0 stripe 0 devid 2 offset 9437184 dev_uuid adb924e5-830b-4eb0-b28a-f5bee6b709ca stripe 1 devid 1 offset 30408704 -- item 4 key (FIRST_CHUNK_TREE CHUNK_ITEM 245104640) itemoff 15751 itemsize 112 length 214695936 owner 2 stripe_len 65536 type DATA|RAID1 io_align 65536 io_width 65536 sector_size 4096 num_stripes 2 sub_stripes 0 stripe 0 devid 2 offset 224133120 dev_uuid adb924e5-830b-4eb0-b28a-f5bee6b709ca stripe 1 devid 1 offset 245104640 DMESG: [78623.920424] BTRFS warning (device loop0): devid 2 uuid adb924e5-830b-4eb0-b28a-f5bee6b709ca is missing > >
On 13/02/2019 15:32, Hans van Kranenburg wrote: [...] >> +++ b/fs/btrfs/volumes.c >> @@ -6794,7 +6794,7 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, >> (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || >> (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) || >> (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) || >> - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) || >> + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) || >> ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 && >> num_stripes != 1)) { >> btrfs_err(fs_info, >> > > It looks like the RAID1 check has a similar problem. Shouldn't that > check also be != 2 ? So looking at the code again I think num_stripes == 1 for RAID1 will result in the same division by 0 in calc_stripe_length(). I'll send a patch for RAID1 as well unless David speaks up and says he wants it amended in this one. Thanks, Johannes
On 2/13/19 3:37 PM, Johannes Thumshirn wrote: > On 13/02/2019 15:32, Hans van Kranenburg wrote: > [...] > >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index 03f223aa7194..b40cc7c830f4 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -6794,7 +6794,7 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, >>> (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || >>> (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) || >>> (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) || >>> - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) || >>> + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) || >>> ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 && >>> num_stripes != 1)) { >>> btrfs_err(fs_info, >>> >> >> It looks like the RAID1 check has a similar problem. Shouldn't that >> check also be != 2 ? > > Hmm I guess a degraded RAID1 can have only 1 stripe, doesn't it? It's reading the metadata from disk here. So, there are always still exactly two 'stripe' structs inside the chunk item data. Hans
On Thu, Feb 14, 2019 at 05:21:54PM +0100, Johannes Thumshirn wrote: > On 13/02/2019 15:32, Hans van Kranenburg wrote: > [...] > > >> +++ b/fs/btrfs/volumes.c > >> @@ -6794,7 +6794,7 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, > >> (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || > >> (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) || > >> (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) || > >> - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) || > >> + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) || > >> ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 && > >> num_stripes != 1)) { > >> btrfs_err(fs_info, > >> > > > > It looks like the RAID1 check has a similar problem. Shouldn't that > > check also be != 2 ? > > So looking at the code again I think num_stripes == 1 for RAID1 will > result in the same division by 0 in calc_stripe_length(). > > I'll send a patch for RAID1 as well unless David speaks up and says he > wants it amended in this one. If the explanation and cause is the same, it's ok to put it into one patch I think, but no strong preference here.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 03f223aa7194..b40cc7c830f4 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -6794,7 +6794,7 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) || (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) || - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) || + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) || ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 && num_stripes != 1)) { btrfs_err(fs_info,
We recently had a customer issue with a corrupted filesystem. When trying to mount this image btrfs panicked with a division by zero in calc_stripe_length(). The corrupt chunk had a 'num_stripes' value of 1. calc_stripe_length() takes this value and divides it by the number of copies the RAID profile is expected to have to calculate the amount of data stripes. As a DUP profile is expected to have 2 copies this division resulted in 1/2 = 0. Later then the 'data_stripes' variable is used as a divisor in the stripe length calculation which results in a division by 0 and thus a kernel panic. When encountering a filesystem with a DUP block group and a 'num_stripes' value unequal to 2, refuse mounting as the image is corrupted and will lead to unexpected behaviour. Fixes: e06cd3dd7cea ("Btrfs: add validadtion checks for chunk loading") Cc: Liu Bo <obuil.liubo@gmail.com> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- fs/btrfs/volumes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)