[PATCH] btrfs: drop unused memparse() parameter

[PATCH] btrfs: drop unused memparse() parameter

[David Disseldorp]

The @retptr parameter for memparse() is optional.
btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
validation, so the parameter can be dropped.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 fs/btrfs/sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index e6b51fb3ddc1e..75f3b774a4d83 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1779,10 +1779,9 @@ 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;
 
-	limit = memparse(buf, &endptr);
+	limit = memparse(buf, NULL);
 	WRITE_ONCE(device->scrub_speed_max, limit);
 	return len;
 }
-- 
2.35.3

Re: [PATCH] btrfs: drop unused memparse() parameter

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH] btrfs: drop unused memparse() parameter

[David Sterba]

On Tue, Dec 05, 2023 at 10:13:29PM +1100, David Disseldorp wrote:
> The @retptr parameter for memparse() is optional.
> btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
> validation, so the parameter can be dropped.

Or should it be used for validation? memparse is also used in
btrfs_chunk_size_store() that accepts whitespace as trailing characters
(namely '\n' if the value is from echo).

Re: [PATCH] btrfs: drop unused memparse() parameter

[David Disseldorp]

On Tue, 5 Dec 2023 15:22:53 +0100, David Sterba wrote:

> On Tue, Dec 05, 2023 at 10:13:29PM +1100, David Disseldorp wrote:
> > The @retptr parameter for memparse() is optional.
> > btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
> > validation, so the parameter can be dropped.  
> 
> Or should it be used for validation? memparse is also used in
> btrfs_chunk_size_store() that accepts whitespace as trailing characters
> (namely '\n' if the value is from echo).

It probably should have been used for validation when originally added,
but the current behaviour is now part of the sysfs scrub_speed_max API.
Failing on invalid input would break scripts which do things like
  echo clear > /sys/fs/btrfs/UUID/devinfo/1/scrub_speed_max

Cheers, David

Re: [PATCH] btrfs: drop unused memparse() parameter

[David Sterba]

On Wed, Dec 06, 2023 at 11:21:43AM +1100, David Disseldorp wrote:
> On Tue, 5 Dec 2023 15:22:53 +0100, David Sterba wrote:
> 
> > On Tue, Dec 05, 2023 at 10:13:29PM +1100, David Disseldorp wrote:
> > > The @retptr parameter for memparse() is optional.
> > > btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
> > > validation, so the parameter can be dropped.  
> > 
> > Or should it be used for validation? memparse is also used in
> > btrfs_chunk_size_store() that accepts whitespace as trailing characters
> > (namely '\n' if the value is from echo).
> 
> It probably should have been used for validation when originally added,
> but the current behaviour is now part of the sysfs scrub_speed_max API.
> Failing on invalid input would break scripts which do things like
>   echo clear > /sys/fs/btrfs/UUID/devinfo/1/scrub_speed_max

I'm not sure the 'part of the API' is a valid agrument here. It's
documented that the value is in bytes and that suffixes can be passed
for convenience. How come anybody would use 'clear' in the first place
and expect it to work with undefined meaning?

Re: [PATCH] btrfs: drop unused memparse() parameter

[David Disseldorp]

On Wed, 6 Dec 2023 19:53:31 +0100, David Sterba wrote:

> On Wed, Dec 06, 2023 at 11:21:43AM +1100, David Disseldorp wrote:
> > On Tue, 5 Dec 2023 15:22:53 +0100, David Sterba wrote:
> >   
> > > On Tue, Dec 05, 2023 at 10:13:29PM +1100, David Disseldorp wrote:  
> > > > The @retptr parameter for memparse() is optional.
> > > > btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
> > > > validation, so the parameter can be dropped.    
> > > 
> > > Or should it be used for validation? memparse is also used in
> > > btrfs_chunk_size_store() that accepts whitespace as trailing characters
> > > (namely '\n' if the value is from echo).  
> > 
> > It probably should have been used for validation when originally added,
> > but the current behaviour is now part of the sysfs scrub_speed_max API.
> > Failing on invalid input would break scripts which do things like
> >   echo clear > /sys/fs/btrfs/UUID/devinfo/1/scrub_speed_max  
> 
> I'm not sure the 'part of the API' is a valid agrument here. It's
> documented that the value is in bytes and that suffixes can be passed
> for convenience. How come anybody would use 'clear' in the first place
> and expect it to work with undefined meaning?

Most people don't read documentation :). If there's a willingness to
accept any fallout from adding the validation then I'm happy to do that.
Will send a v2.

Cheers, David

Re: [PATCH] btrfs: drop unused memparse() parameter

[Qu Wenruo]

[-- Attachment #1.1.1: Type: text/plain, Size: 1581 bytes --]



On 2023/12/5 21:43, David Disseldorp wrote:
> The @retptr parameter for memparse() is optional.
> btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
> validation, so the parameter can be dropped.

To me, I believe it's better to completely get rid of memparse().

As you already found out, some suffix, especially "e|E" can screw up the 
result.
E.g. "25e" would be interpreted as 25 with "e" as suffix, which is fine 
according to the rule. (without prefix, the base is 10, so only "25" is 
valid. Then the remaining part is interpreted as suffix).

And since btrfs is not going to do pretty size output for sysfs (as most 
sysfs is not directly for end users, and we need accurate output), to be 
consistent there isn't much need for suffix handling either.

So can't we just replace memparse() with kstrtoull()?

Thanks,
Qu


> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>   fs/btrfs/sysfs.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index e6b51fb3ddc1e..75f3b774a4d83 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1779,10 +1779,9 @@ 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;
>   
> -	limit = memparse(buf, &endptr);
> +	limit = memparse(buf, NULL);
>   	WRITE_ONCE(device->scrub_speed_max, limit);
>   	return len;
>   }

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7027 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] btrfs: drop unused memparse() parameter

[David Sterba]

On Thu, Dec 07, 2023 at 01:01:50PM +1030, Qu Wenruo wrote:
> 
> 
> On 2023/12/5 21:43, David Disseldorp wrote:
> > The @retptr parameter for memparse() is optional.
> > btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
> > validation, so the parameter can be dropped.
> 
> To me, I believe it's better to completely get rid of memparse().
> 
> As you already found out, some suffix, especially "e|E" can screw up the 
> result.
> E.g. "25e" would be interpreted as 25 with "e" as suffix, which is fine 
> according to the rule. (without prefix, the base is 10, so only "25" is 
> valid. Then the remaining part is interpreted as suffix).
> 
> And since btrfs is not going to do pretty size output for sysfs (as most 
> sysfs is not directly for end users, and we need accurate output), to be 
> consistent there isn't much need for suffix handling either.
> 
> So can't we just replace memparse() with kstrtoull()?

The value that can be read from the sysfs file is in bytes and it's so
that applications do not need to interpret it, like multiplying with
1024. We'll probably never return the pretty values with suffixes in
sysfs files.

However, on the input side the suffixes are a convenience, setting to
limit the throughput as '32m' is better than typing '32000000' and
counting zeros or $((32*1024*1024)) or 33554432.

This is why memparse is there and kstrtoull does not do that.

Re: [PATCH] btrfs: drop unused memparse() parameter

[David Sterba]

