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

[PATCH v2 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 there was
an issue when the flag 'fs_param_can_be_empty' is set in a parameter and it
doesn't have a value

After an initial attempt to fix the issue, Christian suggested a different
approach and the following patches are based in his suggestion.

Another change that I'm introducing in this v2 is the change of parameter
'test_dummy_encryption' to also use the new helper introduced by the first
patch in this series.

Finally, I'd like to ask someone to look closer at the overlayfs patch as
I don't think there were any fstests to validate the case where 'lowerdir'
is empty.

Luis Henriques (3):
  fs_parser: add helper to define parameters with string and flag types
  ext4: fix the parsing of empty string mount parameters
  ovl: fix the parsing of empty string mount parameters

 fs/ext4/super.c           | 25 ++++++++++---------------
 fs/overlayfs/params.c     | 13 +++++--------
 include/linux/fs_parser.h |  8 ++++++++
 3 files changed, 23 insertions(+), 23 deletions(-)

[PATCH v2 1/3] fs_parser: add helper to define parameters with string and flag types

[Luis Henriques]

Add a new helper macro that defines two new parameters, both with the same
name, but one of type 'string' and another of type 'flag'.  The 'string'
parameter may also be empty (i.e. without value).  In practice this helper
allows a filesystem to easily define a parameter that can be empty (flag)
or have a value (string).

Suggested-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 include/linux/fs_parser.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index 01542c4b87a2..f582fb7bdc22 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -131,5 +131,13 @@ 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 two parameters with the same name, with types string and flag.  The
+ * string parameter can be empty, and thus it effectively allows the parameter
+ * to have a value or to be empty.
+ */
+#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 */

[PATCH v2 2/3] ext4: fix the parsing of empty string mount parameters

[Luis Henriques]

This patch fixes the usage of mount parameters that are defined as strings
but which can be empty.  Currently, only 'usrjquota' and 'grpjquota'
parameters are in this situation for ext4.  But since userspace can pass
them in as 'flag' types (when they don't have a value), the parsing will
fail because a 'string' type is assumed.

This issue is fixed by using the new helper fsparam_string_or_flag() and by
also taking the parameter type into account.

While there, also remove the now unused fsparam_string_empty() macro and
change the 'test_dummy_encryption' parameter to also use the new fs_parser
helper.

Suggested-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 fs/ext4/super.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 0f931d0c227d..5a2f178f8fe9 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,9 +1764,9 @@ 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
+	fsparam_string_or_flag
 			("usrjquota",		Opt_usrjquota),
-	fsparam_string_empty
+	fsparam_string_or_flag
 			("grpjquota",		Opt_grpjquota),
 	fsparam_enum	("jqfmt",		Opt_jqfmt, ext4_param_jqfmt),
 	fsparam_flag	("grpquota",		Opt_grpquota),
@@ -1814,9 +1810,8 @@ static const struct fs_parameter_spec ext4_param_specs[] = {
 	fsparam_u32	("fc_debug_max_replay",	Opt_fc_debug_max_replay),
 #endif
 	fsparam_u32	("max_dir_size_kb",	Opt_max_dir_size_kb),
-	fsparam_flag	("test_dummy_encryption",
-						Opt_test_dummy_encryption),
-	fsparam_string	("test_dummy_encryption",
+	fsparam_string_or_flag
+			("test_dummy_encryption",
 						Opt_test_dummy_encryption),
 	fsparam_flag	("inlinecrypt",		Opt_inlinecrypt),
 	fsparam_flag	("nombcache",		Opt_nombcache),
@@ -2183,15 +2178,15 @@ 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) &&
+		    (*param->string))
 			return note_qf_name(fc, USRQUOTA, param);
+		return unnote_qf_name(fc, USRQUOTA);
 	case Opt_grpjquota:
-		if (!*param->string)
-			return unnote_qf_name(fc, GRPQUOTA);
-		else
+		if ((param->type == fs_value_is_string) &&
+		    (*param->string))
 			return note_qf_name(fc, GRPQUOTA, param);
+		return unnote_qf_name(fc, GRPQUOTA);
 #endif
 	case Opt_sb:
 		if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {

[PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Luis Henriques]

This patch fixes the usage of mount parameters that are defined as strings
but which can be empty.  Currently, only 'lowerdir' parameter is in this
situation for overlayfs.  But since userspace can pass it in as 'flag'
type (when it doesn't have a value), the parsing will fail because a
'string' type is assumed.

This issue is fixed by using the new helper fsparam_string_or_flag() and by
also taking the parameter type into account.

While there, also remove the now unused fsparam_string_empty() macro.

Suggested-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 fs/overlayfs/params.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 112b4b12f825..6eb163ca4b92 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -139,12 +139,8 @@ static int ovl_verity_mode_def(void)
 	return OVL_VERITY_OFF;
 }
 
-#define fsparam_string_empty(NAME, OPT) \
-	__fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)
-
-
 const struct fs_parameter_spec ovl_parameter_spec[] = {
-	fsparam_string_empty("lowerdir",    Opt_lowerdir),
+	fsparam_string_or_flag("lowerdir",    Opt_lowerdir),
 	fsparam_string("lowerdir+",         Opt_lowerdir_add),
 	fsparam_string("datadir+",          Opt_datadir_add),
 	fsparam_string("upperdir",          Opt_upperdir),
@@ -424,12 +420,13 @@ static void ovl_reset_lowerdirs(struct ovl_fs_context *ctx)
  *     "/data1" and "/data2" as data lower layers. Any existing lower
  *     layers are replaced.
  */
-static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc)
+static int ovl_parse_param_lowerdir(struct fs_context *fc, struct fs_parameter *param)
 {
 	int err;
 	struct ovl_fs_context *ctx = fc->fs_private;
 	struct ovl_fs_context_layer *l;
 	char *dup = NULL, *iter;
+	const char *name = param->string;
 	ssize_t nr_lower, nr;
 	bool data_layer = false;
 
@@ -441,7 +438,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 ((param->type == fs_value_is_flag) || (!*name))
 		return 0;
 
 	if (*name == ':') {
@@ -572,7 +569,7 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
 
 	switch (opt) {
 	case Opt_lowerdir:
-		err = ovl_parse_param_lowerdir(param->string, fc);
+		err = ovl_parse_param_lowerdir(fc, param);
 		break;
 	case Opt_lowerdir_add:
 	case Opt_datadir_add:

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Miklos Szeredi]

On Thu, 7 Mar 2024 at 19:17, Luis Henriques <lhenriques@suse.de> wrote:
>
> This patch fixes the usage of mount parameters that are defined as strings
> but which can be empty.  Currently, only 'lowerdir' parameter is in this
> situation for overlayfs.  But since userspace can pass it in as 'flag'
> type (when it doesn't have a value), the parsing will fail because a
> 'string' type is assumed.

I don't really get why allowing a flag value instead of an empty
string value is fixing anything.

It just makes the API more liberal, but for what gain?

Thanks,
Miklos

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Luis Henriques]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Thu, 7 Mar 2024 at 19:17, Luis Henriques <lhenriques@suse.de> wrote:
>>
>> This patch fixes the usage of mount parameters that are defined as strings
>> but which can be empty.  Currently, only 'lowerdir' parameter is in this
>> situation for overlayfs.  But since userspace can pass it in as 'flag'
>> type (when it doesn't have a value), the parsing will fail because a
>> 'string' type is assumed.
>
> I don't really get why allowing a flag value instead of an empty
> string value is fixing anything.
>
> It just makes the API more liberal, but for what gain?

The point is that userspace may be passing this parameter as a flag and
not as a string.  I came across this issue with ext4, by doing something
as simple as:

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

(actually, the trigger was fstest ext4/053)

The above mount should succeed.  But it fails because 'usrjquota' is set
to a 'flag' type, not 'string'.

Note that I couldn't find a way to reproduce the same issue in overlayfs
with this 'lowerdir' parameter.  But looking at the code the issue is
similar.

Cheers,
-- 
Luís

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Miklos Szeredi]

On Mon, 11 Mar 2024 at 11:34, Luis Henriques <lhenriques@suse.de> wrote:
>
> Miklos Szeredi <miklos@szeredi.hu> writes:
>
> > On Thu, 7 Mar 2024 at 19:17, Luis Henriques <lhenriques@suse.de> wrote:
> >>
> >> This patch fixes the usage of mount parameters that are defined as strings
> >> but which can be empty.  Currently, only 'lowerdir' parameter is in this
> >> situation for overlayfs.  But since userspace can pass it in as 'flag'
> >> type (when it doesn't have a value), the parsing will fail because a
> >> 'string' type is assumed.
> >
> > I don't really get why allowing a flag value instead of an empty
> > string value is fixing anything.
> >
> > It just makes the API more liberal, but for what gain?
>
> The point is that userspace may be passing this parameter as a flag and
> not as a string.  I came across this issue with ext4, by doing something
> as simple as:
>
>     mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
>
> (actually, the trigger was fstest ext4/053)
>
> The above mount should succeed.  But it fails because 'usrjquota' is set
> to a 'flag' type, not 'string'.

The above looks like a misparsing, since the equals sign clearly
indicates that this is not a flag.

> Note that I couldn't find a way to reproduce the same issue in overlayfs
> with this 'lowerdir' parameter.  But looking at the code the issue is
> similar.

In overlayfs the empty lowerdir parameter has a special meaning when
lowerdirs are appended instead of parsed in one go.   As such it won't
be used from /etc/fstab for example, as that would just result in a
failed mount.

I don't see a reason to allow it as a flag for overlayfs, since that
just add ambiguity to the API.

Thanks,
Miklos

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Luis Henriques]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Mon, 11 Mar 2024 at 11:34, Luis Henriques <lhenriques@suse.de> wrote:
>>
>> Miklos Szeredi <miklos@szeredi.hu> writes:
>>
>> > On Thu, 7 Mar 2024 at 19:17, Luis Henriques <lhenriques@suse.de> wrote:
>> >>
>> >> This patch fixes the usage of mount parameters that are defined as strings
>> >> but which can be empty.  Currently, only 'lowerdir' parameter is in this
>> >> situation for overlayfs.  But since userspace can pass it in as 'flag'
>> >> type (when it doesn't have a value), the parsing will fail because a
>> >> 'string' type is assumed.
>> >
>> > I don't really get why allowing a flag value instead of an empty
>> > string value is fixing anything.
>> >
>> > It just makes the API more liberal, but for what gain?
>>
>> The point is that userspace may be passing this parameter as a flag and
>> not as a string.  I came across this issue with ext4, by doing something
>> as simple as:
>>
>>     mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
>>
>> (actually, the trigger was fstest ext4/053)
>>
>> The above mount should succeed.  But it fails because 'usrjquota' is set
>> to a 'flag' type, not 'string'.
>
> The above looks like a misparsing, since the equals sign clearly
> indicates that this is not a flag.

No, not really.  The same thing happens without the '=':

mount -t ext4 -o usrjquota /dev/loop0p1 /mnt/ 
mount: /mnt: wrong fs type, bad option, bad superblock on /dev/loop0p1, missing codepage or helper program, or other error.
       dmesg(1) may have more information after failed mount system call.

The parsing code gets a FSCONFIG_SET_FLAG instead of FSCONFIG_SET_STRING.

>> Note that I couldn't find a way to reproduce the same issue in overlayfs
>> with this 'lowerdir' parameter.  But looking at the code the issue is
>> similar.
>
> In overlayfs the empty lowerdir parameter has a special meaning when
> lowerdirs are appended instead of parsed in one go.   As such it won't
> be used from /etc/fstab for example, as that would just result in a
> failed mount.
>
> I don't see a reason to allow it as a flag for overlayfs, since that
> just add ambiguity to the API.

Fine with me.  But it'd be nice to double-check (by testing) that when
overlayfs gets a 'lowerdir' without a value it really is doing what you'd
expect it to do.

Cheers,
-- 
Luís

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Christian Brauner]

On Mon, Mar 11, 2024 at 11:53:03AM +0100, Miklos Szeredi wrote:
> On Mon, 11 Mar 2024 at 11:34, Luis Henriques <lhenriques@suse.de> wrote:
> >
> > Miklos Szeredi <miklos@szeredi.hu> writes:
> >
> > > On Thu, 7 Mar 2024 at 19:17, Luis Henriques <lhenriques@suse.de> wrote:
> > >>
> > >> This patch fixes the usage of mount parameters that are defined as strings
> > >> but which can be empty.  Currently, only 'lowerdir' parameter is in this
> > >> situation for overlayfs.  But since userspace can pass it in as 'flag'
> > >> type (when it doesn't have a value), the parsing will fail because a
> > >> 'string' type is assumed.
> > >
> > > I don't really get why allowing a flag value instead of an empty
> > > string value is fixing anything.
> > >
> > > It just makes the API more liberal, but for what gain?
> >
> > The point is that userspace may be passing this parameter as a flag and
> > not as a string.  I came across this issue with ext4, by doing something
> > as simple as:
> >
> >     mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
> >
> > (actually, the trigger was fstest ext4/053)
> >
> > The above mount should succeed.  But it fails because 'usrjquota' is set
> > to a 'flag' type, not 'string'.
> 
> The above looks like a misparsing, since the equals sign clearly
> indicates that this is not a flag.

Yeah, so with that I do agree. But have you read my reply to the other
thread? I'd like to hear your thoughs on that. The problem is that
mount(8) currently does:

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

for both -o usrjquota and -o usrjquota=

So we need a clear contract with userspace or the in-kernel solution
proposed here. I see the following options:

(1) Userspace must know that mount options such as "usrjquota" that can
    have no value must be specified as "usrjquota=" when passed to
    mount(8). This in turn means we need to tell Karel to update
    mount(8) to recognize this and infer from "usrjquota=" that it must
    be passed as FSCONFIG_SET_STRING.

(2) We use the proposed in-kernel solution where relevant filesystems
    get the ability to declare this both as a string or as a flag value
    in their parameter parsing code. That's not a VFS generic thing.
    It's a per-fs thing.

(3) We burden mount(8) with knowing what mount options are string
    options that are allowed to be empty. This is clearly the least
    preferable one, imho.

(4) We add a sentinel such as "usrjquota=default" or
    "usrjquota=auto" as a VFS level keyword.

In any case, we need to document what we want:

https://github.com/brauner/man-pages-md/blob/main/fsconfig.md

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Miklos Szeredi]

On Mon, 11 Mar 2024 at 14:25, Christian Brauner <brauner@kernel.org> wrote:

> Yeah, so with that I do agree. But have you read my reply to the other
> thread? I'd like to hear your thoughs on that. The problem is that
> mount(8) currently does:
>
> fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
>
> for both -o usrjquota and -o usrjquota=

For "-o usrjquota" this seems right.

For "-o usrjquota=" it doesn't.  Flags should never have that "=", so
this seems buggy in more than one ways.

> So we need a clear contract with userspace or the in-kernel solution
> proposed here. I see the following options:
>
> (1) Userspace must know that mount options such as "usrjquota" that can
>     have no value must be specified as "usrjquota=" when passed to
>     mount(8). This in turn means we need to tell Karel to update
>     mount(8) to recognize this and infer from "usrjquota=" that it must
>     be passed as FSCONFIG_SET_STRING.

Yes, this is what I'm thinking.  Of course this only works if there
are no backward compatibility issues, if "-o usrjquota" worked in the
past and some systems out there relied on this, then this is not
sufficient.
>
> (2) We use the proposed in-kernel solution where relevant filesystems
>     get the ability to declare this both as a string or as a flag value
>     in their parameter parsing code. That's not a VFS generic thing.
>     It's a per-fs thing.

This encourages inconsistency between filesystems, but if there's no
other way to preserve backward compatibility, then...

>
> (3) We burden mount(8) with knowing what mount options are string
>     options that are allowed to be empty. This is clearly the least
>     preferable one, imho.
>
> (4) We add a sentinel such as "usrjquota=default" or
>     "usrjquota=auto" as a VFS level keyword.

I don't really understand how this last one is supposed to fix the issue.

> In any case, we need to document what we want:
>
> https://github.com/brauner/man-pages-md/blob/main/fsconfig.md

What's the plan with these?  It would be good if "man fsconfig" would
finally work.

Thanks,
Miklos

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Jan Kara]

On Mon 11-03-24 15:39:39, Miklos Szeredi wrote:
> On Mon, 11 Mar 2024 at 14:25, Christian Brauner <brauner@kernel.org> wrote:
> 
> > Yeah, so with that I do agree. But have you read my reply to the other
> > thread? I'd like to hear your thoughs on that. The problem is that
> > mount(8) currently does:
> >
> > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
> >
> > for both -o usrjquota and -o usrjquota=
> 
> For "-o usrjquota" this seems right.
> 
> For "-o usrjquota=" it doesn't.  Flags should never have that "=", so
> this seems buggy in more than one ways.
> 
> > So we need a clear contract with userspace or the in-kernel solution
> > proposed here. I see the following options:
> >
> > (1) Userspace must know that mount options such as "usrjquota" that can
> >     have no value must be specified as "usrjquota=" when passed to
> >     mount(8). This in turn means we need to tell Karel to update
> >     mount(8) to recognize this and infer from "usrjquota=" that it must
> >     be passed as FSCONFIG_SET_STRING.
> 
> Yes, this is what I'm thinking.  Of course this only works if there
> are no backward compatibility issues, if "-o usrjquota" worked in the
> past and some systems out there relied on this, then this is not
> sufficient.

No, "-o usrjquota" never worked and I'm inclined to keep refusing this
variant as IMHO it is confusing.

> > In any case, we need to document what we want:
> >
> > https://github.com/brauner/man-pages-md/blob/main/fsconfig.md
> 
> What's the plan with these?  It would be good if "man fsconfig" would
> finally work.

Yes, merging these into official manpages would be nice.

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

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Christian Brauner]

On Mon, Mar 11, 2024 at 03:39:39PM +0100, Miklos Szeredi wrote:
> On Mon, 11 Mar 2024 at 14:25, Christian Brauner <brauner@kernel.org> wrote:
> 
> > Yeah, so with that I do agree. But have you read my reply to the other
> > thread? I'd like to hear your thoughs on that. The problem is that
> > mount(8) currently does:
> >
> > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
> >
> > for both -o usrjquota and -o usrjquota=
> 
> For "-o usrjquota" this seems right.
> 
> For "-o usrjquota=" it doesn't.  Flags should never have that "=", so
> this seems buggy in more than one ways.
> 
> > So we need a clear contract with userspace or the in-kernel solution
> > proposed here. I see the following options:
> >
> > (1) Userspace must know that mount options such as "usrjquota" that can
> >     have no value must be specified as "usrjquota=" when passed to
> >     mount(8). This in turn means we need to tell Karel to update
> >     mount(8) to recognize this and infer from "usrjquota=" that it must
> >     be passed as FSCONFIG_SET_STRING.
> 
> Yes, this is what I'm thinking.  Of course this only works if there
> are no backward compatibility issues, if "-o usrjquota" worked in the
> past and some systems out there relied on this, then this is not
> sufficient.

Ok, I spoke to Karel and filed:

https://github.com/util-linux/util-linux/issues/2837

So this should get sorted soon.

> > https://github.com/brauner/man-pages-md/blob/main/fsconfig.md
> 
> What's the plan with these?  It would be good if "man fsconfig" would
> finally work.

Yeah, I have this on my todo list but it hasn't been high-prio for me
and it is just so so nice to update the manpages in markdown.

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Christian Brauner]

On Mon, Mar 11, 2024 at 07:01:27PM +0100, Jan Kara wrote:
> On Mon 11-03-24 15:39:39, Miklos Szeredi wrote:
> > On Mon, 11 Mar 2024 at 14:25, Christian Brauner <brauner@kernel.org> wrote:
> > 
> > > Yeah, so with that I do agree. But have you read my reply to the other
> > > thread? I'd like to hear your thoughs on that. The problem is that
> > > mount(8) currently does:
> > >
> > > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
> > >
> > > for both -o usrjquota and -o usrjquota=
> > 
> > For "-o usrjquota" this seems right.
> > 
> > For "-o usrjquota=" it doesn't.  Flags should never have that "=", so
> > this seems buggy in more than one ways.
> > 
> > > So we need a clear contract with userspace or the in-kernel solution
> > > proposed here. I see the following options:
> > >
> > > (1) Userspace must know that mount options such as "usrjquota" that can
> > >     have no value must be specified as "usrjquota=" when passed to
> > >     mount(8). This in turn means we need to tell Karel to update
> > >     mount(8) to recognize this and infer from "usrjquota=" that it must
> > >     be passed as FSCONFIG_SET_STRING.
> > 
> > Yes, this is what I'm thinking.  Of course this only works if there
> > are no backward compatibility issues, if "-o usrjquota" worked in the
> > past and some systems out there relied on this, then this is not
> > sufficient.
> 
> No, "-o usrjquota" never worked and I'm inclined to keep refusing this
> variant as IMHO it is confusing.

Tbh, I'm not too sure that having empty string options was a good idea
even though it can be useful. I think it would've been better if we had
used a specific phantom value to signify this. But yes, I just filed an
issue on util-linux to get this fixed. I think we should also
util-linux and Karel's up for handling this.

> 
> > > In any case, we need to document what we want:
> > >
> > > https://github.com/brauner/man-pages-md/blob/main/fsconfig.md
> > 
> > What's the plan with these?  It would be good if "man fsconfig" would
> > finally work.
> 
> Yes, merging these into official manpages would be nice.

I'll try to get around to it.

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Luis Henriques]

