[PATCH 0/1] btrfs: Simplify parsing max_inline mount option

[PATCH 0/1] btrfs: Simplify parsing max_inline mount option

[Vladimir Panteleev]

A nit I noticed when writing the global_reserve_size patch.

I'm assuming that rejecting garbage in mount options (where it was
silently accepted before) does not break the first rule of kernel
development?

Vladimir Panteleev (1):
  btrfs: Simplify parsing max_inline mount option

 fs/btrfs/super.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

-- 
2.22.0

[PATCH 1/1] btrfs: Simplify parsing max_inline mount option

[Vladimir Panteleev]

- Avoid an allocation;
- Properly handle non-numerical argument and garbage after numerical
  argument.

Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
---
 fs/btrfs/super.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f56617dfb3cf..6fe8ef6667f3 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -426,7 +426,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			unsigned long new_flags)
 {
 	substring_t args[MAX_OPT_ARGS];
-	char *p, *num;
+	char *p, *retptr;
 	u64 cache_gen;
 	int intarg;
 	int ret = 0;
@@ -630,22 +630,16 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			info->thread_pool_size = intarg;
 			break;
 		case Opt_max_inline:
-			num = match_strdup(&args[0]);
-			if (num) {
-				info->max_inline = memparse(num, NULL);
-				kfree(num);
-
-				if (info->max_inline) {
-					info->max_inline = min_t(u64,
-						info->max_inline,
-						info->sectorsize);
-				}
-				btrfs_info(info, "max_inline at %llu",
-					   info->max_inline);
-			} else {
-				ret = -ENOMEM;
+			info->max_inline = memparse(args[0].from, &retptr);
+			if (retptr != args[0].to || info->max_inline == 0) {
+				ret = -EINVAL;
 				goto out;
 			}
+			info->max_inline = min_t(u64,
+				info->max_inline,
+				info->sectorsize);
+			btrfs_info(info, "max_inline at %llu",
+				   info->max_inline);
 			break;
 		case Opt_alloc_start:
 			btrfs_info(info,
-- 
2.22.0

Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option

[Anand Jain]

On 15/8/19 10:04 AM, Vladimir Panteleev wrote:
> - Avoid an allocation;
> - Properly handle non-numerical argument and garbage after numerical
>    argument.
> 
> Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
> ---
>   fs/btrfs/super.c | 24 +++++++++---------------
>   1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index f56617dfb3cf..6fe8ef6667f3 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -426,7 +426,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>   			unsigned long new_flags)
>   {
>   	substring_t args[MAX_OPT_ARGS];
> -	char *p, *num;
> +	char *p, *retptr;
>   	u64 cache_gen;
>   	int intarg;
>   	int ret = 0;
> @@ -630,22 +630,16 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>   			info->thread_pool_size = intarg;
>   			break;
>   		case Opt_max_inline:
> -			num = match_strdup(&args[0]);
> -			if (num) {
> -				info->max_inline = memparse(num, NULL);
> -				kfree(num);
> -
> -				if (info->max_inline) {
> -					info->max_inline = min_t(u64,
> -						info->max_inline,
> -						info->sectorsize);
> -				}
> -				btrfs_info(info, "max_inline at %llu",
> -					   info->max_inline);
> -			} else {
> -				ret = -ENOMEM;
> +			info->max_inline = memparse(args[0].from, &retptr);


> +			if (retptr != args[0].to || info->max_inline == 0) {
> +				ret = -EINVAL;
>   				goto out;

  This causes regression, max_inline = 0 is a valid parameter.

Thanks, Anand


>   			}
> +			info->max_inline = min_t(u64,
> +				info->max_inline,
> +				info->sectorsize);
> +			btrfs_info(info, "max_inline at %llu",
> +				   info->max_inline);
>   			break;
>   		case Opt_alloc_start:
>   			btrfs_info(info,
> 

Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option

[Vladimir Panteleev]

On Thu, 15 Aug 2019 at 04:54, Anand Jain <anand.jain@oracle.com> wrote:
>   This causes regression, max_inline = 0 is a valid parameter.

Thank you for catching that. Apologies for making such a rudimentary mistake.

I will apply more diligence and resubmit.

Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option

[David Sterba]

On Thu, Aug 15, 2019 at 02:04:53AM +0000, Vladimir Panteleev wrote:
> - Avoid an allocation;
> - Properly handle non-numerical argument and garbage after numerical
>   argument.
> 
> Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
> ---
>  fs/btrfs/super.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index f56617dfb3cf..6fe8ef6667f3 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -426,7 +426,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>  			unsigned long new_flags)
>  {
>  	substring_t args[MAX_OPT_ARGS];
> -	char *p, *num;
> +	char *p, *retptr;
>  	u64 cache_gen;
>  	int intarg;
>  	int ret = 0;
> @@ -630,22 +630,16 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>  			info->thread_pool_size = intarg;
>  			break;
>  		case Opt_max_inline:
> -			num = match_strdup(&args[0]);
> -			if (num) {
> -				info->max_inline = memparse(num, NULL);
> -				kfree(num);
> -
> -				if (info->max_inline) {
> -					info->max_inline = min_t(u64,
> -						info->max_inline,
> -						info->sectorsize);
> -				}
> -				btrfs_info(info, "max_inline at %llu",
> -					   info->max_inline);
> -			} else {
> -				ret = -ENOMEM;
> +			info->max_inline = memparse(args[0].from, &retptr);

I don't think this is a good idea, match_strdup takes an opaque type
substring_t that is used by the parser. So accessing ::from directly in
memparse does not respect the API boundary. I've checked some other
usages in the tree and the match_strdup/memparse/kstrtoull is quite
common.

Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option

[Vladimir Panteleev]

On Wed, 21 Aug 2019 at 14:35, David Sterba <dsterba@suse.cz> wrote:
> match_strdup takes an opaque type
> substring_t that is used by the parser.

Sorry, how would one determine that the type is opaque?

> I've checked some other
> usages in the tree and the match_strdup/memparse/kstrtoull is quite
> common.

Sorry, I also see there are places where substring_t's .from / .to are
still accessed directly (including in btrfs code). Do you think they
ought to use match_strdup instead?

Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option

[David Sterba]

On Wed, Aug 21, 2019 at 03:16:11PM +0000, Vladimir Panteleev wrote:
> On Wed, 21 Aug 2019 at 14:35, David Sterba <dsterba@suse.cz> wrote:
> > match_strdup takes an opaque type
> > substring_t that is used by the parser.
> 
> Sorry, how would one determine that the type is opaque?

It's a typedef, which is kind of indication "don't look inside" as the
kernel coding does not normally use typedefs.

> > I've checked some other
> > usages in the tree and the match_strdup/memparse/kstrtoull is quite
> > common.
> 
> Sorry, I also see there are places where substring_t's .from / .to are
> still accessed directly (including in btrfs code). Do you think they
> ought to use match_strdup instead?

You're right, the compression option parsing references ::from heavily.
So for temporary use it would be ok to avoid the allocation, which means
match_strdup would be used only in btrfs_parse_subvol_options.