Re: [PATCH v2 0/2] btrfs: scrub avoid use-after-free when chunk length is not 64K aligned

[Johannes Thumshirn <Johannes.Thumshirn@wdc.com>]

On 17.01.24 01:33, Qu Wenruo wrote:
> [Changelog]
> v2:
> - Split out the RST code change
>    So that backport can happen more smoothly.
>    Furthermore, the RST specific part is really just a small enhancement.
>    As RST would always do the btrfs_map_block(), even if we have a
>    corrupted extent item beyond chunk, it would be properly caught,
>    thus at most false alerts, no real use-after-free can happen after
>    the first patch.
> 
> - Slight update on the commit message of the first patch
>    Fix a copy-and-paste error of the number used to calculate the chunk
>    end.
>    Remove the RST scrub part, as we won't do any RST fix (although
>    it would still sliently fix RST, since both RST and regular scrub
>    share the same endio function)
> 
> There is a bug report about use-after-free during scrub and crash the
> system.
> It turns out to be a chunk whose lenght is not 64K aligned causing the
> problem.
> 
> The first patch would be the proper fix, needs to be backported to all
> kernel using newer scrub interface.
> 
> The 2nd patch is a small enhancement for RST scrub, inspired by the
> above bug, which doesn't really need to be backported.
> 
> Qu Wenruo (2):
>    btrfs: scrub: avoid use-after-free when chunk length is not 64K
>      aligned
>    btrfs: scrub: limit RST scrub to chunk boundary
> 
>   fs/btrfs/scrub.c | 36 +++++++++++++++++++++++++++++-------
>   1 file changed, 29 insertions(+), 7 deletions(-)
> 

For the series,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

One more thing I personally would add (as a 3rd patch that doesn't need 
to get backported to stable) is this:

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 0123d2728923..046fdf8f6773 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -1641,14 +1641,23 @@ static void scrub_reset_stripe(struct 
scrub_stripe *stripe)
         }
  }

+static unsigned int scrub_nr_stripe_sectors(struct scrub_stripe *stripe)
+{
+       struct btrfs_fs_info *fs_info = stripe->bg->fs_info;
+       struct btrfs_block_group *bg = stripe->bg;
+       u64 bg_end = bg->start + bg->length;
+       unsigned int nr_sectors;
+
+       nr_sectors = min(BTRFS_STRIPE_LEN, bg_end - stripe->logical);
+       return nr_sectors >> fs_info->sectorsize_bits;
+}
+
  static void scrub_submit_extent_sector_read(struct scrub_ctx *sctx,
                                             struct scrub_stripe *stripe)
  {
         struct btrfs_fs_info *fs_info = stripe->bg->fs_info;
         struct btrfs_bio *bbio = NULL;
-       unsigned int nr_sectors = min(BTRFS_STRIPE_LEN, stripe->bg->start +
-                                     stripe->bg->length - 
stripe->logical) >>
-                                 fs_info->sectorsize_bits;
+       unsigned int nr_sectors = scrub_nr_stripe_sectors(stripe);
         u64 stripe_len = BTRFS_STRIPE_LEN;
         int mirror = stripe->mirror_num;
         int i;
@@ -1718,9 +1727,7 @@ static void scrub_submit_initial_read(struct 
scrub_ctx *sctx,
  {
         struct btrfs_fs_info *fs_info = sctx->fs_info;
         struct btrfs_bio *bbio;
-       unsigned int nr_sectors = min(BTRFS_STRIPE_LEN, stripe->bg->start +
-                                     stripe->bg->length - 
stripe->logical) >>
-                                 fs_info->sectorsize_bits;
+       unsigned int nr_sectors = scrub_nr_stripe_sectors(stripe);
         int mirror = stripe->mirror_num;

         ASSERT(stripe->bg);

Sorry for the complete whitespace damage, but I think you get the point.