Christian Brauner <brauner@kernel.org> writes:

> On Mon, Mar 11, 2024 at 03:39:39PM +0100, Miklos Szeredi wrote:
>> On Mon, 11 Mar 2024 at 14:25, Christian Brauner <brauner@kernel.org> wrote:
>> 
>> > Yeah, so with that I do agree. But have you read my reply to the other
>> > thread? I'd like to hear your thoughs on that. The problem is that
>> > mount(8) currently does:
>> >
>> > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
>> >
>> > for both -o usrjquota and -o usrjquota=
>> 
>> For "-o usrjquota" this seems right.
>> 
>> For "-o usrjquota=" it doesn't.  Flags should never have that "=", so
>> this seems buggy in more than one ways.
>> 
>> > So we need a clear contract with userspace or the in-kernel solution
>> > proposed here. I see the following options:
>> >
>> > (1) Userspace must know that mount options such as "usrjquota" that can
>> >     have no value must be specified as "usrjquota=" when passed to
>> >     mount(8). This in turn means we need to tell Karel to update
>> >     mount(8) to recognize this and infer from "usrjquota=" that it must
>> >     be passed as FSCONFIG_SET_STRING.
>> 
>> Yes, this is what I'm thinking.  Of course this only works if there
>> are no backward compatibility issues, if "-o usrjquota" worked in the
>> past and some systems out there relied on this, then this is not
>> sufficient.
>
> Ok, I spoke to Karel and filed:
>
> https://github.com/util-linux/util-linux/issues/2837
>
> So this should get sorted soon.

OK, so I if I understand it correctly I can drop all these changes as
there's nothing else to be done from the kernel, right?

(I'll still send out a patch to move the fsparam_string_empty() helper to
a generic header.)

And thanks everyone for your reviews.

Cheers,
-- 
Luís

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Christian Brauner]

On Tue, Mar 12, 2024 at 10:31:08AM +0000, Luis Henriques wrote:
> Christian Brauner <brauner@kernel.org> writes:
> 
> > On Mon, Mar 11, 2024 at 03:39:39PM +0100, Miklos Szeredi wrote:
> >> On Mon, 11 Mar 2024 at 14:25, Christian Brauner <brauner@kernel.org> wrote:
> >> 
> >> > Yeah, so with that I do agree. But have you read my reply to the other
> >> > thread? I'd like to hear your thoughs on that. The problem is that
> >> > mount(8) currently does:
> >> >
> >> > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
> >> >
> >> > for both -o usrjquota and -o usrjquota=
> >> 
> >> For "-o usrjquota" this seems right.
> >> 
> >> For "-o usrjquota=" it doesn't.  Flags should never have that "=", so
> >> this seems buggy in more than one ways.
> >> 
> >> > So we need a clear contract with userspace or the in-kernel solution
> >> > proposed here. I see the following options:
> >> >
> >> > (1) Userspace must know that mount options such as "usrjquota" that can
> >> >     have no value must be specified as "usrjquota=" when passed to
> >> >     mount(8). This in turn means we need to tell Karel to update
> >> >     mount(8) to recognize this and infer from "usrjquota=" that it must
> >> >     be passed as FSCONFIG_SET_STRING.
> >> 
> >> Yes, this is what I'm thinking.  Of course this only works if there
> >> are no backward compatibility issues, if "-o usrjquota" worked in the
> >> past and some systems out there relied on this, then this is not
> >> sufficient.
> >
> > Ok, I spoke to Karel and filed:
> >
> > https://github.com/util-linux/util-linux/issues/2837

