[PATCH 0/3] fs_parser: handle parameters that can be empty and don't have a value

[PATCH 0/3] fs_parser: handle parameters that can be empty and don't have a value

[Luis Henriques]

While investigating an ext4/053 fstest failure, I realised that when the
flag 'fs_param_can_be_empty' is set in a parameter and it's value is NULL
that parameter isn't being handled as a 'flag' type.  Even if it's type is
set to 'fs_value_is_flag'.  The first patch in this series changes this
behaviour.

Unfortunately, the two filesystems that use this flag (ext4 and overlayfs)
aren't prepared to have the parameter value set to NULL.  Patches #2 and #3
fix this.

Luis Henriques (3):
  fs_parser: handle parameters that can be empty and don't have a value
  ext4: fix mount parameters check for empty values
  overlay: fix mount parameters check for empty values

 fs/ext4/super.c       | 4 ++--
 fs/fs_parser.c        | 3 ++-
 fs/overlayfs/params.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

[PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Luis Henriques]

Currently, only parameters that have the fs_parameter_spec 'type' set to
NULL are handled as 'flag' types.  However, parameters that have the
'fs_param_can_be_empty' flag set and their value is NULL should also be
handled as 'flag' type, as their type is set to 'fs_value_is_flag'.

Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 fs/fs_parser.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index edb3712dcfa5..53f6cb98a3e0 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
 	/* Try to turn the type we were given into the type desired by the
 	 * parameter and give an error if we can't.
 	 */
