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

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

[Qu Wenruo]

[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.

Reported-by: David Disseldorp <ddiss@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/sysfs.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index e6b51fb3ddc1..051642177982 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -760,8 +760,8 @@ static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
 {
 	struct btrfs_space_info *space_info = to_space_info(kobj);
 	struct btrfs_fs_info *fs_info = to_fs_info(get_btrfs_kobj(kobj));
-	char *retptr;
 	u64 val;
+	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -776,11 +776,9 @@ static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
 	if (space_info->flags & BTRFS_BLOCK_GROUP_SYSTEM)
 		return -EPERM;
 
-	val = memparse(buf, &retptr);
-	/* There could be trailing '\n', also catch any typos after the value */
-	retptr = skip_spaces(retptr);
-	if (*retptr != 0 || val == 0)
-		return -EINVAL;
+	ret = kstrtou64(buf, 0, &val);
+	if (ret < 0)
+		return ret;
 
 	val = min(val, BTRFS_MAX_DATA_CHUNK_SIZE);
 
@@ -1779,10 +1777,12 @@ static ssize_t btrfs_devinfo_scrub_speed_max_store(struct kobject *kobj,
 {
 	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
 						   devid_kobj);
-	char *endptr;
-	unsigned long long limit;
+	u64 limit;
+	int ret;
 
-	limit = memparse(buf, &endptr);
+	ret = kstrtou64(buf, 0, &limit);
+	if (ret < 0)
+		return ret;
 	WRITE_ONCE(device->scrub_speed_max, limit);
 	return len;
 }
-- 
2.43.0

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

[David Sterba]

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/

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

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

"This function has caveats. Please use kstrtoull instead."
https://elixir.bootlin.com/linux/latest/source/lib/vsprintf.c#L93

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

[Qu Wenruo]

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