This is now merged as of today and backported to at least util-linux
2.40 which is the current release.
https://github.com/util-linux/util-linux/pull/2849

If your distros ship 2.39 and won't upgrade to 2.40 for a while it might
be worth cherry-picking that fix.

Re: [PATCH v2 3/3] ovl: fix the parsing of empty string mount parameters

[Luis Henriques]

Christian Brauner <brauner@kernel.org> writes:

> On Tue, Mar 12, 2024 at 10:31:08AM +0000, Luis Henriques wrote:
>> Christian Brauner <brauner@kernel.org> writes:
>> 
>> > On Mon, Mar 11, 2024 at 03:39:39PM +0100, Miklos Szeredi wrote:
>> >> On Mon, 11 Mar 2024 at 14:25, Christian Brauner <brauner@kernel.org> wrote:
>> >> 
>> >> > Yeah, so with that I do agree. But have you read my reply to the other
>> >> > thread? I'd like to hear your thoughs on that. The problem is that
>> >> > mount(8) currently does:
>> >> >
>> >> > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
>> >> >
>> >> > for both -o usrjquota and -o usrjquota=
>> >> 
>> >> For "-o usrjquota" this seems right.
>> >> 
>> >> For "-o usrjquota=" it doesn't.  Flags should never have that "=", so
>> >> this seems buggy in more than one ways.
>> >> 
>> >> > So we need a clear contract with userspace or the in-kernel solution
>> >> > proposed here. I see the following options:
>> >> >
>> >> > (1) Userspace must know that mount options such as "usrjquota" that can
>> >> >     have no value must be specified as "usrjquota=" when passed to
>> >> >     mount(8). This in turn means we need to tell Karel to update
>> >> >     mount(8) to recognize this and infer from "usrjquota=" that it must
>> >> >     be passed as FSCONFIG_SET_STRING.
>> >> 
>> >> Yes, this is what I'm thinking.  Of course this only works if there
>> >> are no backward compatibility issues, if "-o usrjquota" worked in the
>> >> past and some systems out there relied on this, then this is not
>> >> sufficient.
>> >
>> > Ok, I spoke to Karel and filed:
>> >
>> > https://github.com/util-linux/util-linux/issues/2837
>
> This is now merged as of today and backported to at least util-linux
> 2.40 which is the current release.
> https://github.com/util-linux/util-linux/pull/2849
>
> If your distros ship 2.39 and won't upgrade to 2.40 for a while it might
> be worth cherry-picking that fix.