-	if (is_flag(p)) {
+	if (is_flag(p) ||
+	    (!param->string && (p->flags & fs_param_can_be_empty))) {
 		if (param->type != fs_value_is_flag)
 			return inval_plog(log, "Unexpected value for '%s'",
 				      param->key);

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Christian Brauner]

On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> Currently, only parameters that have the fs_parameter_spec 'type' set to
> NULL are handled as 'flag' types.  However, parameters that have the
> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> 
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
>  fs/fs_parser.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> index edb3712dcfa5..53f6cb98a3e0 100644
> --- a/fs/fs_parser.c
> +++ b/fs/fs_parser.c
> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>  	/* Try to turn the type we were given into the type desired by the
>  	 * parameter and give an error if we can't.
>  	 */
> -	if (is_flag(p)) {
> +	if (is_flag(p) ||
> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
>  		if (param->type != fs_value_is_flag)
>  			return inval_plog(log, "Unexpected value for '%s'",
>  				      param->key);

If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
param->string is guaranteed to not be NULL. So really this is only
about:

FSCONFIG_SET_FD
FSCONFIG_SET_BINARY
FSCONFIG_SET_PATH
FSCONFIG_SET_PATH_EMPTY

and those values being used without a value. What filesystem does this?
I don't see any.

The tempting thing to do here is to to just remove fs_param_can_be_empty
from every helper that isn't fs_param_is_string() until we actually have
a filesystem that wants to use any of the above as flags. Will lose a
lot of code that isn't currently used.

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Luis Henriques]

Christian Brauner <brauner@kernel.org> writes:

> On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
>> Currently, only parameters that have the fs_parameter_spec 'type' set to
>> NULL are handled as 'flag' types.  However, parameters that have the
>> 'fs_param_can_be_empty' flag set and their value is NULL should also be
>> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
>> 
>> Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> ---
>>  fs/fs_parser.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> index edb3712dcfa5..53f6cb98a3e0 100644
>> --- a/fs/fs_parser.c
>> +++ b/fs/fs_parser.c
>> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>>  	/* Try to turn the type we were given into the type desired by the
>>  	 * parameter and give an error if we can't.
>>  	 */
>> -	if (is_flag(p)) {
>> +	if (is_flag(p) ||
>> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
>>  		if (param->type != fs_value_is_flag)
>>  			return inval_plog(log, "Unexpected value for '%s'",
>>  				      param->key);
>
> If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> param->string is guaranteed to not be NULL. So really this is only
> about:
>
> FSCONFIG_SET_FD
> FSCONFIG_SET_BINARY
> FSCONFIG_SET_PATH
> FSCONFIG_SET_PATH_EMPTY
>
> and those values being used without a value. What filesystem does this?
> I don't see any.
>
> The tempting thing to do here is to to just remove fs_param_can_be_empty
> from every helper that isn't fs_param_is_string() until we actually have
> a filesystem that wants to use any of the above as flags. Will lose a
> lot of code that isn't currently used.

Right, I find it quite confusing and I may be fixing the issue in the
wrong place.  What I'm seeing with ext4 when I mount a filesystem using
the option '-o usrjquota' is that fs_parse() will get:

 * p->type is set to fs_param_is_string
   ('p' is a struct fs_parameter_spec, ->type is a function)
 * param->type is set to fs_value_is_flag
   ('param' is a struct fs_parameter, ->type is an enum)

This is because ext4 will use the __fsparam macro to set define a
fs_param_spec as a fs_param_is_string but will also set the
fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
as a flag.  That's why param->string will be NULL in this case.

Cheers,
-- 
Luís

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Christian Brauner]

On Fri, Mar 01, 2024 at 03:45:27PM +0000, Luis Henriques wrote:
> Christian Brauner <brauner@kernel.org> writes:
> 
> > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> >> NULL are handled as 'flag' types.  However, parameters that have the
> >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> >> 
> >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> >> ---
> >>  fs/fs_parser.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> >> index edb3712dcfa5..53f6cb98a3e0 100644
> >> --- a/fs/fs_parser.c
> >> +++ b/fs/fs_parser.c
> >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> >>  	/* Try to turn the type we were given into the type desired by the
> >>  	 * parameter and give an error if we can't.
> >>  	 */
> >> -	if (is_flag(p)) {
> >> +	if (is_flag(p) ||
> >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
> >>  		if (param->type != fs_value_is_flag)
> >>  			return inval_plog(log, "Unexpected value for '%s'",
> >>  				      param->key);
> >
> > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> > param->string is guaranteed to not be NULL. So really this is only
> > about:
> >
> > FSCONFIG_SET_FD
> > FSCONFIG_SET_BINARY
> > FSCONFIG_SET_PATH
> > FSCONFIG_SET_PATH_EMPTY
> >
> > and those values being used without a value. What filesystem does this?
> > I don't see any.
> >
> > The tempting thing to do here is to to just remove fs_param_can_be_empty
> > from every helper that isn't fs_param_is_string() until we actually have
> > a filesystem that wants to use any of the above as flags. Will lose a
> > lot of code that isn't currently used.
> 
> Right, I find it quite confusing and I may be fixing the issue in the
> wrong place.  What I'm seeing with ext4 when I mount a filesystem using
> the option '-o usrjquota' is that fs_parse() will get:
> 
>  * p->type is set to fs_param_is_string
>    ('p' is a struct fs_parameter_spec, ->type is a function)
>  * param->type is set to fs_value_is_flag
>    ('param' is a struct fs_parameter, ->type is an enum)
> 
> This is because ext4 will use the __fsparam macro to set define a
> fs_param_spec as a fs_param_is_string but will also set the
> fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> as a flag.  That's why param->string will be NULL in this case.

Thanks for the details. Let me see if I get this right. So you're saying that
someone is doing:

fsconfig(..., FSCONFIG_SET_FLAG, "usrjquota", NULL, 0); // [1]

? Is so that is a vital part of the explanation. So please put that in the
commit message.

Then ext4 defines:

	fsparam_string_empty ("usrjquota",		Opt_usrjquota),

So [1] gets us:

        param->type == fs_value_is_flag
        param->string == NULL

Now we enter into
fs_parse()
-> __fs_parse()
   -> fs_lookup_key() for @param and that does:

        bool want_flag = param->type == fs_value_is_flag;

        *negated = false;
        for (p = desc; p->name; p++) {
                if (strcmp(p->name, name) != 0)
                        continue;
                if (likely(is_flag(p) == want_flag))
                        return p;
                other = p;
        }

So we don't have a flag parameter defined so the only real match we get is
@other for:

        fsparam_string_empty ("usrjquota",		Opt_usrjquota),

What happens now is that you call p->type == fs_param_is_string() and that
rejects it as bad parameter because param->type == fs_value_is_flag !=
fs_value_is_string as required. So you dont end up getting Opt_userjquota
called with param->string NULL, right? So there's not NULL deref or anything,
right?

You just fail to set usrjquota. Ok, so I think the correct fix is to do
something like the following in ext4:

        fsparam_string_empty ("usrjquota",      Opt_usrjquota),
        fs_param_flag        ("usrjquota",      Opt_usrjquota_flag),

and then in the switch you can do:

switch (opt)
case Opt_usrjquota:
        // string thing
case Opt_usrjquota_flag:
        // flag thing

And I really think we should kill all empty handling for non-string types and
only add that when there's a filesystem that actually needs it.

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Christian Brauner]

On Sat, Mar 02, 2024 at 12:46:41PM +0100, Christian Brauner wrote:
> On Fri, Mar 01, 2024 at 03:45:27PM +0000, Luis Henriques wrote:
> > Christian Brauner <brauner@kernel.org> writes:
> > 
> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> > >> NULL are handled as 'flag' types.  However, parameters that have the
> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> > >> 
> > >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > >> ---
> > >>  fs/fs_parser.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> > >> index edb3712dcfa5..53f6cb98a3e0 100644
> > >> --- a/fs/fs_parser.c
> > >> +++ b/fs/fs_parser.c
> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> > >>  	/* Try to turn the type we were given into the type desired by the
> > >>  	 * parameter and give an error if we can't.
> > >>  	 */
> > >> -	if (is_flag(p)) {
> > >> +	if (is_flag(p) ||
> > >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
> > >>  		if (param->type != fs_value_is_flag)
> > >>  			return inval_plog(log, "Unexpected value for '%s'",
> > >>  				      param->key);
> > >
> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> > > param->string is guaranteed to not be NULL. So really this is only
> > > about:
> > >
> > > FSCONFIG_SET_FD
> > > FSCONFIG_SET_BINARY
> > > FSCONFIG_SET_PATH
> > > FSCONFIG_SET_PATH_EMPTY
> > >
> > > and those values being used without a value. What filesystem does this?
> > > I don't see any.
> > >
> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
> > > from every helper that isn't fs_param_is_string() until we actually have
> > > a filesystem that wants to use any of the above as flags. Will lose a
> > > lot of code that isn't currently used.
> > 
> > Right, I find it quite confusing and I may be fixing the issue in the
> > wrong place.  What I'm seeing with ext4 when I mount a filesystem using
> > the option '-o usrjquota' is that fs_parse() will get:
> > 
> >  * p->type is set to fs_param_is_string
> >    ('p' is a struct fs_parameter_spec, ->type is a function)
> >  * param->type is set to fs_value_is_flag
> >    ('param' is a struct fs_parameter, ->type is an enum)
> > 
> > This is because ext4 will use the __fsparam macro to set define a
> > fs_param_spec as a fs_param_is_string but will also set the
> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> > as a flag.  That's why param->string will be NULL in this case.
> 
> Thanks for the details. Let me see if I get this right. So you're saying that
> someone is doing:
> 
> fsconfig(..., FSCONFIG_SET_FLAG, "usrjquota", NULL, 0); // [1]
> 
> ? Is so that is a vital part of the explanation. So please put that in the
> commit message.
> 
> Then ext4 defines:
> 
> 	fsparam_string_empty ("usrjquota",		Opt_usrjquota),
> 
> So [1] gets us:
> 
>         param->type == fs_value_is_flag
>         param->string == NULL
> 
> Now we enter into
> fs_parse()
> -> __fs_parse()
>    -> fs_lookup_key() for @param and that does:
> 
>         bool want_flag = param->type == fs_value_is_flag;
> 
>         *negated = false;
>         for (p = desc; p->name; p++) {
>                 if (strcmp(p->name, name) != 0)
>                         continue;
>                 if (likely(is_flag(p) == want_flag))
>                         return p;
>                 other = p;
>         }
> 
> So we don't have a flag parameter defined so the only real match we get is
> @other for:
> 
>         fsparam_string_empty ("usrjquota",		Opt_usrjquota),
> 
> What happens now is that you call p->type == fs_param_is_string() and that
> rejects it as bad parameter because param->type == fs_value_is_flag !=
> fs_value_is_string as required. So you dont end up getting Opt_userjquota
> called with param->string NULL, right? So there's not NULL deref or anything,
> right?
> 
> You just fail to set usrjquota. Ok, so I think the correct fix is to do
> something like the following in ext4:
> 
>         fsparam_string_empty ("usrjquota",      Opt_usrjquota),
>         fs_param_flag        ("usrjquota",      Opt_usrjquota_flag),
> 
> and then in the switch you can do:
> 
> switch (opt)
> case Opt_usrjquota:
>         // string thing
> case Opt_usrjquota_flag:
>         // flag thing
> 
> And I really think we should kill all empty handling for non-string types and
> only add that when there's a filesystem that actually needs it.

So one option is to do the following:

From 8bfb142e6caba70704998be072222d6a31d8b97b Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Sat, 2 Mar 2024 18:54:35 +0100
Subject: [PATCH] [UNTESTED]

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/ext4/super.c           | 32 ++++++++++++++++++--------------
 include/linux/fs_parser.h |  3 +++
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ebd97442d1d4..bd625f06ec0f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1724,10 +1724,6 @@ static const struct constant_table ext4_param_dax[] = {
 	{}
 };
 
-/* String parameter that allows empty argument */
-#define fsparam_string_empty(NAME, OPT) \
-	__fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)
-
 /*
  * Mount option specification
  * We don't use fsparam_flag_no because of the way we set the
@@ -1768,10 +1764,8 @@ static const struct fs_parameter_spec ext4_param_specs[] = {
 	fsparam_enum	("data",		Opt_data, ext4_param_data),
 	fsparam_enum	("data_err",		Opt_data_err,
 						ext4_param_data_err),
-	fsparam_string_empty
-			("usrjquota",		Opt_usrjquota),
-	fsparam_string_empty
-			("grpjquota",		Opt_grpjquota),
+	fsparam_string_or_flag ("usrjquota",	Opt_usrjquota),
+	fsparam_string_or_flag ("grpjquota",	Opt_grpjquota),
 	fsparam_enum	("jqfmt",		Opt_jqfmt, ext4_param_jqfmt),
 	fsparam_flag	("grpquota",		Opt_grpquota),
 	fsparam_flag	("quota",		Opt_quota),
@@ -2183,15 +2177,25 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	switch (token) {
 #ifdef CONFIG_QUOTA
 	case Opt_usrjquota:
-		if (!param->string)
-			return unnote_qf_name(fc, USRQUOTA);
-		else
+		if (param->type == fs_value_is_string) {
+			if (!*param->string)
+				return unnote_qf_name(fc, USRQUOTA);
+
 			return note_qf_name(fc, USRQUOTA, param);
+		}
+
+		// param->type == fs_value_is_flag
+		return note_qf_name(fc, USRQUOTA, param);
 	case Opt_grpjquota:
-		if (!param->string)
-			return unnote_qf_name(fc, GRPQUOTA);
-		else
+		if (param->type == fs_value_is_string) {
+			if (!*param->string)
+				return unnote_qf_name(fc, GRPQUOTA);
+
 			return note_qf_name(fc, GRPQUOTA, param);
+		}
+
+		// param->type == fs_value_is_flag
+		return note_qf_name(fc, GRPQUOTA, param);
 #endif
 	case Opt_sb:
 		if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index 01542c4b87a2..4f45141bea95 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -131,5 +131,8 @@ static inline bool fs_validate_description(const char *name,
 #define fsparam_bdev(NAME, OPT)	__fsparam(fs_param_is_blockdev, NAME, OPT, 0, NULL)
 #define fsparam_path(NAME, OPT)	__fsparam(fs_param_is_path, NAME, OPT, 0, NULL)
 #define fsparam_fd(NAME, OPT)	__fsparam(fs_param_is_fd, NAME, OPT, 0, NULL)
+#define fsparam_string_or_flag(NAME, OPT) \
+	__fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL), \
+	fsparam_flag(NAME, OPT)
 
 #endif /* _LINUX_FS_PARSER_H */
-- 
2.43.0

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Luis Henriques]

Christian Brauner <brauner@kernel.org> writes:

> On Sat, Mar 02, 2024 at 12:46:41PM +0100, Christian Brauner wrote:
>> On Fri, Mar 01, 2024 at 03:45:27PM +0000, Luis Henriques wrote:
>> > Christian Brauner <brauner@kernel.org> writes:
>> > 
>> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
>> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
>> > >> NULL are handled as 'flag' types.  However, parameters that have the
>> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
>> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
>> > >> 
>> > >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> > >> ---
>> > >>  fs/fs_parser.c | 3 ++-
>> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> > >> 
>> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> > >> index edb3712dcfa5..53f6cb98a3e0 100644
>> > >> --- a/fs/fs_parser.c
>> > >> +++ b/fs/fs_parser.c
>> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>> > >>  	/* Try to turn the type we were given into the type desired by the
>> > >>  	 * parameter and give an error if we can't.
>> > >>  	 */
>> > >> -	if (is_flag(p)) {
>> > >> +	if (is_flag(p) ||
>> > >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
>> > >>  		if (param->type != fs_value_is_flag)
>> > >>  			return inval_plog(log, "Unexpected value for '%s'",
>> > >>  				      param->key);
>> > >
>> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
>> > > param->string is guaranteed to not be NULL. So really this is only
>> > > about:
>> > >
>> > > FSCONFIG_SET_FD
>> > > FSCONFIG_SET_BINARY
>> > > FSCONFIG_SET_PATH
>> > > FSCONFIG_SET_PATH_EMPTY
>> > >
>> > > and those values being used without a value. What filesystem does this?
>> > > I don't see any.
>> > >
>> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
>> > > from every helper that isn't fs_param_is_string() until we actually have
>> > > a filesystem that wants to use any of the above as flags. Will lose a
>> > > lot of code that isn't currently used.
>> > 
>> > Right, I find it quite confusing and I may be fixing the issue in the
>> > wrong place.  What I'm seeing with ext4 when I mount a filesystem using
>> > the option '-o usrjquota' is that fs_parse() will get:
>> > 
>> >  * p->type is set to fs_param_is_string
>> >    ('p' is a struct fs_parameter_spec, ->type is a function)
>> >  * param->type is set to fs_value_is_flag
>> >    ('param' is a struct fs_parameter, ->type is an enum)
>> > 
>> > This is because ext4 will use the __fsparam macro to set define a
>> > fs_param_spec as a fs_param_is_string but will also set the
>> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
>> > as a flag.  That's why param->string will be NULL in this case.
>> 
>> Thanks for the details. Let me see if I get this right. So you're saying that
>> someone is doing:
>> 
>> fsconfig(..., FSCONFIG_SET_FLAG, "usrjquota", NULL, 0); // [1]
>> 
>> ? Is so that is a vital part of the explanation. So please put that in the
>> commit message.
>> 
>> Then ext4 defines:
>> 
>> 	fsparam_string_empty ("usrjquota",		Opt_usrjquota),
>> 
>> So [1] gets us:
>> 
>>         param->type == fs_value_is_flag
>>         param->string == NULL
>> 
>> Now we enter into
>> fs_parse()
>> -> __fs_parse()
>>    -> fs_lookup_key() for @param and that does:
>> 
>>         bool want_flag = param->type == fs_value_is_flag;
>> 
>>         *negated = false;
>>         for (p = desc; p->name; p++) {
>>                 if (strcmp(p->name, name) != 0)
>>                         continue;
>>                 if (likely(is_flag(p) == want_flag))
>>                         return p;
>>                 other = p;
>>         }
>> 
>> So we don't have a flag parameter defined so the only real match we get is
>> @other for:
>> 
>>         fsparam_string_empty ("usrjquota",		Opt_usrjquota),
>> 
>> What happens now is that you call p->type == fs_param_is_string() and that
>> rejects it as bad parameter because param->type == fs_value_is_flag !=
>> fs_value_is_string as required. So you dont end up getting Opt_userjquota
>> called with param->string NULL, right? So there's not NULL deref or anything,
>> right?
>> 
>> You just fail to set usrjquota. Ok, so I think the correct fix is to do
>> something like the following in ext4:
>> 
>>         fsparam_string_empty ("usrjquota",      Opt_usrjquota),
>>         fs_param_flag        ("usrjquota",      Opt_usrjquota_flag),
>> 
>> and then in the switch you can do:
>> 
>> switch (opt)
>> case Opt_usrjquota:
>>         // string thing
>> case Opt_usrjquota_flag:
>>         // flag thing
>> 
>> And I really think we should kill all empty handling for non-string types and
>> only add that when there's a filesystem that actually needs it.
>
> So one option is to do the following:

Thanks a lot of your review (I forgot to thank you in my other reply!).

Now, I haven't yet tested this properly, but I think that's a much simpler
and cleaner way of fixing this issue.  Now, although it needs some
testing, I think the patch has one problem (see comment below).

Do you want me to send out a cleaned-up version[*] of it after some
testing (+ a similar fix for overlayfs)?  Or would you rather handle this
yourself?

(I'll probably won't be able to look into it until mid-week.)

[*] Obviously, keeping a notice that you were the original author.

> From 8bfb142e6caba70704998be072222d6a31d8b97b Mon Sep 17 00:00:00 2001
> From: Christian Brauner <brauner@kernel.org>
> Date: Sat, 2 Mar 2024 18:54:35 +0100
> Subject: [PATCH] [UNTESTED]
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/ext4/super.c           | 32 ++++++++++++++++++--------------
>  include/linux/fs_parser.h |  3 +++
>  2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index ebd97442d1d4..bd625f06ec0f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1724,10 +1724,6 @@ static const struct constant_table ext4_param_dax[] = {
>  	{}
>  };
>  
> -/* String parameter that allows empty argument */
> -#define fsparam_string_empty(NAME, OPT) \
> -	__fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)
> -
>  /*
>   * Mount option specification
>   * We don't use fsparam_flag_no because of the way we set the
> @@ -1768,10 +1764,8 @@ static const struct fs_parameter_spec ext4_param_specs[] = {
>  	fsparam_enum	("data",		Opt_data, ext4_param_data),
>  	fsparam_enum	("data_err",		Opt_data_err,
>  						ext4_param_data_err),
> -	fsparam_string_empty
> -			("usrjquota",		Opt_usrjquota),
> -	fsparam_string_empty
> -			("grpjquota",		Opt_grpjquota),
> +	fsparam_string_or_flag ("usrjquota",	Opt_usrjquota),
> +	fsparam_string_or_flag ("grpjquota",	Opt_grpjquota),
>  	fsparam_enum	("jqfmt",		Opt_jqfmt, ext4_param_jqfmt),
>  	fsparam_flag	("grpquota",		Opt_grpquota),
>  	fsparam_flag	("quota",		Opt_quota),
> @@ -2183,15 +2177,25 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  	switch (token) {
>  #ifdef CONFIG_QUOTA
>  	case Opt_usrjquota:
> -		if (!param->string)
> -			return unnote_qf_name(fc, USRQUOTA);
> -		else
> +		if (param->type == fs_value_is_string) {
> +			if (!*param->string)
> +				return unnote_qf_name(fc, USRQUOTA);
> +
>  			return note_qf_name(fc, USRQUOTA, param);
> +		}
> +
> +		// param->type == fs_value_is_flag
> +		return note_qf_name(fc, USRQUOTA, param);
                       ^^^^^^^^^^^^
I think this isn't correct -- the correct function to call in this case is
unnote_qf_name(), not note_qf_name().  The same comment applies to the
grpjquota parameter handling.

Cheers,
-- 
Luís

>  	case Opt_grpjquota:
> -		if (!param->string)
> -			return unnote_qf_name(fc, GRPQUOTA);
> -		else
> +		if (param->type == fs_value_is_string) {
> +			if (!*param->string)
> +				return unnote_qf_name(fc, GRPQUOTA);
> +
>  			return note_qf_name(fc, GRPQUOTA, param);
> +		}
> +
> +		// param->type == fs_value_is_flag
> +		return note_qf_name(fc, GRPQUOTA, param);
>  #endif
>  	case Opt_sb:
>  		if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
> diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
> index 01542c4b87a2..4f45141bea95 100644
> --- a/include/linux/fs_parser.h
> +++ b/include/linux/fs_parser.h
> @@ -131,5 +131,8 @@ static inline bool fs_validate_description(const char *name,
>  #define fsparam_bdev(NAME, OPT)	__fsparam(fs_param_is_blockdev, NAME, OPT, 0, NULL)
>  #define fsparam_path(NAME, OPT)	__fsparam(fs_param_is_path, NAME, OPT, 0, NULL)
>  #define fsparam_fd(NAME, OPT)	__fsparam(fs_param_is_fd, NAME, OPT, 0, NULL)
> +#define fsparam_string_or_flag(NAME, OPT) \
> +	__fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL), \
> +	fsparam_flag(NAME, OPT)
>  
>  #endif /* _LINUX_FS_PARSER_H */
> -- 
>
> 2.43.0
>

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Christian Brauner]

On Sun, Mar 03, 2024 at 09:31:42PM +0000, Luis Henriques wrote:
> Christian Brauner <brauner@kernel.org> writes:
> 
> > On Sat, Mar 02, 2024 at 12:46:41PM +0100, Christian Brauner wrote:
> >> On Fri, Mar 01, 2024 at 03:45:27PM +0000, Luis Henriques wrote:
> >> > Christian Brauner <brauner@kernel.org> writes:
> >> > 
> >> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> >> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> >> > >> NULL are handled as 'flag' types.  However, parameters that have the
> >> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> >> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> >> > >> 
> >> > >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> >> > >> ---
> >> > >>  fs/fs_parser.c | 3 ++-
> >> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> > >> 
> >> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> >> > >> index edb3712dcfa5..53f6cb98a3e0 100644
> >> > >> --- a/fs/fs_parser.c
> >> > >> +++ b/fs/fs_parser.c
> >> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> >> > >>  	/* Try to turn the type we were given into the type desired by the
> >> > >>  	 * parameter and give an error if we can't.
> >> > >>  	 */
> >> > >> -	if (is_flag(p)) {
> >> > >> +	if (is_flag(p) ||
> >> > >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
> >> > >>  		if (param->type != fs_value_is_flag)
> >> > >>  			return inval_plog(log, "Unexpected value for '%s'",
> >> > >>  				      param->key);
> >> > >
> >> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> >> > > param->string is guaranteed to not be NULL. So really this is only
> >> > > about:
> >> > >
> >> > > FSCONFIG_SET_FD
> >> > > FSCONFIG_SET_BINARY
> >> > > FSCONFIG_SET_PATH
> >> > > FSCONFIG_SET_PATH_EMPTY
> >> > >
> >> > > and those values being used without a value. What filesystem does this?
> >> > > I don't see any.
> >> > >
> >> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
> >> > > from every helper that isn't fs_param_is_string() until we actually have
> >> > > a filesystem that wants to use any of the above as flags. Will lose a
> >> > > lot of code that isn't currently used.
> >> > 
> >> > Right, I find it quite confusing and I may be fixing the issue in the
> >> > wrong place.  What I'm seeing with ext4 when I mount a filesystem using
> >> > the option '-o usrjquota' is that fs_parse() will get:
> >> > 
> >> >  * p->type is set to fs_param_is_string
> >> >    ('p' is a struct fs_parameter_spec, ->type is a function)
> >> >  * param->type is set to fs_value_is_flag
> >> >    ('param' is a struct fs_parameter, ->type is an enum)
> >> > 
> >> > This is because ext4 will use the __fsparam macro to set define a
> >> > fs_param_spec as a fs_param_is_string but will also set the
> >> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> >> > as a flag.  That's why param->string will be NULL in this case.
> >> 
> >> Thanks for the details. Let me see if I get this right. So you're saying that
> >> someone is doing:
> >> 
> >> fsconfig(..., FSCONFIG_SET_FLAG, "usrjquota", NULL, 0); // [1]
> >> 
> >> ? Is so that is a vital part of the explanation. So please put that in the
> >> commit message.
> >> 
> >> Then ext4 defines:
> >> 
> >> 	fsparam_string_empty ("usrjquota",		Opt_usrjquota),
> >> 
> >> So [1] gets us:
> >> 
> >>         param->type == fs_value_is_flag
> >>         param->string == NULL
> >> 
> >> Now we enter into
> >> fs_parse()
> >> -> __fs_parse()
> >>    -> fs_lookup_key() for @param and that does:
> >> 
> >>         bool want_flag = param->type == fs_value_is_flag;
> >> 
> >>         *negated = false;
> >>         for (p = desc; p->name; p++) {
> >>                 if (strcmp(p->name, name) != 0)
> >>                         continue;
> >>                 if (likely(is_flag(p) == want_flag))
> >>                         return p;
> >>                 other = p;
> >>         }
> >> 
> >> So we don't have a flag parameter defined so the only real match we get is
> >> @other for:
> >> 
> >>         fsparam_string_empty ("usrjquota",		Opt_usrjquota),
> >> 
> >> What happens now is that you call p->type == fs_param_is_string() and that
> >> rejects it as bad parameter because param->type == fs_value_is_flag !=
> >> fs_value_is_string as required. So you dont end up getting Opt_userjquota
> >> called with param->string NULL, right? So there's not NULL deref or anything,
> >> right?
> >> 
> >> You just fail to set usrjquota. Ok, so I think the correct fix is to do
> >> something like the following in ext4:
> >> 
> >>         fsparam_string_empty ("usrjquota",      Opt_usrjquota),
> >>         fs_param_flag        ("usrjquota",      Opt_usrjquota_flag),
> >> 
> >> and then in the switch you can do:
> >> 
> >> switch (opt)
> >> case Opt_usrjquota:
> >>         // string thing
> >> case Opt_usrjquota_flag:
> >>         // flag thing
> >> 
> >> And I really think we should kill all empty handling for non-string types and
> >> only add that when there's a filesystem that actually needs it.
> >
> > So one option is to do the following:
> 
> Thanks a lot of your review (I forgot to thank you in my other reply!).
> 
> Now, I haven't yet tested this properly, but I think that's a much simpler
> and cleaner way of fixing this issue.  Now, although it needs some
> testing, I think the patch has one problem (see comment below).
> 
> Do you want me to send out a cleaned-up version[*] of it after some

Yes, please. That would be great!

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Luis Henriques]

Christian Brauner <brauner@kernel.org> writes:

> On Fri, Mar 01, 2024 at 03:45:27PM +0000, Luis Henriques wrote:
>> Christian Brauner <brauner@kernel.org> writes:
>> 
>> > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
>> >> Currently, only parameters that have the fs_parameter_spec 'type' set to
>> >> NULL are handled as 'flag' types.  However, parameters that have the
>> >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
>> >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
>> >> 
>> >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> >> ---
>> >>  fs/fs_parser.c | 3 ++-
>> >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> >> index edb3712dcfa5..53f6cb98a3e0 100644
>> >> --- a/fs/fs_parser.c
>> >> +++ b/fs/fs_parser.c
>> >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>> >>  	/* Try to turn the type we were given into the type desired by the
>> >>  	 * parameter and give an error if we can't.
>> >>  	 */
>> >> -	if (is_flag(p)) {
>> >> +	if (is_flag(p) ||
>> >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
>> >>  		if (param->type != fs_value_is_flag)
>> >>  			return inval_plog(log, "Unexpected value for '%s'",
>> >>  				      param->key);
>> >
>> > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
>> > param->string is guaranteed to not be NULL. So really this is only
>> > about:
>> >
>> > FSCONFIG_SET_FD
>> > FSCONFIG_SET_BINARY
>> > FSCONFIG_SET_PATH
>> > FSCONFIG_SET_PATH_EMPTY
>> >
>> > and those values being used without a value. What filesystem does this?
>> > I don't see any.
>> >
>> > The tempting thing to do here is to to just remove fs_param_can_be_empty
>> > from every helper that isn't fs_param_is_string() until we actually have
>> > a filesystem that wants to use any of the above as flags. Will lose a
>> > lot of code that isn't currently used.
>> 
>> Right, I find it quite confusing and I may be fixing the issue in the
>> wrong place.  What I'm seeing with ext4 when I mount a filesystem using
>> the option '-o usrjquota' is that fs_parse() will get:
>> 
>>  * p->type is set to fs_param_is_string
>>    ('p' is a struct fs_parameter_spec, ->type is a function)
>>  * param->type is set to fs_value_is_flag
>>    ('param' is a struct fs_parameter, ->type is an enum)
>> 
>> This is because ext4 will use the __fsparam macro to set define a
>> fs_param_spec as a fs_param_is_string but will also set the
>> fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
>> as a flag.  That's why param->string will be NULL in this case.
>
> Thanks for the details. Let me see if I get this right. So you're saying that
> someone is doing:
>
> fsconfig(..., FSCONFIG_SET_FLAG, "usrjquota", NULL, 0); // [1]
>
> ? Is so that is a vital part of the explanation. So please put that in the
> commit message.

Right, I guess I should have added a simple usecase for that in the commit
message.  I.e. add a simple 'mount' command with this parameter without
any value.

> Then ext4 defines:
>
> 	fsparam_string_empty ("usrjquota",		Opt_usrjquota),
>
> So [1] gets us:
>
>         param->type == fs_value_is_flag
>         param->string == NULL
>
> Now we enter into
> fs_parse()
> -> __fs_parse()
>    -> fs_lookup_key() for @param and that does:
>
>         bool want_flag = param->type == fs_value_is_flag;
>
>         *negated = false;
>         for (p = desc; p->name; p++) {
>                 if (strcmp(p->name, name) != 0)
>                         continue;
>                 if (likely(is_flag(p) == want_flag))
>                         return p;
>                 other = p;
>         }
>
> So we don't have a flag parameter defined so the only real match we get is
> @other for:
>
>         fsparam_string_empty ("usrjquota",		Opt_usrjquota),
>
> What happens now is that you call p->type == fs_param_is_string() and that
> rejects it as bad parameter because param->type == fs_value_is_flag !=
> fs_value_is_string as required. So you dont end up getting Opt_userjquota
> called with param->string NULL, right? So there's not NULL deref or anything,
> right?
>
> You just fail to set usrjquota. Ok, so I think the correct fix is to do
> something like the following in ext4:
>
>         fsparam_string_empty ("usrjquota",      Opt_usrjquota),
>         fs_param_flag        ("usrjquota",      Opt_usrjquota_flag),
>
> and then in the switch you can do:
>
> switch (opt)
> case Opt_usrjquota:
>         // string thing
> case Opt_usrjquota_flag:
>         // flag thing
>
> And I really think we should kill all empty handling for non-string types and
> only add that when there's a filesystem that actually needs it.

Yeah, that looks like the right fix.  I see you sent out a patch doing
something like this, so I'll comment there instead.

Cheers,
-- 
Luís

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Jan Kara]

On Fri 01-03-24 15:45:27, Luis Henriques wrote:
> Christian Brauner <brauner@kernel.org> writes:
> 
> > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> >> NULL are handled as 'flag' types.  However, parameters that have the
> >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> >> 
> >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> >> ---
> >>  fs/fs_parser.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> >> index edb3712dcfa5..53f6cb98a3e0 100644
> >> --- a/fs/fs_parser.c
> >> +++ b/fs/fs_parser.c
> >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> >>  	/* Try to turn the type we were given into the type desired by the
> >>  	 * parameter and give an error if we can't.
> >>  	 */
> >> -	if (is_flag(p)) {
> >> +	if (is_flag(p) ||
> >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
> >>  		if (param->type != fs_value_is_flag)
> >>  			return inval_plog(log, "Unexpected value for '%s'",
> >>  				      param->key);
> >
> > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> > param->string is guaranteed to not be NULL. So really this is only
> > about:
> >
> > FSCONFIG_SET_FD
> > FSCONFIG_SET_BINARY
> > FSCONFIG_SET_PATH
> > FSCONFIG_SET_PATH_EMPTY
> >
> > and those values being used without a value. What filesystem does this?
> > I don't see any.
> >
> > The tempting thing to do here is to to just remove fs_param_can_be_empty
> > from every helper that isn't fs_param_is_string() until we actually have
> > a filesystem that wants to use any of the above as flags. Will lose a
> > lot of code that isn't currently used.
> 
> Right, I find it quite confusing and I may be fixing the issue in the
> wrong place.  What I'm seeing with ext4 when I mount a filesystem using
> the option '-o usrjquota' is that fs_parse() will get:
> 
>  * p->type is set to fs_param_is_string
>    ('p' is a struct fs_parameter_spec, ->type is a function)
>  * param->type is set to fs_value_is_flag
>    ('param' is a struct fs_parameter, ->type is an enum)
> 
> This is because ext4 will use the __fsparam macro to set define a
> fs_param_spec as a fs_param_is_string but will also set the
> fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> as a flag.  That's why param->string will be NULL in this case.

So I'm a bit confused here. Valid variants of these quota options are like
"usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
quota file name). The variant "usrjquota" should ideally be rejected
because it doesn't make a good sense and only adds to confusion. Now as far
as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
shows) this is what is happening so what is exactly the problem you're
trying to fix?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Christian Brauner]

On Thu, Mar 07, 2024 at 04:13:56PM +0100, Jan Kara wrote:
> On Fri 01-03-24 15:45:27, Luis Henriques wrote:
> > Christian Brauner <brauner@kernel.org> writes:
> > 
> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> > >> NULL are handled as 'flag' types.  However, parameters that have the
> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> > >> 
> > >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > >> ---
> > >>  fs/fs_parser.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> > >> index edb3712dcfa5..53f6cb98a3e0 100644
> > >> --- a/fs/fs_parser.c
> > >> +++ b/fs/fs_parser.c
> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> > >>  	/* Try to turn the type we were given into the type desired by the
> > >>  	 * parameter and give an error if we can't.
> > >>  	 */
> > >> -	if (is_flag(p)) {
> > >> +	if (is_flag(p) ||
> > >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
> > >>  		if (param->type != fs_value_is_flag)
> > >>  			return inval_plog(log, "Unexpected value for '%s'",
> > >>  				      param->key);
> > >
> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> > > param->string is guaranteed to not be NULL. So really this is only
> > > about:
> > >
> > > FSCONFIG_SET_FD
> > > FSCONFIG_SET_BINARY
> > > FSCONFIG_SET_PATH
> > > FSCONFIG_SET_PATH_EMPTY
> > >
> > > and those values being used without a value. What filesystem does this?
> > > I don't see any.
> > >
> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
> > > from every helper that isn't fs_param_is_string() until we actually have
> > > a filesystem that wants to use any of the above as flags. Will lose a
> > > lot of code that isn't currently used.
> > 
> > Right, I find it quite confusing and I may be fixing the issue in the
> > wrong place.  What I'm seeing with ext4 when I mount a filesystem using
> > the option '-o usrjquota' is that fs_parse() will get:
> > 
> >  * p->type is set to fs_param_is_string
> >    ('p' is a struct fs_parameter_spec, ->type is a function)
> >  * param->type is set to fs_value_is_flag
> >    ('param' is a struct fs_parameter, ->type is an enum)
> > 
> > This is because ext4 will use the __fsparam macro to set define a
> > fs_param_spec as a fs_param_is_string but will also set the
> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> > as a flag.  That's why param->string will be NULL in this case.
> 
> So I'm a bit confused here. Valid variants of these quota options are like
> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
> quota file name). The variant "usrjquota" should ideally be rejected
> because it doesn't make a good sense and only adds to confusion. Now as far
> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
> shows) this is what is happening so what is exactly the problem you're
> trying to fix?

mount(8) has no way of easily knowing that for something like
mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
indistinguishable from a flag because it's specified without an
argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
we should require mount(8) to know what mount options are strings or no.
I've ran into this issue before myself when using the mount api
programatically.

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Luis Henriques]

Christian Brauner <brauner@kernel.org> writes:

> On Thu, Mar 07, 2024 at 04:13:56PM +0100, Jan Kara wrote:
>> On Fri 01-03-24 15:45:27, Luis Henriques wrote:
>> > Christian Brauner <brauner@kernel.org> writes:
>> > 
>> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
>> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
>> > >> NULL are handled as 'flag' types.  However, parameters that have the
>> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
>> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
>> > >> 
>> > >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> > >> ---
>> > >>  fs/fs_parser.c | 3 ++-
>> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> > >> 
>> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> > >> index edb3712dcfa5..53f6cb98a3e0 100644
>> > >> --- a/fs/fs_parser.c
>> > >> +++ b/fs/fs_parser.c
>> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>> > >>  	/* Try to turn the type we were given into the type desired by the
>> > >>  	 * parameter and give an error if we can't.
>> > >>  	 */
>> > >> -	if (is_flag(p)) {
>> > >> +	if (is_flag(p) ||
>> > >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
>> > >>  		if (param->type != fs_value_is_flag)
>> > >>  			return inval_plog(log, "Unexpected value for '%s'",
>> > >>  				      param->key);
>> > >
>> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
>> > > param->string is guaranteed to not be NULL. So really this is only
>> > > about:
>> > >
>> > > FSCONFIG_SET_FD
>> > > FSCONFIG_SET_BINARY
>> > > FSCONFIG_SET_PATH
>> > > FSCONFIG_SET_PATH_EMPTY
>> > >
>> > > and those values being used without a value. What filesystem does this?
>> > > I don't see any.
>> > >
>> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
>> > > from every helper that isn't fs_param_is_string() until we actually have
>> > > a filesystem that wants to use any of the above as flags. Will lose a
>> > > lot of code that isn't currently used.
>> > 
>> > Right, I find it quite confusing and I may be fixing the issue in the
>> > wrong place.  What I'm seeing with ext4 when I mount a filesystem using
>> > the option '-o usrjquota' is that fs_parse() will get:
>> > 
>> >  * p->type is set to fs_param_is_string
>> >    ('p' is a struct fs_parameter_spec, ->type is a function)
>> >  * param->type is set to fs_value_is_flag
>> >    ('param' is a struct fs_parameter, ->type is an enum)
>> > 
>> > This is because ext4 will use the __fsparam macro to set define a
>> > fs_param_spec as a fs_param_is_string but will also set the
>> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
>> > as a flag.  That's why param->string will be NULL in this case.
>> 
>> So I'm a bit confused here. Valid variants of these quota options are like
>> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
>> quota file name). The variant "usrjquota" should ideally be rejected
>> because it doesn't make a good sense and only adds to confusion. Now as far
>> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
>> shows) this is what is happening so what is exactly the problem you're
>> trying to fix?
>
> mount(8) has no way of easily knowing that for something like
> mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
> set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
> indistinguishable from a flag because it's specified without an
> argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
> we should require mount(8) to know what mount options are strings or no.
> I've ran into this issue before myself when using the mount api
> programatically.

Right.  A simple usecase is to try to do:

  mount -t ext4 -o usrjquota= /dev/sda1 /mnt/

It will fail, and this has been broken for a while.

(And btw: email here is broken again -- I haven't received Jan's email
yet.  And this reply will likely take a while to reach its recipients.)

Cheers,
-- 
Luís

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Jan Kara]

On Fri 08-03-24 10:12:13, Luis Henriques wrote:
> Christian Brauner <brauner@kernel.org> writes:
> 
> > On Thu, Mar 07, 2024 at 04:13:56PM +0100, Jan Kara wrote:
> >> On Fri 01-03-24 15:45:27, Luis Henriques wrote:
> >> > Christian Brauner <brauner@kernel.org> writes:
> >> > 
> >> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> >> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> >> > >> NULL are handled as 'flag' types.  However, parameters that have the
> >> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> >> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> >> > >> 
> >> > >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> >> > >> ---
> >> > >>  fs/fs_parser.c | 3 ++-
> >> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> > >> 
> >> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> >> > >> index edb3712dcfa5..53f6cb98a3e0 100644
> >> > >> --- a/fs/fs_parser.c
> >> > >> +++ b/fs/fs_parser.c
> >> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> >> > >>  	/* Try to turn the type we were given into the type desired by the
> >> > >>  	 * parameter and give an error if we can't.
> >> > >>  	 */
> >> > >> -	if (is_flag(p)) {
> >> > >> +	if (is_flag(p) ||
> >> > >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
> >> > >>  		if (param->type != fs_value_is_flag)
> >> > >>  			return inval_plog(log, "Unexpected value for '%s'",
> >> > >>  				      param->key);
> >> > >
> >> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> >> > > param->string is guaranteed to not be NULL. So really this is only
> >> > > about:
> >> > >
> >> > > FSCONFIG_SET_FD
> >> > > FSCONFIG_SET_BINARY
> >> > > FSCONFIG_SET_PATH
> >> > > FSCONFIG_SET_PATH_EMPTY
> >> > >
> >> > > and those values being used without a value. What filesystem does this?
> >> > > I don't see any.
> >> > >
> >> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
> >> > > from every helper that isn't fs_param_is_string() until we actually have
> >> > > a filesystem that wants to use any of the above as flags. Will lose a
> >> > > lot of code that isn't currently used.
> >> > 
> >> > Right, I find it quite confusing and I may be fixing the issue in the
> >> > wrong place.  What I'm seeing with ext4 when I mount a filesystem using
> >> > the option '-o usrjquota' is that fs_parse() will get:
> >> > 
> >> >  * p->type is set to fs_param_is_string
> >> >    ('p' is a struct fs_parameter_spec, ->type is a function)
> >> >  * param->type is set to fs_value_is_flag
> >> >    ('param' is a struct fs_parameter, ->type is an enum)
> >> > 
> >> > This is because ext4 will use the __fsparam macro to set define a
> >> > fs_param_spec as a fs_param_is_string but will also set the
> >> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> >> > as a flag.  That's why param->string will be NULL in this case.
> >> 
> >> So I'm a bit confused here. Valid variants of these quota options are like
> >> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
> >> quota file name). The variant "usrjquota" should ideally be rejected
> >> because it doesn't make a good sense and only adds to confusion. Now as far
> >> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
> >> shows) this is what is happening so what is exactly the problem you're
> >> trying to fix?
> >
> > mount(8) has no way of easily knowing that for something like
> > mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
> > set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
> > indistinguishable from a flag because it's specified without an
> > argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
> > we should require mount(8) to know what mount options are strings or no.
> > I've ran into this issue before myself when using the mount api
> > programatically.
> 
> Right.  A simple usecase is to try to do:
> 
>   mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
> 
> It will fail, and this has been broken for a while.

I see. But you have to have new enough mount that is using fsconfig, don't
you? Because for me in my test VM this works just fine...

But anyway, I get the point. Thanks for educating me :)

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Luis Henriques]

Jan Kara <jack@suse.cz> writes:

> On Fri 08-03-24 10:12:13, Luis Henriques wrote:
>> Christian Brauner <brauner@kernel.org> writes:
>> 
>> > On Thu, Mar 07, 2024 at 04:13:56PM +0100, Jan Kara wrote:
>> >> On Fri 01-03-24 15:45:27, Luis Henriques wrote:
>> >> > Christian Brauner <brauner@kernel.org> writes:
>> >> > 
>> >> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
>> >> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
>> >> > >> NULL are handled as 'flag' types.  However, parameters that have the
>> >> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
>> >> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
>> >> > >> 
>> >> > >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> >> > >> ---
>> >> > >>  fs/fs_parser.c | 3 ++-
>> >> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> >> > >> 
>> >> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> >> > >> index edb3712dcfa5..53f6cb98a3e0 100644
>> >> > >> --- a/fs/fs_parser.c
>> >> > >> +++ b/fs/fs_parser.c
>> >> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>> >> > >>  	/* Try to turn the type we were given into the type desired by the
>> >> > >>  	 * parameter and give an error if we can't.
>> >> > >>  	 */
>> >> > >> -	if (is_flag(p)) {
>> >> > >> +	if (is_flag(p) ||
>> >> > >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
>> >> > >>  		if (param->type != fs_value_is_flag)
>> >> > >>  			return inval_plog(log, "Unexpected value for '%s'",
>> >> > >>  				      param->key);
>> >> > >
>> >> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
>> >> > > param->string is guaranteed to not be NULL. So really this is only
>> >> > > about:
>> >> > >
>> >> > > FSCONFIG_SET_FD
>> >> > > FSCONFIG_SET_BINARY
>> >> > > FSCONFIG_SET_PATH
>> >> > > FSCONFIG_SET_PATH_EMPTY
>> >> > >
>> >> > > and those values being used without a value. What filesystem does this?
>> >> > > I don't see any.
>> >> > >
>> >> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
>> >> > > from every helper that isn't fs_param_is_string() until we actually have
>> >> > > a filesystem that wants to use any of the above as flags. Will lose a
>> >> > > lot of code that isn't currently used.
>> >> > 
>> >> > Right, I find it quite confusing and I may be fixing the issue in the
>> >> > wrong place.  What I'm seeing with ext4 when I mount a filesystem using
>> >> > the option '-o usrjquota' is that fs_parse() will get:
>> >> > 
>> >> >  * p->type is set to fs_param_is_string
>> >> >    ('p' is a struct fs_parameter_spec, ->type is a function)
>> >> >  * param->type is set to fs_value_is_flag
>> >> >    ('param' is a struct fs_parameter, ->type is an enum)
>> >> > 
>> >> > This is because ext4 will use the __fsparam macro to set define a
>> >> > fs_param_spec as a fs_param_is_string but will also set the
>> >> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
>> >> > as a flag.  That's why param->string will be NULL in this case.
>> >> 
>> >> So I'm a bit confused here. Valid variants of these quota options are like
>> >> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
>> >> quota file name). The variant "usrjquota" should ideally be rejected
>> >> because it doesn't make a good sense and only adds to confusion. Now as far
>> >> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
>> >> shows) this is what is happening so what is exactly the problem you're
>> >> trying to fix?
>> >
>> > mount(8) has no way of easily knowing that for something like
>> > mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
>> > set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
>> > indistinguishable from a flag because it's specified without an
>> > argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
>> > we should require mount(8) to know what mount options are strings or no.
>> > I've ran into this issue before myself when using the mount api
>> > programatically.
>> 
>> Right.  A simple usecase is to try to do:
>> 
>>   mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
>> 
>> It will fail, and this has been broken for a while.
>
> I see. But you have to have new enough mount that is using fsconfig, don't
> you? Because for me in my test VM this works just fine...

Oh, interesting.  FTR I'm using mount from util-linux 2.39.3, but I
haven't tried this with older versions.

Cheers,
-- 
Luís


>
> But anyway, I get the point. Thanks for educating me :)
>
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and don't have a value

