[PATCH] btrfs: handle missing chunk mapping more gracefully

[PATCH] btrfs: handle missing chunk mapping more gracefully

[Qu Wenruo]

[BUG]
During my scrub rework, I did a stupid thing like this:

        bio->bi_iter.bi_sector = stripe->logical;
        btrfs_submit_bio(fs_info, bio, stripe->mirror_num);

Above bi_sector assignment is using logical address directly, which
lacks ">> SECTOR_SHIFT".

This results a read on a range which has no chunk mapping.

This results the following crash:

 BTRFS critical (device dm-1): unable to find logical 11274289152 length 65536
 assertion failed: !IS_ERR(em), in fs/btrfs/volumes.c:6387
 ------------[ cut here ]------------

Sure this is all my fault, but this shows a possible problem in real
world, that some bitflip in file extents/tree block can point to
unmapped ranges, and trigger above ASSERT().

[PROBLEMS]
In above call chain, there are 2 locations not properly handling the
errors:

- __btrfs_map_block()
  If btrfs_get_chunk_map() returned error, we just trigger an ASSERT().

- btrfs_submit_bio()
  If the returned mapped length is smaller than expected, we just BUG().

[FIX]
This patch will fix the problems by:

- Add extra WARN_ON() if btrfs_get_chunk_map() failed
  I know syzbot will complain, but it's better noisy for fstests.

- Replace the ASSERT()
  By returning the error.

- Handle the error when mapped length is smaller than expected length

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/bio.c     | 6 +++++-
 fs/btrfs/volumes.c | 5 ++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index b8fb7ef6b520..8f7b56a0290f 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -246,7 +246,11 @@ void btrfs_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio, int mirror
 		btrfs_crit(fs_info,
 			   "mapping failed logical %llu bio len %llu len %llu",
 			   logical, length, map_length);
