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

[Qu Wenruo <wqu@suse.com>]

On 2023/6/20 19:56, David Sterba wrote:
> 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.

E.g. for the code inside RAID56 utilizing the left shift, they are all 
safe and no need to do a u64 cast.

Yes, I got your point, but for the bug fix, can we split them into two 
patches?
The first one introduce the helper and fix the 5 call sites, this should 
be very small and easy to backport.

Then the second patch to convert the remaining ones no matter if it's 
safe or not.

Would this be a reasonable solution?

Thanks,
Qu