[PATCH v2] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes

[PATCH v2] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes

[Johannes Thumshirn]

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 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..a4d12ada0565 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 != 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,
-- 
2.16.4

Re: [PATCH v2] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes

[Hans van Kranenburg]

On 2/18/19 10:48 AM, 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>
> ---
> 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..a4d12ada0565 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 != 1) ||

I think you meant != 2 here. It's just like DUP, but with the two of
them on different devices instead of the same.

>  	    (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,
> 

Thanks,

-- 
Hans van Kranenburg

Re: [PATCH v2] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes

[Johannes Thumshirn]

On 18/02/2019 10:55, Hans van Kranenburg wrote:
> On 2/18/19 10:48 AM, 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>
>> ---
>> 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..a4d12ada0565 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 != 1) ||
> 
> I think you meant != 2 here. It's just like DUP, but with the two of
> them on different devices instead of the same.

Ah damn, thanks.




-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH v2.1] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes

[Johannes Thumshirn]

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

Re: [PATCH v2.1] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes

[Peter Becker]

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
>

Re: [PATCH v2.1] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes

[Nikolay Borisov]

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.

Re: [PATCH v2.1] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes

[David Sterba]

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.

Re: [PATCH v2.1] btrfs: ensure that a DUP or RAID1 block group has exactly two stripes

[David Sterba]

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.