Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <a364f344-b718-48ff-9e2a-484c5ded6e7f@gmx.com>
Date:   Wed, 20 Sep 2023 07:20:49 +0930
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/4] btrfs: fix 64bit division in
 btrfs_insert_striped_mirrored_raid_extents
To:     dsterba@suse.cz
Cc:     Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>, Qu Wenru <wqu@suse.com>,
        "linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20230918-rst-updates-v1-0-17686dc06859@wdc.com>
 <20230918-rst-updates-v1-1-17686dc06859@wdc.com>
 <CAMuHMdWM3_cj4Nb96pZQfErx7n+0Cd7RUQZV+bpvr1Tz5T3sgw@mail.gmail.com>
 <e12a171e-d3b8-401e-b01a-9440f5c75293@wdc.com>
 <20230918162448.GI2747@suse.cz>
 <a0a5c7a3-4e55-4490-a2f9-fae2b0247829@gmx.com>
 <20230919135810.GT2747@twin.jikos.cz>
Content-Language: en-US
From:   Qu Wenruo <quwenruo.btrfs@gmx.com>
Autocrypt: addr=quwenruo.btrfs@gmx.com; keydata=
 xsBNBFnVga8BCACyhFP3ExcTIuB73jDIBA/vSoYcTyysFQzPvez64TUSCv1SgXEByR7fju3o
 8RfaWuHCnkkea5luuTZMqfgTXrun2dqNVYDNOV6RIVrc4YuG20yhC1epnV55fJCThqij0MRL
 1NxPKXIlEdHvN0Kov3CtWA+R1iNN0RCeVun7rmOrrjBK573aWC5sgP7YsBOLK79H3tmUtz6b
 9Imuj0ZyEsa76Xg9PX9Hn2myKj1hfWGS+5og9Va4hrwQC8ipjXik6NKR5GDV+hOZkktU81G5
 gkQtGB9jOAYRs86QG/b7PtIlbd3+pppT0gaS+wvwMs8cuNG+Pu6KO1oC4jgdseFLu7NpABEB
 AAHNIlF1IFdlbnJ1byA8cXV3ZW5ydW8uYnRyZnNAZ214LmNvbT7CwJQEEwEIAD4CGwMFCwkI
 BwIGFQgJCgsCBBYCAwECHgECF4AWIQQt33LlpaVbqJ2qQuHCPZHzoSX+qAUCY00iVQUJDToH
 pgAKCRDCPZHzoSX+qNKACACkjDLzCvcFuDlgqCiS4ajHAo6twGra3uGgY2klo3S4JespWifr
 BLPPak74oOShqNZ8yWzB1Bkz1u93Ifx3c3H0r2vLWrImoP5eQdymVqMWmDAq+sV1Koyt8gXQ
 XPD2jQCrfR9nUuV1F3Z4Lgo+6I5LjuXBVEayFdz/VYK63+YLEAlSowCF72Lkz06TmaI0XMyj
 jgRNGM2MRgfxbprCcsgUypaDfmhY2nrhIzPUICURfp9t/65+/PLlV4nYs+DtSwPyNjkPX72+
 LdyIdY+BqS8cZbPG5spCyJIlZonADojLDYQq4QnufARU51zyVjzTXMg5gAttDZwTH+8LbNI4
 mm2YzsBNBFnVga8BCACqU+th4Esy/c8BnvliFAjAfpzhI1wH76FD1MJPmAhA3DnX5JDORcga
 CbPEwhLj1xlwTgpeT+QfDmGJ5B5BlrrQFZVE1fChEjiJvyiSAO4yQPkrPVYTI7Xj34FnscPj
 /IrRUUka68MlHxPtFnAHr25VIuOS41lmYKYNwPNLRz9Ik6DmeTG3WJO2BQRNvXA0pXrJH1fN
 GSsRb+pKEKHKtL1803x71zQxCwLh+zLP1iXHVM5j8gX9zqupigQR/Cel2XPS44zWcDW8r7B0
 q1eW4Jrv0x19p4P923voqn+joIAostyNTUjCeSrUdKth9jcdlam9X2DziA/DHDFfS5eq4fEv
 ABEBAAHCwHwEGAEIACYCGwwWIQQt33LlpaVbqJ2qQuHCPZHzoSX+qAUCY00ibgUJDToHvwAK
 CRDCPZHzoSX+qK6vB/9yyZlsS+ijtsvwYDjGA2WhVhN07Xa5SBBvGCAycyGGzSMkOJcOtUUf
 tD+ADyrLbLuVSfRN1ke738UojphwkSFj4t9scG5A+U8GgOZtrlYOsY2+cG3R5vjoXUgXMP37
 INfWh0KbJodf0G48xouesn08cbfUdlphSMXujCA8y5TcNyRuNv2q5Nizl8sKhUZzh4BascoK
 DChBuznBsucCTAGrwPgG4/ul6HnWE8DipMKvkV9ob1xJS2W4WJRPp6QdVrBWJ9cCdtpR6GbL
 iQi22uZXoSPv/0oUrGU+U5X4IvdnvT+8viPzszL5wXswJZfqfy8tmHM85yjObVdIG6AlnrrD
In-Reply-To: <20230919135810.GT2747@twin.jikos.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:Xe7D27dGaVg=;P8QDX1wyPkWCu64NBzyHrk64UYY
 xgTAUZKwE76HVmAB8Nx1VHUDRIXHrEcHqeNj6FqZlAClrvdXXsepk0/3lq2BkMaWJvN31y+uS
 iyvQ8nKYnYo4+RvCI+8J0TRwBbDZ+XJuuIhRSkO++dZaLElSwxm4+Rv8R369oY3cnZ2VcRkBo
 3gXwYPROB9sBLaFb1Wtpn2yx1xFBNsC9poXuJDtVqXlQNE6UlbmXOdjksYMxTVIx7e0+S7MLV
 dQRkc1/0vuLtQRvap+1Z/0ELRR43Njb+FQb8aLQD8/4r75hZF20xKYypbkSXs6yGQhPU4TIu5
 w5FMtHukRU0QCYC7qxu6Tv9TFZ5FSXXjpQpkqWUVwtch59setz756YuGS4MtqyMIqcpoW2/kL
 dCugTTM8vIQ4LfJzmOiYb/VUxyHuLH/UrAorS76cPIqHNlcaB/WBukdT5Vhqg3jaRCo57ZPmM
 0l9no6SZcKt1bAkjm1Lh5CU8jNmiTE95L5cSottyBqYxTEq0a/ozFX95Usj3/45LjFx7+FhWL
 GezqgVfvba/rvRuuF7k5hw53PrnmuIQiQUV81eWE5T1EQjEhIsOky+9qlw33cR0aoVCAcqHzL
 RjZv1GW3dfFo5OJg5vtwb2xArx7GwBCmOE06YmoRL/4+JcUunCfSkt5XqksubsriCUJIiGzfi
 yFgnk86FilKzcU8gCNsNIV4qaPqhV8t2crPBc0bvReAjaVAj7xPALSg8z+/Ex5+kmc3oQYCRV
 1DnfJ7ZPw+ay2tPa9K5ArOrl3Zv2/lMvP9qyGubCKU3wHFxSAvWqsWYgQlQ+OxTlvGzn5kuxh
 s1JcID3eZo8RHO6r9k10AUaHdnAgo+W1aRr9p/r22tOwItZB8OYMYm5G0qncKqYvbJQ/R6Jjs
 xyFMEWI1JT5flvXdGt5YCzLRK/Z3qhvvfqQHj+KnoTMq2AHfz0lzDrznciw44HTtVXOgnNI50
 fO7wAA==
Precedence: bulk


On 2023/9/19 23:28, David Sterba wrote:
> On Tue, Sep 19, 2023 at 10:07:00AM +0930, Qu Wenruo wrote:
>> On 2023/9/19 01:54, David Sterba 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=E2=80=AFPM Johannes Thumshirn
>>>>> <johannes.thumshirn@wdc.com> wrote:
>>>>>> Fix modpost error due to 64bit division on 32bit systems in
>>>>>> btrfs_insert_striped_mirrored_raid_extents.
>>>>>>
>>>>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>>>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>>>>
>>>>> 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 =3D list_count_nodes(&ordered->bioc_list=
);
>>>>>> -       const int index =3D btrfs_bg_flags_to_raid_index(map_type);
>>>>>> -       const int substripes =3D btrfs_raid_array[index].sub_stripe=
s;
>>>>>> -       const int max_stripes =3D trans->fs_info->fs_devices->rw_de=
vices / substripes;
>>>>>> +       const size_t nstripes =3D list_count_nodes(&ordered->bioc_l=
ist);
>>>>>> +       const enum btrfs_raid_types index =3D btrfs_bg_flags_to_rai=
d_index(map_type);
>>>>>> +       const u8 substripes =3D btrfs_raid_array[index].sub_stripes=
;
>>>>>> +       const int max_stripes =3D div_u64(trans->fs_info->fs_device=
s->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 su=
ch
>>> huge values for rw_devices. The 'theoretically' would fit for a machin=
e
>>> with infinite resources, otherwise the maximum number of devices I'd
>>> expect is a few thousand.
>>
>> In fact, we already have an check in btrfs_validate_super(), if the
>> num_devices is over 1<<31, we would reject the fs.
>
> No, it's just a warning in that case.

We can make it a proper reject.

>
>> I think we should be safe to further reduce the threshold.
>>
>> U16_MAX sounds a valid and sane value to me.
>> If no rejection I can send out a patch for this.
>>
>> And later change internal rw_devices/num_devices to u16.
>
> U16 does not make sense here, it's not a native int type on many
> architectures and generates awkward assembly code. We use it in
> justified cases where it's saving space in structures that are allocated
> thousand times. The arbitrary limit 65536 is probably sane but not
> much different than 1<<31, practically not hit and was useful to
> note fuzzed superblocks.

OK, we can make it unsigned int (mostly u32) for fs_info::*_devices, but
still do extra limits on things like device add to limit it to U16_MAX.

Would this be a better solution?
At least it would still half the width while keep it native to most (if
not all) archs.

Thanks,
Qu