| Message ID | 20190218102837.15219-1-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2.1] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes | expand |
I would prefer "< 2" .. in both cases (RAID1 and DUP) because this
allow us to implement N-Copy RAID1 in the future.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 03f223aa7194..9024eee889b9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6791,10 +6791,10 @@ static int btrfs_check_chunk_valid(struct
btrfs_fs_info *fs_info,
}
if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
- (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
+ (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 2) ||
(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,
Am Mo., 18. Feb. 2019 um 12:24 Uhr schrieb Johannes Thumshirn
<jthumshirn@suse.de>:
>
> 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.
>
> Code inspection showed a RAID1 block group has the same issues.
>
> Fixes: e06cd3dd7cea ("Btrfs: add validadtion checks for chunk loading")
> Cc: Liu Bo <obuil.liubo@gmail.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
> Changes to v2:
> - Really use 2 stripes not 1 (Hans)
> Changes to v1:
> - Also add the check for RAID1 (Hans)
> ---
> fs/btrfs/volumes.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 03f223aa7194..9024eee889b9 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6791,10 +6791,10 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
> }
>
> if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
> - (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
> + (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes != 2) ||
> (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,
> --
> 2.16.4
>
On 19.02.19 г. 9:33 ч., Peter Becker wrote: > I would prefer "< 2" .. in both cases (RAID1 and DUP) because this > allow us to implement N-Copy RAID1 in the future. NAK When/if N-Copy raid1 patches land then they can modify the code and it will be visible from the commit message why it's < 2. For now we only support ordinary raid1, hence the code should do just enough to support the current design. Even now there are artifacts of similar "future proofing" for patches that never landed i.e dedupe stuff and pointless args to extent_clear_unlock_delalloc and some other functions.
On Tue, Feb 19, 2019 at 08:33:23AM +0100, Peter Becker wrote: > I would prefer "< 2" .. in both cases (RAID1 and DUP) because this > allow us to implement N-Copy RAID1 in the future. The n-copy raid has or will have own block group type so there will be a separate check for the constraints.
On Mon, Feb 18, 2019 at 11:28:37AM +0100, 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. > > Code inspection showed a RAID1 block group has the same issues. > > Fixes: e06cd3dd7cea ("Btrfs: add validadtion checks for chunk loading") > Cc: Liu Bo <obuil.liubo@gmail.com> > Reviewed-by: Qu Wenruo <wqu@suse.com> > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> Added to misc-next, thanks.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 03f223aa7194..9024eee889b9 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -6791,10 +6791,10 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info, } if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) || - (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) || + (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes != 2) || (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. Code inspection showed a RAID1 block group has the same issues. Fixes: e06cd3dd7cea ("Btrfs: add validadtion checks for chunk loading") Cc: Liu Bo <obuil.liubo@gmail.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- Changes to v2: - Really use 2 stripes not 1 (Hans) Changes to v1: - Also add the check for RAID1 (Hans) --- fs/btrfs/volumes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)