On Thu, Dec 07, 2023 at 10:52:55AM +1100, David Disseldorp wrote:
> On Wed, 6 Dec 2023 19:53:31 +0100, David Sterba wrote:
> 
> > On Wed, Dec 06, 2023 at 11:21:43AM +1100, David Disseldorp wrote:
> > > On Tue, 5 Dec 2023 15:22:53 +0100, David Sterba wrote:
> > >   
> > > > On Tue, Dec 05, 2023 at 10:13:29PM +1100, David Disseldorp wrote:  
> > > > > The @retptr parameter for memparse() is optional.
> > > > > btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
> > > > > validation, so the parameter can be dropped.    
> > > > 
> > > > Or should it be used for validation? memparse is also used in
> > > > btrfs_chunk_size_store() that accepts whitespace as trailing characters
> > > > (namely '\n' if the value is from echo).  
> > > 
> > > It probably should have been used for validation when originally added,
> > > but the current behaviour is now part of the sysfs scrub_speed_max API.
> > > Failing on invalid input would break scripts which do things like
> > >   echo clear > /sys/fs/btrfs/UUID/devinfo/1/scrub_speed_max  
> > 
> > I'm not sure the 'part of the API' is a valid agrument here. It's
> > documented that the value is in bytes and that suffixes can be passed
> > for convenience. How come anybody would use 'clear' in the first place
> > and expect it to work with undefined meaning?
> 
> Most people don't read documentation :).

Unfortunatelly true. Here the expected practice is that size values on
input can have a suffix so it should be consistent and understandable
even without reading documentation.

> If there's a willingness to
> accept any fallout from adding the validation then I'm happy to do that.
> Will send a v2.

Yes, I think the simple fix is to add the handling as
btrfs_chunk_size_store() does and this will be the pattern for any
future memparsed values in sysfs.

There's one outlier discard/kbps_limit that takes KiB directly so that's
a plain integer. Eventually discard/max_discard_size can also use memparse.

Re: [PATCH] btrfs: drop unused memparse() parameter

[Qu Wenruo]

On 2023/12/7 22:45, David Sterba wrote:
> On Thu, Dec 07, 2023 at 01:01:50PM +1030, Qu Wenruo wrote:
>>
>>
>> On 2023/12/5 21:43, David Disseldorp wrote:
>>> The @retptr parameter for memparse() is optional.
>>> btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
>>> validation, so the parameter can be dropped.
>>
>> To me, I believe it's better to completely get rid of memparse().
>>
>> As you already found out, some suffix, especially "e|E" can screw up the
>> result.
>> E.g. "25e" would be interpreted as 25 with "e" as suffix, which is fine
>> according to the rule. (without prefix, the base is 10, so only "25" is
>> valid. Then the remaining part is interpreted as suffix).
>>
>> And since btrfs is not going to do pretty size output for sysfs (as most
>> sysfs is not directly for end users, and we need accurate output), to be
>> consistent there isn't much need for suffix handling either.
>>
>> So can't we just replace memparse() with kstrtoull()?
>
> The value that can be read from the sysfs file is in bytes and it's so
> that applications do not need to interpret it, like multiplying with
> 1024. We'll probably never return the pretty values with suffixes in
> sysfs files.
>
> However, on the input side the suffixes are a convenience, setting to
> limit the throughput as '32m' is better than typing '32000000' and
> counting zeros or $((32*1024*1024)) or 33554432.
>
> This is why memparse is there and kstrtoull does not do that.

That suffix is causing confusion already, just check my "25e" case.
(It does not only lead to huge number, but also lead to incorrect value
even if we treat "e" as a suffix)

Furthermore, the convenience argument is not that strong, you won't
expect end users to do the change for a fs every time.
Thus it's mostly managed by a small script or some other tool.

In that case I don't think doing extra bash calculation is a big deal
anyway.

Thanks,
Qu

[PATCH] btrfs: validate scrub_speed_max sysfs string

[David Disseldorp]

Fail the sysfs I/O on any trailing non-space characters.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 fs/btrfs/sysfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index e6b51fb3ddc1e..4c4642ef7c5f0 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1783,6 +1783,9 @@ static ssize_t btrfs_devinfo_scrub_speed_max_store(struct kobject *kobj,
 	unsigned long long limit;
 
 	limit = memparse(buf, &endptr);
+	endptr = skip_spaces(endptr);
+	if (*endptr != 0)
+		return -EINVAL;
 	WRITE_ONCE(device->scrub_speed_max, limit);
 	return len;
 }
