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 <[email protected]>
---
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);
Christian Brauner <[email protected]> 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 <[email protected]>
>> ---
>> fs/fs_parser.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> index edb3712dcfa5..53f6cb98a3e0 100644
>> --- a/fs/fs_parser.c
>> +++ b/fs/fs_parser.c
>> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>> /* Try to turn the type we were given into the type desired by the
>> * parameter and give an error if we can't.
>> */
>> - if (is_flag(p)) {
>> + if (is_flag(p) ||
>> + (!param->string && (p->flags & fs_param_can_be_empty))) {
>> if (param->type != fs_value_is_flag)
>> return inval_plog(log, "Unexpected value for '%s'",
>> param->key);
>
> If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> param->string is guaranteed to not be NULL. So really this is only
> about:
>
> FSCONFIG_SET_FD
> FSCONFIG_SET_BINARY
> FSCONFIG_SET_PATH
> FSCONFIG_SET_PATH_EMPTY
>
> and those values being used without a value. What filesystem does this?
> I don't see any.
>
> The tempting thing to do here is to to just remove fs_param_can_be_empty
> from every helper that isn't fs_param_is_string() until we actually have
> a filesystem that wants to use any of the above as flags. Will lose a
> lot of code that isn't currently used.
Right, I find it quite confusing and I may be fixing the issue in the
wrong place. What I'm seeing with ext4 when I mount a filesystem using
the option '-o usrjquota' is that fs_parse() will get:
* p->type is set to fs_param_is_string
('p' is a struct fs_parameter_spec, ->type is a function)
* param->type is set to fs_value_is_flag
('param' is a struct fs_parameter, ->type is an enum)
This is because ext4 will use the __fsparam macro to set define a
fs_param_spec as a fs_param_is_string but will also set the
fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
as a flag. That's why param->string will be NULL in this case.
Cheers,
--
Luís
On Fri, Mar 01, 2024 at 03:45:27PM +0000, Luis Henriques wrote:
> Christian Brauner <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]> 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 <[email protected]>
> > >> ---
> > >> 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 <[email protected]>
Date: Sat, 2 Mar 2024 18:54:35 +0100
Subject: [PATCH] [UNTESTED]
Signed-off-by: Christian Brauner <[email protected]>
---
fs/ext4/super.c | 32 ++++++++++++++++++--------------
include/linux/fs_parser.h | 3 +++
2 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ebd97442d1d4..bd625f06ec0f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1724,10 +1724,6 @@ static const struct constant_table ext4_param_dax[] = {
{}
};
-/* String parameter that allows empty argument */
-#define fsparam_string_empty(NAME, OPT) \
- __fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)
-
/*
* Mount option specification
* We don't use fsparam_flag_no because of the way we set the
@@ -1768,10 +1764,8 @@ static const struct fs_parameter_spec ext4_param_specs[] = {
fsparam_enum ("data", Opt_data, ext4_param_data),
fsparam_enum ("data_err", Opt_data_err,
ext4_param_data_err),
- fsparam_string_empty
- ("usrjquota", Opt_usrjquota),
- fsparam_string_empty
- ("grpjquota", Opt_grpjquota),
+ fsparam_string_or_flag ("usrjquota", Opt_usrjquota),
+ fsparam_string_or_flag ("grpjquota", Opt_grpjquota),
fsparam_enum ("jqfmt", Opt_jqfmt, ext4_param_jqfmt),
fsparam_flag ("grpquota", Opt_grpquota),
fsparam_flag ("quota", Opt_quota),
@@ -2183,15 +2177,25 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
switch (token) {
#ifdef CONFIG_QUOTA
case Opt_usrjquota:
- if (!param->string)
- return unnote_qf_name(fc, USRQUOTA);
- else
+ if (param->type == fs_value_is_string) {
+ if (!*param->string)
+ return unnote_qf_name(fc, USRQUOTA);
+
return note_qf_name(fc, USRQUOTA, param);
+ }
+
+ // param->type == fs_value_is_flag
+ return note_qf_name(fc, USRQUOTA, param);
case Opt_grpjquota:
- if (!param->string)
- return unnote_qf_name(fc, GRPQUOTA);
- else
+ if (param->type == fs_value_is_string) {
+ if (!*param->string)
+ return unnote_qf_name(fc, GRPQUOTA);
+
return note_qf_name(fc, GRPQUOTA, param);
+ }
+
+ // param->type == fs_value_is_flag
+ return note_qf_name(fc, GRPQUOTA, param);
#endif
case Opt_sb:
if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index 01542c4b87a2..4f45141bea95 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -131,5 +131,8 @@ static inline bool fs_validate_description(const char *name,
#define fsparam_bdev(NAME, OPT) __fsparam(fs_param_is_blockdev, NAME, OPT, 0, NULL)
#define fsparam_path(NAME, OPT) __fsparam(fs_param_is_path, NAME, OPT, 0, NULL)
#define fsparam_fd(NAME, OPT) __fsparam(fs_param_is_fd, NAME, OPT, 0, NULL)
+#define fsparam_string_or_flag(NAME, OPT) \
+ __fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL), \
+ fsparam_flag(NAME, OPT)
#endif /* _LINUX_FS_PARSER_H */
--
2.43.0
Christian Brauner <[email protected]> writes:
> On Fri, Mar 01, 2024 at 03:45:27PM +0000, Luis Henriques wrote:
>> Christian Brauner <[email protected]> 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 <[email protected]>
>> >> ---
>> >> fs/fs_parser.c | 3 ++-
>> >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> >> index edb3712dcfa5..53f6cb98a3e0 100644
>> >> --- a/fs/fs_parser.c
>> >> +++ b/fs/fs_parser.c
>> >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>> >> /* Try to turn the type we were given into the type desired by the
>> >> * parameter and give an error if we can't.
>> >> */
>> >> - if (is_flag(p)) {
>> >> + if (is_flag(p) ||
>> >> + (!param->string && (p->flags & fs_param_can_be_empty))) {
>> >> if (param->type != fs_value_is_flag)
>> >> return inval_plog(log, "Unexpected value for '%s'",
>> >> param->key);
>> >
>> > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
>> > param->string is guaranteed to not be NULL. So really this is only
>> > about:
>> >
>> > FSCONFIG_SET_FD
>> > FSCONFIG_SET_BINARY
>> > FSCONFIG_SET_PATH
>> > FSCONFIG_SET_PATH_EMPTY
>> >
>> > and those values being used without a value. What filesystem does this?
>> > I don't see any.
>> >
>> > The tempting thing to do here is to to just remove fs_param_can_be_empty
>> > from every helper that isn't fs_param_is_string() until we actually have
>> > a filesystem that wants to use any of the above as flags. Will lose a
>> > lot of code that isn't currently used.
>>
>> Right, I find it quite confusing and I may be fixing the issue in the
>> wrong place. What I'm seeing with ext4 when I mount a filesystem using
>> the option '-o usrjquota' is that fs_parse() will get:
>>
>> * p->type is set to fs_param_is_string
>> ('p' is a struct fs_parameter_spec, ->type is a function)
>> * param->type is set to fs_value_is_flag
>> ('param' is a struct fs_parameter, ->type is an enum)
>>
>> This is because ext4 will use the __fsparam macro to set define a
>> fs_param_spec as a fs_param_is_string but will also set the
>> fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
>> as a flag. That's why param->string will be NULL in this case.
>
> Thanks for the details. Let me see if I get this right. So you're saying that
> someone is doing:
>
> fsconfig(..., FSCONFIG_SET_FLAG, "usrjquota", NULL, 0); // [1]
>
> ? Is so that is a vital part of the explanation. So please put that in the
> commit message.
Right, I guess I should have added a simple usecase for that in the commit
message. I.e. add a simple 'mount' command with this parameter without
any value.
> Then ext4 defines:
>
> fsparam_string_empty ("usrjquota", Opt_usrjquota),
>
> So [1] gets us:
>
> param->type == fs_value_is_flag
> param->string == NULL
>
> Now we enter into
> fs_parse()
> -> __fs_parse()
> -> fs_lookup_key() for @param and that does:
>
> bool want_flag = param->type == fs_value_is_flag;
>
> *negated = false;
> for (p = desc; p->name; p++) {
> if (strcmp(p->name, name) != 0)
> continue;
> if (likely(is_flag(p) == want_flag))
> return p;
> other = p;
> }
>
> So we don't have a flag parameter defined so the only real match we get is
> @other for:
>
> fsparam_string_empty ("usrjquota", Opt_usrjquota),
>
> What happens now is that you call p->type == fs_param_is_string() and that
> rejects it as bad parameter because param->type == fs_value_is_flag !=
> fs_value_is_string as required. So you dont end up getting Opt_userjquota
> called with param->string NULL, right? So there's not NULL deref or anything,
> right?
>
> You just fail to set usrjquota. Ok, so I think the correct fix is to do
> something like the following in ext4:
>
> fsparam_string_empty ("usrjquota", Opt_usrjquota),
> fs_param_flag ("usrjquota", Opt_usrjquota_flag),
>
> and then in the switch you can do:
>
> switch (opt)
> case Opt_usrjquota:
> // string thing
> case Opt_usrjquota_flag:
> // flag thing
>
> And I really think we should kill all empty handling for non-string types and
> only add that when there's a filesystem that actually needs it.
Yeah, that looks like the right fix. I see you sent out a patch doing
something like this, so I'll comment there instead.
Cheers,
--
Luís
On Sun, Mar 03, 2024 at 09:31:42PM +0000, Luis Henriques wrote:
> Christian Brauner <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> > >> ---
> >> > >> 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!
Christian Brauner <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> > >> ---
>> > >> fs/fs_parser.c | 3 ++-
>> > >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> > >>
>> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> > >> index edb3712dcfa5..53f6cb98a3e0 100644
>> > >> --- a/fs/fs_parser.c
>> > >> +++ b/fs/fs_parser.c
>> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>> > >> /* Try to turn the type we were given into the type desired by the
>> > >> * parameter and give an error if we can't.
>> > >> */
>> > >> - if (is_flag(p)) {
>> > >> + if (is_flag(p) ||
>> > >> + (!param->string && (p->flags & fs_param_can_be_empty))) {
>> > >> if (param->type != fs_value_is_flag)
>> > >> return inval_plog(log, "Unexpected value for '%s'",
>> > >> param->key);
>> > >
>> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
>> > > param->string is guaranteed to not be NULL. So really this is only
>> > > about:
>> > >
>> > > FSCONFIG_SET_FD
>> > > FSCONFIG_SET_BINARY
>> > > FSCONFIG_SET_PATH
>> > > FSCONFIG_SET_PATH_EMPTY
>> > >
>> > > and those values being used without a value. What filesystem does this?
>> > > I don't see any.
>> > >
>> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
>> > > from every helper that isn't fs_param_is_string() until we actually have
>> > > a filesystem that wants to use any of the above as flags. Will lose a
>> > > lot of code that isn't currently used.
>> >
>> > Right, I find it quite confusing and I may be fixing the issue in the
>> > wrong place. What I'm seeing with ext4 when I mount a filesystem using
>> > the option '-o usrjquota' is that fs_parse() will get:
>> >
>> > * p->type is set to fs_param_is_string
>> > ('p' is a struct fs_parameter_spec, ->type is a function)
>> > * param->type is set to fs_value_is_flag
>> > ('param' is a struct fs_parameter, ->type is an enum)
>> >
>> > This is because ext4 will use the __fsparam macro to set define a
>> > fs_param_spec as a fs_param_is_string but will also set the
>> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
>> > as a flag. That's why param->string will be NULL in this case.
>>
>> So I'm a bit confused here. Valid variants of these quota options are like
>> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
>> quota file name). The variant "usrjquota" should ideally be rejected
>> because it doesn't make a good sense and only adds to confusion. Now as far
>> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
>> shows) this is what is happening so what is exactly the problem you're
>> trying to fix?
>
> mount(8) has no way of easily knowing that for something like
> mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
> set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
> indistinguishable from a flag because it's specified without an
> argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
> we should require mount(8) to know what mount options are strings or no.
> I've ran into this issue before myself when using the mount api
> programatically.
Right. A simple usecase is to try to do:
mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
It will fail, and this has been broken for a while.
(And btw: email here is broken again -- I haven't received Jan's email
yet. And this reply will likely take a while to reach its recipients.)
Cheers,
--
Luís
On Fri 08-03-24 10:12:13, Luis Henriques wrote:
> Christian Brauner <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> > >> ---
> >> > >> 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 <[email protected]>
SUSE Labs, CR
Jan Kara <[email protected]> writes:
> On Fri 08-03-24 10:12:13, Luis Henriques wrote:
>> Christian Brauner <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> >> > >> ---
>> >> > >> fs/fs_parser.c | 3 ++-
>> >> > >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >> > >>
>> >> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> >> > >> index edb3712dcfa5..53f6cb98a3e0 100644
>> >> > >> --- a/fs/fs_parser.c
>> >> > >> +++ b/fs/fs_parser.c
>> >> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
>> >> > >> /* Try to turn the type we were given into the type desired by the
>> >> > >> * parameter and give an error if we can't.
>> >> > >> */
>> >> > >> - if (is_flag(p)) {
>> >> > >> + if (is_flag(p) ||
>> >> > >> + (!param->string && (p->flags & fs_param_can_be_empty))) {
>> >> > >> if (param->type != fs_value_is_flag)
>> >> > >> return inval_plog(log, "Unexpected value for '%s'",
>> >> > >> param->key);
>> >> > >
>> >> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
>> >> > > param->string is guaranteed to not be NULL. So really this is only
>> >> > > about:
>> >> > >
>> >> > > FSCONFIG_SET_FD
>> >> > > FSCONFIG_SET_BINARY
>> >> > > FSCONFIG_SET_PATH
>> >> > > FSCONFIG_SET_PATH_EMPTY
>> >> > >
>> >> > > and those values being used without a value. What filesystem does this?
>> >> > > I don't see any.
>> >> > >
>> >> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
>> >> > > from every helper that isn't fs_param_is_string() until we actually have
>> >> > > a filesystem that wants to use any of the above as flags. Will lose a
>> >> > > lot of code that isn't currently used.
>> >> >
>> >> > Right, I find it quite confusing and I may be fixing the issue in the
>> >> > wrong place. What I'm seeing with ext4 when I mount a filesystem using
>> >> > the option '-o usrjquota' is that fs_parse() will get:
>> >> >
>> >> > * p->type is set to fs_param_is_string
>> >> > ('p' is a struct fs_parameter_spec, ->type is a function)
>> >> > * param->type is set to fs_value_is_flag
>> >> > ('param' is a struct fs_parameter, ->type is an enum)
>> >> >
>> >> > This is because ext4 will use the __fsparam macro to set define a
>> >> > fs_param_spec as a fs_param_is_string but will also set the
>> >> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
>> >> > as a flag. That's why param->string will be NULL in this case.
>> >>
>> >> So I'm a bit confused here. Valid variants of these quota options are like
>> >> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
>> >> quota file name). The variant "usrjquota" should ideally be rejected
>> >> because it doesn't make a good sense and only adds to confusion. Now as far
>> >> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
>> >> shows) this is what is happening so what is exactly the problem you're
>> >> trying to fix?
>> >
>> > mount(8) has no way of easily knowing that for something like
>> > mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
>> > set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
>> > indistinguishable from a flag because it's specified without an
>> > argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
>> > we should require mount(8) to know what mount options are strings or no.
>> > I've ran into this issue before myself when using the mount api
>> > programatically.
>>
>> Right. A simple usecase is to try to do:
>>
>> mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
>>
>> It will fail, and this has been broken for a while.
>
> I see. But you have to have new enough mount that is using fsconfig, don't
> you? Because for me in my test VM this works just fine...
Oh, interesting. FTR I'm using mount from util-linux 2.39.3, but I
haven't tried this with older versions.
Cheers,
--
Luís
>
> But anyway, I get the point. Thanks for educating me :)
>
> Honza
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
On Mon 11-03-24 10:26:05, Luis Henriques wrote:
> Jan Kara <[email protected]> writes:
> > On Fri 08-03-24 10:12:13, Luis Henriques wrote:
> >> Christian Brauner <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> >> > >> ---
> >> >> > >> fs/fs_parser.c | 3 ++-
> >> >> > >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >> >> > >>
> >> >> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> >> >> > >> index edb3712dcfa5..53f6cb98a3e0 100644
> >> >> > >> --- a/fs/fs_parser.c
> >> >> > >> +++ b/fs/fs_parser.c
> >> >> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> >> >> > >> /* Try to turn the type we were given into the type desired by the
> >> >> > >> * parameter and give an error if we can't.
> >> >> > >> */
> >> >> > >> - if (is_flag(p)) {
> >> >> > >> + if (is_flag(p) ||
> >> >> > >> + (!param->string && (p->flags & fs_param_can_be_empty))) {
> >> >> > >> if (param->type != fs_value_is_flag)
> >> >> > >> return inval_plog(log, "Unexpected value for '%s'",
> >> >> > >> param->key);
> >> >> > >
> >> >> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> >> >> > > param->string is guaranteed to not be NULL. So really this is only
> >> >> > > about:
> >> >> > >
> >> >> > > FSCONFIG_SET_FD
> >> >> > > FSCONFIG_SET_BINARY
> >> >> > > FSCONFIG_SET_PATH
> >> >> > > FSCONFIG_SET_PATH_EMPTY
> >> >> > >
> >> >> > > and those values being used without a value. What filesystem does this?
> >> >> > > I don't see any.
> >> >> > >
> >> >> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
> >> >> > > from every helper that isn't fs_param_is_string() until we actually have
> >> >> > > a filesystem that wants to use any of the above as flags. Will lose a
> >> >> > > lot of code that isn't currently used.
> >> >> >
> >> >> > Right, I find it quite confusing and I may be fixing the issue in the
> >> >> > wrong place. What I'm seeing with ext4 when I mount a filesystem using
> >> >> > the option '-o usrjquota' is that fs_parse() will get:
> >> >> >
> >> >> > * p->type is set to fs_param_is_string
> >> >> > ('p' is a struct fs_parameter_spec, ->type is a function)
> >> >> > * param->type is set to fs_value_is_flag
> >> >> > ('param' is a struct fs_parameter, ->type is an enum)
> >> >> >
> >> >> > This is because ext4 will use the __fsparam macro to set define a
> >> >> > fs_param_spec as a fs_param_is_string but will also set the
> >> >> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> >> >> > as a flag. That's why param->string will be NULL in this case.
> >> >>
> >> >> So I'm a bit confused here. Valid variants of these quota options are like
> >> >> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
> >> >> quota file name). The variant "usrjquota" should ideally be rejected
> >> >> because it doesn't make a good sense and only adds to confusion. Now as far
> >> >> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
> >> >> shows) this is what is happening so what is exactly the problem you're
> >> >> trying to fix?
> >> >
> >> > mount(8) has no way of easily knowing that for something like
> >> > mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
> >> > set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
> >> > indistinguishable from a flag because it's specified without an
> >> > argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
> >> > we should require mount(8) to know what mount options are strings or no.
> >> > I've ran into this issue before myself when using the mount api
> >> > programatically.
> >>
> >> Right. A simple usecase is to try to do:
> >>
> >> mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
> >>
> >> It will fail, and this has been broken for a while.
> >
> > I see. But you have to have new enough mount that is using fsconfig, don't
> > you? Because for me in my test VM this works just fine...
>
> Oh, interesting. FTR I'm using mount from util-linux 2.39.3, but I
> haven't tried this with older versions.
I'm using util-linux 2.37.2 and checking the changelogs indeed 2.39 started
to use the new mount API from the kernel. Checking strace of the new mount
I can indeed see mount(8) does:
fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
So it is actually util-linux, not the kernel parser, that IMHO incorrectly
parses the mount options and uses FSCONFIG_SET_FLAG instead of
FSCONFIG_SET_STRING with an empty string.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR