* [PATCH 0/1] btrfs: Simplify parsing max_inline mount option @ 2019-08-15 2:04 Vladimir Panteleev 2019-08-15 2:04 ` [PATCH 1/1] " Vladimir Panteleev 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Panteleev @ 2019-08-15 2:04 UTC (permalink / raw) To: linux-btrfs; +Cc: 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] btrfs: Simplify parsing max_inline mount option 2019-08-15 2:04 [PATCH 0/1] btrfs: Simplify parsing max_inline mount option Vladimir Panteleev @ 2019-08-15 2:04 ` Vladimir Panteleev 2019-08-15 4:54 ` Anand Jain 2019-08-21 14:35 ` David Sterba 0 siblings, 2 replies; 7+ messages in thread From: Vladimir Panteleev @ 2019-08-15 2:04 UTC (permalink / raw) To: linux-btrfs; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option 2019-08-15 2:04 ` [PATCH 1/1] " Vladimir Panteleev @ 2019-08-15 4:54 ` Anand Jain 2019-08-15 14:54 ` Vladimir Panteleev 2019-08-21 14:35 ` David Sterba 1 sibling, 1 reply; 7+ messages in thread From: Anand Jain @ 2019-08-15 4:54 UTC (permalink / raw) To: Vladimir Panteleev; +Cc: linux-btrfs 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, > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option 2019-08-15 4:54 ` Anand Jain @ 2019-08-15 14:54 ` Vladimir Panteleev 0 siblings, 0 replies; 7+ messages in thread From: Vladimir Panteleev @ 2019-08-15 14:54 UTC (permalink / raw) To: Anand Jain; +Cc: Btrfs BTRFS 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option 2019-08-15 2:04 ` [PATCH 1/1] " Vladimir Panteleev 2019-08-15 4:54 ` Anand Jain @ 2019-08-21 14:35 ` David Sterba 2019-08-21 15:16 ` Vladimir Panteleev 1 sibling, 1 reply; 7+ messages in thread From: David Sterba @ 2019-08-21 14:35 UTC (permalink / raw) To: Vladimir Panteleev; +Cc: linux-btrfs 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option 2019-08-21 14:35 ` David Sterba @ 2019-08-21 15:16 ` Vladimir Panteleev 2019-08-21 16:08 ` David Sterba 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Panteleev @ 2019-08-21 15:16 UTC (permalink / raw) To: dsterba, Btrfs BTRFS 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] btrfs: Simplify parsing max_inline mount option 2019-08-21 15:16 ` Vladimir Panteleev @ 2019-08-21 16:08 ` David Sterba 0 siblings, 0 replies; 7+ messages in thread From: David Sterba @ 2019-08-21 16:08 UTC (permalink / raw) To: Vladimir Panteleev; +Cc: dsterba, Btrfs BTRFS 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-21 16:07 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-15 2:04 [PATCH 0/1] btrfs: Simplify parsing max_inline mount option Vladimir Panteleev 2019-08-15 2:04 ` [PATCH 1/1] " Vladimir Panteleev 2019-08-15 4:54 ` Anand Jain 2019-08-15 14:54 ` Vladimir Panteleev 2019-08-21 14:35 ` David Sterba 2019-08-21 15:16 ` Vladimir Panteleev 2019-08-21 16:08 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).