-- 
2.35.3

Re: [PATCH] btrfs: validate scrub_speed_max sysfs string

[Qu Wenruo]

On 2023/12/8 11:11, David Disseldorp wrote:
> Fail the sysfs I/O on any trailing non-space characters.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Although I have an unrelated idea.

Since memparse() provides the @endptr, can we rewind the @endptr, so
that we can check if the last valid charactor is suffix 'e'.
Then reject it from btrfs size.

I really don't think we need exabytes suffix for our scrub speed limit
usage.

Thanks,
Qu

> ---
>   fs/btrfs/sysfs.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index e6b51fb3ddc1e..4c4642ef7c5f0 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1783,6 +1783,9 @@ static ssize_t btrfs_devinfo_scrub_speed_max_store(struct kobject *kobj,
>   	unsigned long long limit;
>
>   	limit = memparse(buf, &endptr);
> +	endptr = skip_spaces(endptr);
> +	if (*endptr != 0)
> +		return -EINVAL;
>   	WRITE_ONCE(device->scrub_speed_max, limit);
>   	return len;
>   }

Re: [PATCH] btrfs: validate scrub_speed_max sysfs string

[David Disseldorp]

On Mon, 11 Dec 2023 13:48:15 +1030, Qu Wenruo wrote:

> On 2023/12/8 11:11, David Disseldorp wrote:
> > Fail the sysfs I/O on any trailing non-space characters.
> >
> > Signed-off-by: David Disseldorp <ddiss@suse.de>  
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks.

> Although I have an unrelated idea.
> 
> Since memparse() provides the @endptr, can we rewind the @endptr, so
> that we can check if the last valid charactor is suffix 'e'.
> Then reject it from btrfs size.

Hmm we could do that, but then we'd also break hex parsing. Rejecting
'e' in the absence of a leading '0x' or '0X' would work, but it's a bit
ugly.

Cheers, David

Re: [PATCH] btrfs: validate scrub_speed_max sysfs string

[David Sterba]

On Mon, Dec 11, 2023 at 01:48:15PM +1030, Qu Wenruo wrote:
> 
> 
> On 2023/12/8 11:11, David Disseldorp wrote:
> > Fail the sysfs I/O on any trailing non-space characters.
> >
> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Although I have an unrelated idea.
> 
> Since memparse() provides the @endptr, can we rewind the @endptr, so
> that we can check if the last valid charactor is suffix 'e'.
> Then reject it from btrfs size.
> 
> I really don't think we need exabytes suffix for our scrub speed limit
> usage.

I think nobody will intentionally use the 'e' exabyte suffix in this
case but I don't want to add an exception to parsing the values. We'd
have to document that, explain why it's not accepted, it's additional
work for a case I still dont understand why it's so important.

If the exabyte scale values are not properly parsed by memparse() due to
simple_strtoull() as a workaround we can add our parser based on
kstrtoull() or do such change in memparse() proper.

Re: [PATCH] btrfs: validate scrub_speed_max sysfs string

[David Sterba]

On Fri, Dec 08, 2023 at 11:41:56AM +1100, David Disseldorp wrote:
> Fail the sysfs I/O on any trailing non-space characters.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>

Added to misc-next a few days ago with updated changelog with reasoning
based on the recent discussions why we want the memparse and suffixes.
Thanks.

Re: [PATCH] btrfs: drop unused memparse() parameter

[David Sterba]