[Jan Kara]

On Mon 11-03-24 10:26:05, Luis Henriques wrote:
> Jan Kara <jack@suse.cz> writes:
> > On Fri 08-03-24 10:12:13, Luis Henriques wrote:
> >> Christian Brauner <brauner@kernel.org> writes:
> >> 
> >> > On Thu, Mar 07, 2024 at 04:13:56PM +0100, Jan Kara wrote:
> >> >> On Fri 01-03-24 15:45:27, Luis Henriques wrote:
> >> >> > Christian Brauner <brauner@kernel.org> writes:
> >> >> > 
> >> >> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> >> >> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> >> >> > >> NULL are handled as 'flag' types.  However, parameters that have the
> >> >> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> >> >> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> >> >> > >> 
> >> >> > >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> >> >> > >> ---
> >> >> > >>  fs/fs_parser.c | 3 ++-
> >> >> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> >> > >> 
> >> >> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> >> >> > >> index edb3712dcfa5..53f6cb98a3e0 100644
> >> >> > >> --- a/fs/fs_parser.c
> >> >> > >> +++ b/fs/fs_parser.c
> >> >> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> >> >> > >>  	/* Try to turn the type we were given into the type desired by the
> >> >> > >>  	 * parameter and give an error if we can't.
> >> >> > >>  	 */
> >> >> > >> -	if (is_flag(p)) {
> >> >> > >> +	if (is_flag(p) ||
> >> >> > >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
> >> >> > >>  		if (param->type != fs_value_is_flag)
> >> >> > >>  			return inval_plog(log, "Unexpected value for '%s'",
> >> >> > >>  				      param->key);
> >> >> > >
> >> >> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> >> >> > > param->string is guaranteed to not be NULL. So really this is only
> >> >> > > about:
> >> >> > >
> >> >> > > FSCONFIG_SET_FD
> >> >> > > FSCONFIG_SET_BINARY
> >> >> > > FSCONFIG_SET_PATH
> >> >> > > FSCONFIG_SET_PATH_EMPTY
> >> >> > >
> >> >> > > and those values being used without a value. What filesystem does this?
> >> >> > > I don't see any.
> >> >> > >
> >> >> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
> >> >> > > from every helper that isn't fs_param_is_string() until we actually have
> >> >> > > a filesystem that wants to use any of the above as flags. Will lose a
> >> >> > > lot of code that isn't currently used.
> >> >> > 
> >> >> > Right, I find it quite confusing and I may be fixing the issue in the
> >> >> > wrong place.  What I'm seeing with ext4 when I mount a filesystem using
> >> >> > the option '-o usrjquota' is that fs_parse() will get:
> >> >> > 
> >> >> >  * p->type is set to fs_param_is_string
> >> >> >    ('p' is a struct fs_parameter_spec, ->type is a function)
> >> >> >  * param->type is set to fs_value_is_flag
> >> >> >    ('param' is a struct fs_parameter, ->type is an enum)
> >> >> > 
> >> >> > This is because ext4 will use the __fsparam macro to set define a
> >> >> > fs_param_spec as a fs_param_is_string but will also set the
> >> >> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> >> >> > as a flag.  That's why param->string will be NULL in this case.
> >> >> 
> >> >> So I'm a bit confused here. Valid variants of these quota options are like
> >> >> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
> >> >> quota file name). The variant "usrjquota" should ideally be rejected
> >> >> because it doesn't make a good sense and only adds to confusion. Now as far
> >> >> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
> >> >> shows) this is what is happening so what is exactly the problem you're
> >> >> trying to fix?
> >> >
> >> > mount(8) has no way of easily knowing that for something like
> >> > mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
> >> > set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
> >> > indistinguishable from a flag because it's specified without an
> >> > argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
> >> > we should require mount(8) to know what mount options are strings or no.
> >> > I've ran into this issue before myself when using the mount api
> >> > programatically.
> >> 
> >> Right.  A simple usecase is to try to do:
> >> 
> >>   mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
> >> 
> >> It will fail, and this has been broken for a while.
> >
> > I see. But you have to have new enough mount that is using fsconfig, don't
> > you? Because for me in my test VM this works just fine...
> 
> Oh, interesting.  FTR I'm using mount from util-linux 2.39.3, but I
> haven't tried this with older versions.

