Re: [PATCH] btrfs: get rid of memparse() for sysfs operations

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/12/8 00:39, David Sterba wrote:
> On Thu, Dec 07, 2023 at 02:06:11PM +1030, Qu Wenruo wrote:
>> [CONFUSION]
>> Btrfs is using memparse() for the following sysfs interfaces:
>>
>> - /sys/fs/btrfs/<uuid>/devinfo/<devid>/scrub_speed_max
>> - /sys/fs/btrfs/<uuid>/allocation/<type>/chunk_size
>>
>> Thus we can echo some seemingly valid values into them (using
>> scrub_speed_max as an example):
>>
>>   # echo 25e > scrub_speed_max
>>   # cat scrub_speed_max
>>   10376293541461622784
>>
>> This can cause several confusion:
>>
>> - The end user may just want to type "0x25e"
>> - Even if it's treated as decimal "25" and E (exabyte), the result is
>>    not correct
>>    25 exabyte should be 28147497671065600, as 25 with 2 ** 10 would lead
>>    to something ends with "00" in decimal (25 * 4 = 100).
>>
>> [CAUSE]
>> Above "25e" is valid because memparse() handles the extra suffix and do
>> proper base conversion.
>>
>> "25e" has no "0x" or "0" prefix, thus it's treated as decimal, and only
>> "25" is the valid part. Then it goes with number detection, but the
>> final "e" is treated as invalid since the base is 10.
>>
>> Then we do the extra left shift based on the suffix.
>>
>> There are several problem in memparse itself:
>>
>> - No overflow check
>>    The usage of simple_strtoull() lacks the overflow check, thus it's
>>    already discouraged to use.
>>
>> - The suffix "E" (exabyte) can be easily confused with 0xe.
>>
>> [FIX]
>> For btrfs sysfs interface, we don't accept extra suffix, except the
>> mentioned two entries.
>>
>> Furthermore since we don't do pretty size output, and considering the
>> sysfs interfaces are mostly for script or other tools, there is no need
>> to accept extra suffix either.
>>
>> So here we can just replace the memparse() to kstrou64() instead, and
>> reject the above "25e" example.
>
> No, please read the reasoning why we want the suffixes,
> https://lore.kernel.org/linux-btrfs/20231207135522.GX2751@twin.jikos.cz/

I'm strongly against the expectation that all sysfs entrances should
accept suffix.

In fact, no matter if it's btrfs or not, memparse() is the minor usage,
most are not accepting the suffix.

>
> That memparse accepts 0x is fine but I'd say highly uncommon to be ever
> seen as input.

Really? 0x10000000 or 4294967296, which is easily to grasp?

Thus the whole thing really depends on your expectation.

But one thing is for sure, all entries should accept bytes, that should
always be true.
Whether suffix should be acceptable is already debatable.

>
> If memparse is not able to correctly parse exabytes then it's not our
> bug, as the comment says

But it's our fault to use functions known to lead to wrong numbers.

>
> "This function has caveats. Please use kstrtoull instead."
> https://elixir.bootlin.com/linux/latest/source/lib/vsprintf.c#L93
>
The comment says exactly what we should do, "use strtoull instead".

Thanks,
Qu