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

[Boris Burkov <boris@bur.io>]

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