On Fri, Dec 08, 2023 at 06:26:50AM +1030, Qu Wenruo wrote:
> > The value that can be read from the sysfs file is in bytes and it's so
> > that applications do not need to interpret it, like multiplying with
> > 1024. We'll probably never return the pretty values with suffixes in
> > sysfs files.
> >
> > However, on the input side the suffixes are a convenience, setting to
> > limit the throughput as '32m' is better than typing '32000000' and
> > counting zeros or $((32*1024*1024)) or 33554432.
> >
> > This is why memparse is there and kstrtoull does not do that.
> 
> That suffix is causing confusion already, just check my "25e" case.
> (It does not only lead to huge number, but also lead to incorrect value
> even if we treat "e" as a suffix)
> 
> Furthermore, the convenience argument is not that strong, you won't
> expect end users to do the change for a fs every time.
> Thus it's mostly managed by a small script or some other tool.
> 
> In that case I don't think doing extra bash calculation is a big deal
> anyway.

This is a nice study of convenience vs extra work "that's no big deal".
The sysfs files can be used by scripts, often times it's the only access
to some settings (like /usr/bin/rescan-scsi-bus.sh/usr/bin/rescan-scsi-bus.sh).
The value format, naming of the sysfs files is inconsistent and
navigating to a particular directory and setting some magic value is I
think common. Another example are trace points, or dynamic printk.

There are tools abstracting that but sometimes it's a person that needs
to type it and the shortcuts like the suffix are convenient because
there's no extra need to type exact syntax and numbers for a $((...))
expression. Ask 3 people if they'd prefer to type "4m" or
"$((4*1024*1024))".

If you ask me I'm all for allowing "4m" and I did that several times
when testing things like the discard tunables, chunk size or the scrub
limits. I consider the sysfs files an interface for human interaction so
even if it's used like that in 10% of time it's still saving typing or
allowing one to focus on the real prolem instead of shell sytax.

Re: [PATCH] btrfs: drop unused memparse() parameter

[Qu Wenruo]

On 2023/12/14 09:45, David Sterba wrote:
> On Fri, Dec 08, 2023 at 06:26:50AM +1030, Qu Wenruo wrote:
>>> The value that can be read from the sysfs file is in bytes and it's so
>>> that applications do not need to interpret it, like multiplying with
>>> 1024. We'll probably never return the pretty values with suffixes in
>>> sysfs files.
>>>
>>> However, on the input side the suffixes are a convenience, setting to
>>> limit the throughput as '32m' is better than typing '32000000' and
>>> counting zeros or $((32*1024*1024)) or 33554432.
>>>
>>> This is why memparse is there and kstrtoull does not do that.
>>
>> That suffix is causing confusion already, just check my "25e" case.
>> (It does not only lead to huge number, but also lead to incorrect value
>> even if we treat "e" as a suffix)
>>
>> Furthermore, the convenience argument is not that strong, you won't
>> expect end users to do the change for a fs every time.
>> Thus it's mostly managed by a small script or some other tool.
>>
>> In that case I don't think doing extra bash calculation is a big deal
>> anyway.
>
> This is a nice study of convenience vs extra work "that's no big deal".
> The sysfs files can be used by scripts, often times it's the only access
> to some settings (like /usr/bin/rescan-scsi-bus.sh/usr/bin/rescan-scsi-bus.sh).
> The value format, naming of the sysfs files is inconsistent and
> navigating to a particular directory and setting some magic value is I
> think common. Another example are trace points, or dynamic printk.
>
> There are tools abstracting that but sometimes it's a person that needs
> to type it and the shortcuts like the suffix are convenient because
> there's no extra need to type exact syntax and numbers for a $((...))
> expression. Ask 3 people if they'd prefer to type "4m" or
> "$((4*1024*1024))".
>
> If you ask me I'm all for allowing "4m" and I did that several times
> when testing things like the discard tunables, chunk size or the scrub
> limits. I consider the sysfs files an interface for human interaction so
> even if it's used like that in 10% of time it's still saving typing or
> allowing one to focus on the real prolem instead of shell sytax.
>

OK, then let me jump into the rabbit hole of stroxll family for my
holiday projects, to make the memparse() itself more reliable and get
rid of the caveats.

Hopefully allowing callers to select which suffixes they want to use.

(Well, at least I hope "25e" can give us a correct value after the fix)

Thanks,
Qu