Re: [PATCH 1/4] btrfs: fix 64bit division in btrfs_insert_striped_mirrored_raid_extents

[David Sterba]

On Mon, Sep 18, 2023 at 08:31:19PM +0200, Geert Uytterhoeven wrote:
> Hi David,
> 
> On Mon, Sep 18, 2023 at 6:31 PM David Sterba <dsterba@xxxxxxx> wrote:
> > On Mon, Sep 18, 2023 at 03:03:10PM +0000, Johannes Thumshirn wrote:
> > > On 18.09.23 16:19, Geert Uytterhoeven wrote:
> > > > Hi Johannes,
> > > >
> > > > On Mon, Sep 18, 2023 at 4:14 PM Johannes Thumshirn
> > > > <johannes.thumshirn@xxxxxxx> wrote:
> > > >> Fix modpost error due to 64bit division on 32bit systems in
> > > >> btrfs_insert_striped_mirrored_raid_extents.
> > > >>
> > > >> Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
> > > >
> > > > Thanks for your patch!
> > > >
> > > >> --- a/fs/btrfs/raid-stripe-tree.c
> > > >> +++ b/fs/btrfs/raid-stripe-tree.c
> > > >> @@ -148,10 +148,10 @@ static int btrfs_insert_striped_mirrored_raid_extents(
> > > >>   {
> > > >>          struct btrfs_io_context *bioc;
> > > >>          struct btrfs_io_context *rbioc;
> > > >> -       const int nstripes = list_count_nodes(&ordered->bioc_list);
> > > >> -       const int index = btrfs_bg_flags_to_raid_index(map_type);
> > > >> -       const int substripes = btrfs_raid_array[index].sub_stripes;
> > > >> -       const int max_stripes = trans->fs_info->fs_devices->rw_devices / substripes;
> > > >> +       const size_t nstripes = list_count_nodes(&ordered->bioc_list);
> > > >> +       const enum btrfs_raid_types index = btrfs_bg_flags_to_raid_index(map_type);
> > > >> +       const u8 substripes = btrfs_raid_array[index].sub_stripes;
> > > >> +       const int max_stripes = div_u64(trans->fs_info->fs_devices->rw_devices, substripes);
> > > >
> > > > What if the quotient does not fit in a signed 32-bit value?
> > >
> > > Then you've bought a lot of HDDs ;-)
> > >
> > > Jokes aside, yes this is theoretically correct. Dave can you fix
> > > max_stripes up to be u64 when applying?
> >
> > I think we can keep it int, or unsigned int if needed, we can't hit such
> > huge values for rw_devices. The 'theoretically' would fit for a machine
> > with infinite resources, otherwise the maximum number of devices I'd
> > expect is a few thousand.
> 
> rw_devices and various other *_devices are u64.
> Is there a good reason they are that big?

Many members' types of the on-disk structures are generous and u64 was
the default choice, in many cases practically meaning "you don't have to
care about it for the whole fileystem lifetime" or when u32 would be
close to some potentially reachable value (like 4GiB chunks). You could
find examples where u64 is too much but it's not a big deal for data
stored once and over time I don't remember that we'd have to regret that
some struct member is not big enough.

> With the fs fuzzing threads in mind, is any validation done on their values?

I think the superblock is the most fuzzed structure of btrfs and we do a
lot of direct validation,

https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git/tree/fs/btrfs/disk-io.c#n2299

regarding the number of devices there's a warning when the value is
larger than "1<<31"

https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git/tree/fs/btrfs/disk-io.c#n2433

The rw_devices are counting how many devices are actually found (i.e.
represented by a block device) and compared against the value stored in
the super block.

The u64 is also convenient for calculations where a e.g. a type counting
zones was u32 because it's a sane type but then we need to convert it to
bytes the shift overflows, we had such bugs.  Fortunatelly the sector_t
is u64 for a long time but it was also source of subtle errors when
converting to bytes.