That's awesome, thanks a lot for pushing this.  I just gave it a try and
it looks good -- ext4/053 isn't failing any more with the next version.

Cheers,
-- 
Luis

Re: [PATCH v2 2/3] ext4: fix the parsing of empty string mount parameters

[kernel test robot]

hi, Luis Henriques,

we noticed in
https://lore.kernel.org/all/20240307160225.23841-1-lhenriques@suse.de/
it was mentioned this patch is for "ext4/053 fstest failure",
however, in our tests, 053 can pass on parent but fail on this commit.


12dbddcebcb8e fs_parser: add helper to define parameters with string and flag types
2de45c422fe6a ext4: fix the parsing of empty string mount parameters

12dbddcebcb8e3e1 2de45c422fe6ae4f64d35df99cd
---------------- ---------------------------
       fail:runs  %reproduction    fail:runs
           |             |             |
           :6          100%           6:6     xfstests.ext4.053.fail

not sure if there is a xfstests patch works with this patch? Thanks!

below report just FYI.


Hello,

kernel test robot noticed "xfstests.ext4.053.fail" on:

commit: 2de45c422fe6ae4f64d35df99cdaf2c6fee2a5ac ("[PATCH v2 2/3] ext4: fix the parsing of empty string mount parameters")
url: https://github.com/intel-lab-lkp/linux/commits/Luis-Henriques/fs_parser-add-helper-to-define-parameters-with-string-and-flag-types/20240308-104759
base: https://git.kernel.org/cgit/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/all/20240307160225.23841-3-lhenriques@suse.de/
patch subject: [PATCH v2 2/3] ext4: fix the parsing of empty string mount parameters

