Re: [PATCH v2] btrfs: fix u32 overflows when left shifting @stripe_nr

[David Sterba <dsterba@suse.cz>]

On Tue, Jun 20, 2023 at 07:24:24PM +0800, Qu Wenruo wrote:
> On 2023/6/20 18:27, David Sterba wrote:
> > On Tue, Jun 20, 2023 at 05:57:31PM +0800, Qu Wenruo wrote:
> >> [BUG]
> >> David reported an ASSERT() get triggered during certain fio load.
> >>
> >> The ASSERT() is from rbio_add_bio() of raid56.c:
> >>
> >> 	ASSERT(orig_logical >= full_stripe_start &&
> >> 	       orig_logical + orig_len <= full_stripe_start +
> >> 	       rbio->nr_data * BTRFS_STRIPE_LEN);
> >>
> >> Which is checking if the target rbio is crossing the full stripe
> >> boundary.
> >>
> >> [CAUSE]
> >> Commit a97699d1d610 ("btrfs: replace map_lookup->stripe_len by
> >> BTRFS_STRIPE_LEN") changes how we calculate the map length, to reduce
> >> u64 division.
> >>
> >> Function btrfs_max_io_len() is to get the length to the stripe boundary.
> >>
> >> It calculates the full stripe start offset (inside the chunk) by the
> >> following command:
> >>
> >> 		*full_stripe_start =
> >> 			rounddown(*stripe_nr, nr_data_stripes(map)) <<
> >> 			BTRFS_STRIPE_LEN_SHIFT;
> >>
> >> The calculation itself is fine, but the value returned by rounddown() is
> >> dependent on both @stripe_nr (which is u32) and nr_data_stripes() (which
> >> returned int).
> >>
> >> Thus the result is also u32, then we do the left shift, which can
> >> overflow u32.
> >>
> >> If such overflow happens, @full_stripe_start will be a value way smaller
> >> than @offset, causing later "full_stripe_len - (offset -
> >> *full_stripe_start)" to underflow, thus make later length calculation to
> >> have no stripe boundary limit, resulting a write bio to exceed stripe
> >> boundary.
> >>
> >> There are some other locations like this, with a u32 @stripe_nr got left
> >> shift, which can lead to a similar overflow.
> >>
> >> [FIX]
> >> Fix all @stripe_nr with left shift with a type cast to u64 before the
> >> left shift.
> >>
> >> Those involved @stripe_nr or similar variables are recording the stripe
> >> number inside the chunk, which is small enough to be contained by u32,
> >> but their offset inside the chunk can not fit into u32.
> >>
> >> Thus for those specific left shifts, a type cast to u64 is necessary.
> >>
> >> Reported-by: David Sterba <dsterba@suse.com>
> >> Fixes: a97699d1d610 ("btrfs: replace map_lookup->stripe_len by BTRFS_STRIPE_LEN")
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >> Changelog:
> >> v2:
> >> - Fix all @stripe_nr with left shift
> >> - Apply the ASSERT() on full stripe checks for all RAID56 IOs.
> >> ---
> >>   fs/btrfs/volumes.c | 15 +++++++++------
> >>   1 file changed, 9 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >> index b8540af6e136..ed3765d21cb0 100644
> >> --- a/fs/btrfs/volumes.c
> >> +++ b/fs/btrfs/volumes.c
> >> @@ -5985,12 +5985,12 @@ struct btrfs_discard_stripe *btrfs_map_discard(struct btrfs_fs_info *fs_info,
> >>   	stripe_nr = offset >> BTRFS_STRIPE_LEN_SHIFT;
> >>
> >>   	/* stripe_offset is the offset of this block in its stripe */
> >> -	stripe_offset = offset - (stripe_nr << BTRFS_STRIPE_LEN_SHIFT);
> >> +	stripe_offset = offset - ((u64)stripe_nr << BTRFS_STRIPE_LEN_SHIFT);
> >
> > This needs a helper, mandating a type cast for correctness in so many
> > places is a bad pattern.
> 
> The problem is, we still need to manually determine if we need a cast or
> not.
> 
> For a lot of cases like "for (int i = 0; i < nr_data_stripes; i++) { do
> with i << BTRFS_STRIPE_LEN_SHIFT;}", it's safe to go with 32 bit and
> left shift.

The helper is supposed to avoid deciding if the cast is needed or not,
so the raw "<< BTRFS_STRIPE_LEN_SHIFT" should be abstracted away
everywhere and any uncommented occurece considered for closer
inspection. If you have a specific example where this would not work
please point to the code.