diff mbox series

btrfs: btrfs: don't trust sub_stripes from disk

Message ID 90e84962486d7ab5a8bca92e329fe3ee6864680f.1666312963.git.wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: btrfs: don't trust sub_stripes from disk | expand

Commit Message

Qu Wenruo Oct. 21, 2022, 12:43 a.m. UTC
[BUG]
There are two reports (the earliest one from LKP, a more recent one from
kernel bugzilla) that we can have some chunks with 0 as sub_stripes.

This will cause divide-by-zero errors at btrfs_rmap_block, which is
introduced by a recent kernel patch ac0677348f3c ("btrfs: merge
calculations for simple striped profiles in btrfs_rmap_block"):

		if (map->type & (BTRFS_BLOCK_GROUP_RAID0 |
				 BTRFS_BLOCK_GROUP_RAID10)) {
			stripe_nr = stripe_nr * map->num_stripes + i;
			stripe_nr = div_u64(stripe_nr, map->sub_stripes); <<<
		}

[CAUSE]
From the more recent report, it has been proven that we have some chunks
with 0 as sub_stripes, mostly caused by older mkfs.

It turns out that the mkfs.btrfs fix is only introduced in 6718ab4d33aa
("btrfs-progs: Initialize sub_stripes to 1 in btrfs_alloc_data_chunk")
which is included in v5.4 btrfs-progs release.

So there would be quite some old fses with such 0 sub_stripes.

[FIX]
Just don't trust the sub_stripes values from disk.

We have a trusted btrfs_raid_array[] to fetch the correct sub_stripes
numbers for each profile.

By this, we can keep the compatibility with older fses while still avoid
divide-by-zero bugs.

Fixes: ac0677348f3c ("btrfs: merge calculations for simple striped profiles in btrfs_rmap_block")
Reported-by: kernel test robot <oliver.sang@intel.com>
Reported-by: Viktor Kuzmin <kvaster@gmail.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216559
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Su Yue Oct. 21, 2022, 12:47 a.m. UTC | #1
On 2022/10/21 08:44, Qu Wenruo wrote:
> [BUG]
> There are two reports (the earliest one from LKP, a more recent one from
> kernel bugzilla) that we can have some chunks with 0 as sub_stripes.
> 
> This will cause divide-by-zero errors at btrfs_rmap_block, which is
> introduced by a recent kernel patch ac0677348f3c ("btrfs: merge
> calculations for simple striped profiles in btrfs_rmap_block"):
> 
> 		if (map->type & (BTRFS_BLOCK_GROUP_RAID0 |
> 				 BTRFS_BLOCK_GROUP_RAID10)) {
> 			stripe_nr = stripe_nr * map->num_stripes + i;
> 			stripe_nr = div_u64(stripe_nr, map->sub_stripes); <<<
> 		}
> 
> [CAUSE]
>  From the more recent report, it has been proven that we have some chunks
> with 0 as sub_stripes, mostly caused by older mkfs.
> 
> It turns out that the mkfs.btrfs fix is only introduced in 6718ab4d33aa
> ("btrfs-progs: Initialize sub_stripes to 1 in btrfs_alloc_data_chunk")
> which is included in v5.4 btrfs-progs release.
> 
> So there would be quite some old fses with such 0 sub_stripes.
> 
> [FIX]
> Just don't trust the sub_stripes values from disk.
> 
> We have a trusted btrfs_raid_array[] to fetch the correct sub_stripes
> numbers for each profile.
> 
> By this, we can keep the compatibility with older fses while still avoid
> divide-by-zero bugs.
> 
> Fixes: ac0677348f3c ("btrfs: merge calculations for simple striped profiles in btrfs_rmap_block")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Reported-by: Viktor Kuzmin <kvaster@gmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216559
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---

LGTM,
Reviewed-by: Su Yue <glass@fydeos.io>
>   fs/btrfs/volumes.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 94ba46d57920..39588cb9a7b6 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7142,6 +7142,7 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
>   	u64 devid;
>   	u64 type;
>   	u8 uuid[BTRFS_UUID_SIZE];
> +	int index;
>   	int num_stripes;
>   	int ret;
>   	int i;
> @@ -7149,6 +7150,7 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
>   	logical = key->offset;
>   	length = btrfs_chunk_length(leaf, chunk);
>   	type = btrfs_chunk_type(leaf, chunk);
> +	index = btrfs_bg_flags_to_raid_index(type);
>   	num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
>   
>   #if BITS_PER_LONG == 32
> @@ -7202,7 +7204,14 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
>   	map->io_align = btrfs_chunk_io_align(leaf, chunk);
>   	map->stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
>   	map->type = type;
> -	map->sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
> +	/*
> +	 * Don't trust the sub_stripes value, as for profiles other
> +	 * than RAID10, they may have 0 as sub_stripes for older mkfs.
> +	 * In that case, it can cause divide-by-zero errors later.
> +	 * Since currently sub_stripes is fixed for each profile, let's
> +	 * use the trusted value instead.
> +	 */
> +	map->sub_stripes = btrfs_raid_array[index].sub_stripes;
>   	map->verified_stripes = 0;
>   	em->orig_block_len = btrfs_calc_stripe_length(em);
>   	for (i = 0; i < num_stripes; i++) {
Johannes Thumshirn Oct. 21, 2022, 8:05 a.m. UTC | #2
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
David Sterba Oct. 24, 2022, 2:18 p.m. UTC | #3
On Fri, Oct 21, 2022 at 08:43:45AM +0800, Qu Wenruo wrote:
> [BUG]
> There are two reports (the earliest one from LKP, a more recent one from
> kernel bugzilla) that we can have some chunks with 0 as sub_stripes.
> 
> This will cause divide-by-zero errors at btrfs_rmap_block, which is
> introduced by a recent kernel patch ac0677348f3c ("btrfs: merge
> calculations for simple striped profiles in btrfs_rmap_block"):
> 
> 		if (map->type & (BTRFS_BLOCK_GROUP_RAID0 |
> 				 BTRFS_BLOCK_GROUP_RAID10)) {
> 			stripe_nr = stripe_nr * map->num_stripes + i;
> 			stripe_nr = div_u64(stripe_nr, map->sub_stripes); <<<
> 		}
> 
> [CAUSE]
> >From the more recent report, it has been proven that we have some chunks
> with 0 as sub_stripes, mostly caused by older mkfs.
> 
> It turns out that the mkfs.btrfs fix is only introduced in 6718ab4d33aa
> ("btrfs-progs: Initialize sub_stripes to 1 in btrfs_alloc_data_chunk")
> which is included in v5.4 btrfs-progs release.
> 
> So there would be quite some old fses with such 0 sub_stripes.
> 
> [FIX]
> Just don't trust the sub_stripes values from disk.
> 
> We have a trusted btrfs_raid_array[] to fetch the correct sub_stripes
> numbers for each profile.
> 
> By this, we can keep the compatibility with older fses while still avoid
> divide-by-zero bugs.
> 
> Fixes: ac0677348f3c ("btrfs: merge calculations for simple striped profiles in btrfs_rmap_block")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Reported-by: Viktor Kuzmin <kvaster@gmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216559
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to misc-next, thanks.
diff mbox series

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 94ba46d57920..39588cb9a7b6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7142,6 +7142,7 @@  static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
 	u64 devid;
 	u64 type;
 	u8 uuid[BTRFS_UUID_SIZE];
+	int index;
 	int num_stripes;
 	int ret;
 	int i;
@@ -7149,6 +7150,7 @@  static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
 	logical = key->offset;
 	length = btrfs_chunk_length(leaf, chunk);
 	type = btrfs_chunk_type(leaf, chunk);
+	index = btrfs_bg_flags_to_raid_index(type);
 	num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
 
 #if BITS_PER_LONG == 32
@@ -7202,7 +7204,14 @@  static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
 	map->io_align = btrfs_chunk_io_align(leaf, chunk);
 	map->stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
 	map->type = type;
-	map->sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
+	/*
+	 * Don't trust the sub_stripes value, as for profiles other
+	 * than RAID10, they may have 0 as sub_stripes for older mkfs.
+	 * In that case, it can cause divide-by-zero errors later.
+	 * Since currently sub_stripes is fixed for each profile, let's
+	 * use the trusted value instead.
+	 */
+	map->sub_stripes = btrfs_raid_array[index].sub_stripes;
 	map->verified_stripes = 0;
 	em->orig_block_len = btrfs_calc_stripe_length(em);
 	for (i = 0; i < num_stripes; i++) {