in testcase: xfstests
version: xfstests-x86_64-9b6df9a0-1_20240318
with following parameters:

	disk: 4HDD
	fs: ext4
	test: ext4-053



compiler: gcc-12
test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)




If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202403251019.50cab6c8-oliver.sang@intel.com

2024-03-22 09:38:10 export TEST_DIR=/fs/sdb1
2024-03-22 09:38:10 export TEST_DEV=/dev/sdb1
2024-03-22 09:38:10 export FSTYP=ext4
2024-03-22 09:38:10 export SCRATCH_MNT=/fs/scratch
2024-03-22 09:38:10 mkdir /fs/scratch -p
2024-03-22 09:38:10 export SCRATCH_DEV=/dev/sdb4
2024-03-22 09:38:10 echo ext4/053
2024-03-22 09:38:10 ./check -E tests/exclude/ext4 ext4/053
FSTYP         -- ext4
PLATFORM      -- Linux/x86_64 lkp-skl-d05 6.8.0-rc3-00019-g2de45c422fe6 #1 SMP PREEMPT_DYNAMIC Sat Mar 16 03:20:13 CST 2024
MKFS_OPTIONS  -- -F /dev/sdb4
MOUNT_OPTIONS -- -o acl,user_xattr /dev/sdb4 /fs/scratch

ext4/053       [failed, exit status 1]- output mismatch (see /lkp/benchmarks/xfstests/results//ext4/053.out.bad)
    --- tests/ext4/053.out	2024-03-18 16:30:59.000000000 +0000
    +++ /lkp/benchmarks/xfstests/results//ext4/053.out.bad	2024-03-22 09:44:06.379217460 +0000
    @@ -1,2 +1,3 @@
     QA output created by 053
     Silence is golden.
    +SHOULD FAIL mounting ext4 "test_dummy_encryption=" (mount unexpectedly succeeded) FAILED
    ...
    (Run 'diff -u /lkp/benchmarks/xfstests/tests/ext4/053.out /lkp/benchmarks/xfstests/results//ext4/053.out.bad'  to see the entire diff)
Ran: ext4/053
Failures: ext4/053
Failed 1 of 1 tests




The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240325/202403251019.50cab6c8-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki