| Message ID | 20240229163011.16248-2-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs_parser: handle parameters that can be empty and don't have a value | expand |
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.
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,
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.
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 */
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,
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,
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!
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
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.
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,
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.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,
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
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);
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(-)