| Message ID | 7ff90508841683ca3aaeb5c84e27d7d823218146.1670389796.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: handle missing chunk mapping more gracefully | expand |
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
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
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; } /*
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.
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)
[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(-)