Re: [PATCH 2/2] btrfs: sysfs: use kstrtoull_suffix() to replace memparse()

[David Disseldorp <ddiss@suse.de>]

Hi Qu,

On Fri, 15 Dec 2023 19:09:24 +1030, Qu Wenruo wrote:

> Since memparse() itself can not handle overflow at all, use
> memparse_ull() to be extra safe.

s/memparse_ull/kstrtoull_suffix/

> Now overflow values can be properly detected.

Please document how the sysfs API changes with this, in addition to
overflow handling:
- support for 'E' / 'e' suffixes dropped
- only one trailing '\n' accepted, instead of many isspace()

The latter might break a few scripts.

Cheers, David

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/sysfs.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 84c05246ffd8..089c3fc123fe 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -760,7 +760,7 @@ 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;
> +	int ret;
>  	u64 val;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -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 = kstrtoull_suffix(buf, 0, &val, KSTRTOULL_SUFFIX_DEFAULT);
> +	if (ret < 0)
> +		return ret;
>  
>  	val = min(val, BTRFS_MAX_DATA_CHUNK_SIZE);
>  
> @@ -1779,14 +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;
> +	int ret;
>  
> -	limit = memparse(buf, &endptr);
> -	/* There could be trailing '\n', also catch any typos after the value. */
> -	endptr = skip_spaces(endptr);
> -	if (*endptr != 0)
> -		return -EINVAL;
> +	ret = kstrtoull_suffix(buf, 0, &limit, KSTRTOULL_SUFFIX_DEFAULT);
> +	if (ret < 0)
> +		return ret;
>  	WRITE_ONCE(device->scrub_speed_max, limit);
>  	return len;
>  }