-		BUG();
+		WARN_ON(1);
+		ret = -EINVAL;
+		btrfs_bio_counter_dec(fs_info);
+		btrfs_bio_end_io(btrfs_bio(bio), errno_to_blk_status(ret));
+		return;
 	}
 
 	if (!bioc) {
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index aa25fa335d3e..f69475fb1bc1 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3012,6 +3012,7 @@ struct extent_map *btrfs_get_chunk_map(struct btrfs_fs_info *fs_info,
 	if (!em) {
 		btrfs_crit(fs_info, "unable to find logical %llu length %llu",
 			   logical, length);
+		WARN_ON(1);
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -3020,6 +3021,7 @@ struct extent_map *btrfs_get_chunk_map(struct btrfs_fs_info *fs_info,
 			   "found a bad mapping, wanted %llu-%llu, found %llu-%llu",
 			   logical, length, em->start, em->start + em->len);
 		free_extent_map(em);
+		WARN_ON(1);
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -6384,7 +6386,8 @@ int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	ASSERT(op != BTRFS_MAP_DISCARD);
 
 	em = btrfs_get_chunk_map(fs_info, logical, *length);
-	ASSERT(!IS_ERR(em));
+	if (IS_ERR(em))
+		return PTR_ERR(em);
 
 	ret = btrfs_get_io_geometry(fs_info, em, op, logical, &geom);
 	if (ret < 0)
-- 
2.38.1

Re: [PATCH] btrfs: handle missing chunk mapping more gracefully

[Boris Burkov]

On Wed, Dec 07, 2022 at 01:09:59PM +0800, Qu Wenruo wrote:
> [BUG]
> During my scrub rework, I did a stupid thing like this:
> 
>         bio->bi_iter.bi_sector = stripe->logical;
>         btrfs_submit_bio(fs_info, bio, stripe->mirror_num);
> 
> Above bi_sector assignment is using logical address directly, which
> lacks ">> SECTOR_SHIFT".
> 
> This results a read on a range which has no chunk mapping.
> 
> This results the following crash:
> 
>  BTRFS critical (device dm-1): unable to find logical 11274289152 length 65536
>  assertion failed: !IS_ERR(em), in fs/btrfs/volumes.c:6387
>  ------------[ cut here ]------------
> 
> Sure this is all my fault, but this shows a possible problem in real
> world, that some bitflip in file extents/tree block can point to
> unmapped ranges, and trigger above ASSERT().
> 
> [PROBLEMS]
> In above call chain, there are 2 locations not properly handling the
> errors:
> 
> - __btrfs_map_block()
>   If btrfs_get_chunk_map() returned error, we just trigger an ASSERT().
> 
> - btrfs_submit_bio()
>   If the returned mapped length is smaller than expected, we just BUG().
> 
> [FIX]
> This patch will fix the problems by:
> 
> - Add extra WARN_ON() if btrfs_get_chunk_map() failed
>   I know syzbot will complain, but it's better noisy for fstests.
> 
> - Replace the ASSERT()
>   By returning the error.
> 
> - Handle the error when mapped length is smaller than expected length
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Looks good to me, you can add
Reviewed-by: Boris Burkov <boris@bur.io>

> ---
>  fs/btrfs/bio.c     | 6 +++++-
>  fs/btrfs/volumes.c | 5 ++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index b8fb7ef6b520..8f7b56a0290f 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -246,7 +246,11 @@ void btrfs_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio, int mirror
>  		btrfs_crit(fs_info,
>  			   "mapping failed logical %llu bio len %llu len %llu",
>  			   logical, length, map_length);

nit: for these WARN_ON(1)s, how about changing them from
if (cond) {
        btrfs_crit(<msg>);
        WARN_ON(1);
        <return error>;
}

to

if (WARN_ON(<cond>)) {
        btrfs_crit(<msg>);
	<return err>
}

> -		BUG();
> +		WARN_ON(1);
> +		ret = -EINVAL;
> +		btrfs_bio_counter_dec(fs_info);
> +		btrfs_bio_end_io(btrfs_bio(bio), errno_to_blk_status(ret));
> +		return;
>  	}
>  
>  	if (!bioc) {
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index aa25fa335d3e..f69475fb1bc1 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3012,6 +3012,7 @@ struct extent_map *btrfs_get_chunk_map(struct btrfs_fs_info *fs_info,
>  	if (!em) {
>  		btrfs_crit(fs_info, "unable to find logical %llu length %llu",
>  			   logical, length);
> +		WARN_ON(1);
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> @@ -3020,6 +3021,7 @@ struct extent_map *btrfs_get_chunk_map(struct btrfs_fs_info *fs_info,
>  			   "found a bad mapping, wanted %llu-%llu, found %llu-%llu",
>  			   logical, length, em->start, em->start + em->len);
>  		free_extent_map(em);
> +		WARN_ON(1);
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> @@ -6384,7 +6386,8 @@ int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>  	ASSERT(op != BTRFS_MAP_DISCARD);
>  
>  	em = btrfs_get_chunk_map(fs_info, logical, *length);
> -	ASSERT(!IS_ERR(em));
> +	if (IS_ERR(em))
> +		return PTR_ERR(em);
>  
>  	ret = btrfs_get_io_geometry(fs_info, em, op, logical, &geom);
>  	if (ret < 0)
> -- 
> 2.38.1

Re: [PATCH] btrfs: handle missing chunk mapping more gracefully

[Qu Wenruo]

On 2023/1/28 04:10, Boris Burkov wrote:
> On Wed, Dec 07, 2022 at 01:09:59PM +0800, Qu Wenruo wrote:
>> [BUG]
>> During my scrub rework, I did a stupid thing like this:
>>
>>          bio->bi_iter.bi_sector = stripe->logical;
>>          btrfs_submit_bio(fs_info, bio, stripe->mirror_num);
>>
>> Above bi_sector assignment is using logical address directly, which
>> lacks ">> SECTOR_SHIFT".
>>
>> This results a read on a range which has no chunk mapping.
>>
>> This results the following crash:
>>
>>   BTRFS critical (device dm-1): unable to find logical 11274289152 length 65536
>>   assertion failed: !IS_ERR(em), in fs/btrfs/volumes.c:6387
>>   ------------[ cut here ]------------
>>
>> Sure this is all my fault, but this shows a possible problem in real
>> world, that some bitflip in file extents/tree block can point to
>> unmapped ranges, and trigger above ASSERT().
>>
>> [PROBLEMS]
>> In above call chain, there are 2 locations not properly handling the
>> errors:
>>
>> - __btrfs_map_block()
>>    If btrfs_get_chunk_map() returned error, we just trigger an ASSERT().
>>
>> - btrfs_submit_bio()
>>    If the returned mapped length is smaller than expected, we just BUG().
>>
>> [FIX]
>> This patch will fix the problems by:
>>
>> - Add extra WARN_ON() if btrfs_get_chunk_map() failed
>>    I know syzbot will complain, but it's better noisy for fstests.
>>
>> - Replace the ASSERT()
>>    By returning the error.
>>
>> - Handle the error when mapped length is smaller than expected length
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Looks good to me, you can add
> Reviewed-by: Boris Burkov <boris@bur.io>
> 
>> ---
>>   fs/btrfs/bio.c     | 6 +++++-
>>   fs/btrfs/volumes.c | 5 ++++-
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
>> index b8fb7ef6b520..8f7b56a0290f 100644
>> --- a/fs/btrfs/bio.c
>> +++ b/fs/btrfs/bio.c
>> @@ -246,7 +246,11 @@ void btrfs_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio, int mirror
>>   		btrfs_crit(fs_info,
>>   			   "mapping failed logical %llu bio len %llu len %llu",
>>   			   logical, length, map_length);
> 
> nit: for these WARN_ON(1)s, how about changing them from
> if (cond) {
>          btrfs_crit(<msg>);
>          WARN_ON(1);
>          <return error>;
> }
> 
> to
> 
> if (WARN_ON(<cond>)) {
>          btrfs_crit(<msg>);
> 	<return err>
> }

In fact, the behavior is discouraged by David IIRC.

The problem is, one has to rely on the fact WARN_ON() has a return 
value, which is not straightforward by a first glance.

Thanks,
Qu
> 
>> -		BUG();
>> +		WARN_ON(1);
>> +		ret = -EINVAL;
>> +		btrfs_bio_counter_dec(fs_info);
>> +		btrfs_bio_end_io(btrfs_bio(bio), errno_to_blk_status(ret));
>> +		return;
>>   	}
>>   
>>   	if (!bioc) {
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index aa25fa335d3e..f69475fb1bc1 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -3012,6 +3012,7 @@ struct extent_map *btrfs_get_chunk_map(struct btrfs_fs_info *fs_info,
>>   	if (!em) {
>>   		btrfs_crit(fs_info, "unable to find logical %llu length %llu",
>>   			   logical, length);
>> +		WARN_ON(1);
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> @@ -3020,6 +3021,7 @@ struct extent_map *btrfs_get_chunk_map(struct btrfs_fs_info *fs_info,
>>   			   "found a bad mapping, wanted %llu-%llu, found %llu-%llu",
>>   			   logical, length, em->start, em->start + em->len);
>>   		free_extent_map(em);
>> +		WARN_ON(1);
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> @@ -6384,7 +6386,8 @@ int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>>   	ASSERT(op != BTRFS_MAP_DISCARD);
>>   
>>   	em = btrfs_get_chunk_map(fs_info, logical, *length);
>> -	ASSERT(!IS_ERR(em));
>> +	if (IS_ERR(em))
>> +		return PTR_ERR(em);
>>   
>>   	ret = btrfs_get_io_geometry(fs_info, em, op, logical, &geom);
>>   	if (ret < 0)
>> -- 
>> 2.38.1

Re: [PATCH] btrfs: handle missing chunk mapping more gracefully

[Anand Jain]

LGTM.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


Nit:

> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index b8fb7ef6b520..8f7b56a0290f 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -246,7 +246,11 @@ void btrfs_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio, int mirror
>   		btrfs_crit(fs_info,
>   			   "mapping failed logical %llu bio len %llu len %llu",
>   			   logical, length, map_length);
> -		BUG();
> +		WARN_ON(1);
> +		ret = -EINVAL;
> +		btrfs_bio_counter_dec(fs_info);
> +		btrfs_bio_end_io(btrfs_bio(bio), errno_to_blk_status(ret));
> +		return;
>   	}

After this patch, the duplicate code lines can be consolidated to:


diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 64268278bf8c..d13825a4ea7c 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -236,11 +236,8 @@ void btrfs_submit_bio(struct btrfs_fs_info 
*fs_info, struct bio *bio, int mirror
         btrfs_bio_counter_inc_blocked(fs_info);
         ret = __btrfs_map_block(fs_info, btrfs_op(bio), logical, 
&map_length,
                                 &bioc, &smap, &mirror_num, 1);
-       if (ret) {
-               btrfs_bio_counter_dec(fs_info);
-               btrfs_bio_end_io(btrfs_bio(bio), errno_to_blk_status(ret));
-               return;
-       }
+       if (ret)
+               goto err_out;

         if (map_length < length) {
                 btrfs_crit(fs_info,
@@ -248,9 +245,7 @@ void btrfs_submit_bio(struct btrfs_fs_info *fs_info, 
struct bio *bio, int mirror
                            logical, length, map_length);
                 WARN_ON(1);
                 ret = -EINVAL;
-               btrfs_bio_counter_dec(fs_info);
-               btrfs_bio_end_io(btrfs_bio(bio), errno_to_blk_status(ret));
-               return;
+               goto err_out;
         }

         if (!bioc) {
@@ -278,6 +273,13 @@ void btrfs_submit_bio(struct btrfs_fs_info 
*fs_info, struct bio *bio, int mirror
                 for (dev_nr = 0; dev_nr < total_devs; dev_nr++)
                         btrfs_submit_mirrored_bio(bioc, dev_nr);
         }
+
+       return;
+
+err_out:
+       btrfs_bio_counter_dec(fs_info);
+       btrfs_bio_end_io(btrfs_bio(bio), errno_to_blk_status(ret));
+       return;
  }

  /*

Re: [PATCH] btrfs: handle missing chunk mapping more gracefully

[David Sterba]

On Wed, Dec 07, 2022 at 01:09:59PM +0800, Qu Wenruo wrote:
> [BUG]
> During my scrub rework, I did a stupid thing like this:
> 
>         bio->bi_iter.bi_sector = stripe->logical;
>         btrfs_submit_bio(fs_info, bio, stripe->mirror_num);
> 
> Above bi_sector assignment is using logical address directly, which
> lacks ">> SECTOR_SHIFT".
> 
> This results a read on a range which has no chunk mapping.
> 
> This results the following crash:
> 
>  BTRFS critical (device dm-1): unable to find logical 11274289152 length 65536
>  assertion failed: !IS_ERR(em), in fs/btrfs/volumes.c:6387
>  ------------[ cut here ]------------
> 
> Sure this is all my fault, but this shows a possible problem in real
> world, that some bitflip in file extents/tree block can point to
> unmapped ranges, and trigger above ASSERT().
> 
> [PROBLEMS]
> In above call chain, there are 2 locations not properly handling the
> errors:
> 
> - __btrfs_map_block()
>   If btrfs_get_chunk_map() returned error, we just trigger an ASSERT().
> 
> - btrfs_submit_bio()
>   If the returned mapped length is smaller than expected, we just BUG().
> 
> [FIX]
> This patch will fix the problems by:
> 
> - Add extra WARN_ON() if btrfs_get_chunk_map() failed
>   I know syzbot will complain, but it's better noisy for fstests.
> 
> - Replace the ASSERT()
>   By returning the error.
> 
> - Handle the error when mapped length is smaller than expected length
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

This does not apply since the recent bio and checksum rework, please
resend, thanks.