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

[David Sterba <dsterba@suse.cz>]

On Tue, Jun 20, 2023 at 08:05:58PM +0800, Qu Wenruo wrote:
> 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:
> >>>> ---
> >>>>    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?

Yes, given the time constraints it's safer to do a minimal fix. No other
problem has appeared, I tried the workload on 26 devices, some profile
conversions, device deletions, scrub etc.