I'm using util-linux 2.37.2 and checking the changelogs indeed 2.39 started
to use the new mount API from the kernel. Checking strace of the new mount
I can indeed see mount(8) does:

fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)

So it is actually util-linux, not the kernel parser, that IMHO incorrectly
parses the mount options and uses FSCONFIG_SET_FLAG instead of
FSCONFIG_SET_STRING with an empty string.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/3] ext4: fix mount parameters check for empty values

[Luis Henriques]

Now that parameters that have the flag 'fs_param_can_be_empty' set and
their value is NULL are handled as 'flag' type, we need to properly check
for empty (NULL) values.

Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 fs/ext4/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 0f931d0c227d..44ba2212dfb3 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2183,12 +2183,12 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	switch (token) {
 #ifdef CONFIG_QUOTA
 	case Opt_usrjquota:
-		if (!*param->string)
+		if (!param->string)
 			return unnote_qf_name(fc, USRQUOTA);
 		else
 			return note_qf_name(fc, USRQUOTA, param);
 	case Opt_grpjquota:
-		if (!*param->string)
+		if (!param->string)
 			return unnote_qf_name(fc, GRPQUOTA);
 		else
 			return note_qf_name(fc, GRPQUOTA, param);

Re: [PATCH 2/3] ext4: fix mount parameters check for empty values

[Christian Brauner]

On Thu, Feb 29, 2024 at 04:30:09PM +0000, Luis Henriques wrote:
> Now that parameters that have the flag 'fs_param_can_be_empty' set and
> their value is NULL are handled as 'flag' type, we need to properly check
> for empty (NULL) values.
> 
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
>  fs/ext4/super.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 0f931d0c227d..44ba2212dfb3 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2183,12 +2183,12 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  	switch (token) {
>  #ifdef CONFIG_QUOTA
>  	case Opt_usrjquota:
> -		if (!*param->string)
> +		if (!param->string)
>  			return unnote_qf_name(fc, USRQUOTA);

I fail to understand how that can happen. Currently both of these
options are parsed as strings via:

#define fsparam_string_empty(NAME, OPT) \
        __fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)


So if someone sets fsconfig(..., FSCONFIG_SET_STRING, "usrquota", NULL, ...)
we give an immediate

        case FSCONFIG_SET_STRING:
                if (!_key || !_value || aux) return -EINVAL;

from fsconfig() so we know that param->string cannot be NULL. If that
were the case we'd NULL deref in fs_param_is_string():

int fs_param_is_string(struct p_log *log, const struct fs_parameter_spec *p,
                       struct fs_parameter *param, struct fs_parse_result *result)
{
        if (param->type != fs_value_is_string ||
            (!*param->string && !(p->flags & fs_param_can_be_empty)))

So you're check above seems wrong. If I'm mistaken, please explain, how
this can happen in detail.

Re: [PATCH 2/3] ext4: fix mount parameters check for empty values

[Luis Henriques]

Christian Brauner <brauner@kernel.org> writes:

> On Thu, Feb 29, 2024 at 04:30:09PM +0000, Luis Henriques wrote:
>> Now that parameters that have the flag 'fs_param_can_be_empty' set and
>> their value is NULL are handled as 'flag' type, we need to properly check
>> for empty (NULL) values.
>> 
>> Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> ---
>>  fs/ext4/super.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 0f931d0c227d..44ba2212dfb3 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -2183,12 +2183,12 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>>  	switch (token) {
>>  #ifdef CONFIG_QUOTA
>>  	case Opt_usrjquota:
>> -		if (!*param->string)
>> +		if (!param->string)
>>  			return unnote_qf_name(fc, USRQUOTA);
>
> I fail to understand how that can happen. Currently both of these
> options are parsed as strings via:
>
> #define fsparam_string_empty(NAME, OPT) \
>         __fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)
>
>
> So if someone sets fsconfig(..., FSCONFIG_SET_STRING, "usrquota", NULL, ...)
> we give an immediate
>
>         case FSCONFIG_SET_STRING:
>                 if (!_key || !_value || aux) return -EINVAL;
>
> from fsconfig() so we know that param->string cannot be NULL. If that
> were the case we'd NULL deref in fs_param_is_string():
>
> int fs_param_is_string(struct p_log *log, const struct fs_parameter_spec *p,
>                        struct fs_parameter *param, struct fs_parse_result *result)
> {
>         if (param->type != fs_value_is_string ||
>             (!*param->string && !(p->flags & fs_param_can_be_empty)))
>
> So you're check above seems wrong. If I'm mistaken, please explain, how
> this can happen in detail.

I hope my reply to the previous patch helps clarifying this issue (which
is quite confusing, and I'm probably  the confused one!).  To summarize,
fsconfig() will (or can) get this parameter as a flag, not as string.

Cheers,
-- 
Luís

[PATCH 3/3] overlay: fix mount parameters check for empty values

[Luis Henriques]

Now that parameters that have the flag 'fs_param_can_be_empty' set and
their value is NULL are handled as 'flag' type, we need to properly check
for empty (NULL) values.

Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 fs/overlayfs/params.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 112b4b12f825..09428af6abc5 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -441,7 +441,7 @@ static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc)
 	/* drop all existing lower layers */
 	ovl_reset_lowerdirs(ctx);
 
-	if (!*name)
+	if (!name)
 		return 0;
 
 	if (*name == ':') {

Re: [PATCH 3/3] overlay: fix mount parameters check for empty values

[Christian Brauner]

On Thu, Feb 29, 2024 at 04:30:10PM +0000, Luis Henriques wrote:
> Now that parameters that have the flag 'fs_param_can_be_empty' set and
> their value is NULL are handled as 'flag' type, we need to properly check
> for empty (NULL) values.
> 
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
>  fs/overlayfs/params.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 112b4b12f825..09428af6abc5 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -441,7 +441,7 @@ static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc)
>  	/* drop all existing lower layers */
>  	ovl_reset_lowerdirs(ctx);
>  
> -	if (!*name)
> +	if (!name)

Same comment/qestion as on ext4 patch.

Re: [PATCH 0/3] fs_parser: handle parameters that can be empty and don't have a value

[Christian Brauner]

> Unfortunately, the two filesystems that use this flag (ext4 and overlayfs)
> aren't prepared to have the parameter value set to NULL.  Patches #2 and #3
> fix this.

Both ext4 and overlayfs define

#define fsparam_string_empty(NAME, OPT) \
        __fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)

Please add that to include/linux/fs_parser.h so that it can be used by both.

Re: [PATCH 0/3] fs_parser: handle parameters that can be empty and don't have a value

[Eric Sandeen]

On 3/1/24 7:12 AM, Christian Brauner wrote:
>> Unfortunately, the two filesystems that use this flag (ext4 and overlayfs)
>> aren't prepared to have the parameter value set to NULL.  Patches #2 and #3
>> fix this.
> 
> Both ext4 and overlayfs define
> 
> #define fsparam_string_empty(NAME, OPT) \
>         __fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)
> 
> Please add that to include/linux/fs_parser.h so that it can be used by both.

The f2fs conversion will also need this, so yes, it'd be nice to hoist it out
of ext4 and overlayfs.

-Eric