This patch fixes the usage of mount parameters that are defined as strings
but which can be empty. Currently, only 'lowerdir' parameter is in this
situation for overlayfs. But since userspace can pass it in as 'flag'
type (when it doesn't have a value), the parsing will fail because a
'string' type is assumed.
This issue is fixed by using the new helper fsparam_string_or_flag() and by
also taking the parameter type into account.
While there, also remove the now unused fsparam_string_empty() macro.
Suggested-by: Christian Brauner <[email protected]>
Signed-off-by: Luis Henriques <[email protected]>
---
fs/overlayfs/params.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 112b4b12f825..6eb163ca4b92 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -139,12 +139,8 @@ static int ovl_verity_mode_def(void)
return OVL_VERITY_OFF;
}
-#define fsparam_string_empty(NAME, OPT) \
- __fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)
-
-
const struct fs_parameter_spec ovl_parameter_spec[] = {
- fsparam_string_empty("lowerdir", Opt_lowerdir),
+ fsparam_string_or_flag("lowerdir", Opt_lowerdir),
fsparam_string("lowerdir+", Opt_lowerdir_add),
fsparam_string("datadir+", Opt_datadir_add),
fsparam_string("upperdir", Opt_upperdir),
@@ -424,12 +420,13 @@ static void ovl_reset_lowerdirs(struct ovl_fs_context *ctx)
* "/data1" and "/data2" as data lower layers. Any existing lower
* layers are replaced.
*/
-static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc)
+static int ovl_parse_param_lowerdir(struct fs_context *fc, struct fs_parameter *param)
{
int err;
struct ovl_fs_context *ctx = fc->fs_private;
struct ovl_fs_context_layer *l;
char *dup = NULL, *iter;
+ const char *name = param->string;
ssize_t nr_lower, nr;
bool data_layer = false;
@@ -441,7 +438,7 @@ static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc)
/* drop all existing lower layers */
ovl_reset_lowerdirs(ctx);
- if (!*name)
+ if ((param->type == fs_value_is_flag) || (!*name))
return 0;
if (*name == ':') {
@@ -572,7 +569,7 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
switch (opt) {
case Opt_lowerdir:
- err = ovl_parse_param_lowerdir(param->string, fc);
+ err = ovl_parse_param_lowerdir(fc, param);
break;
case Opt_lowerdir_add:
case Opt_datadir_add:
On Thu, 7 Mar 2024 at 19:17, Luis Henriques <[email protected]> wrote:
>
> This patch fixes the usage of mount parameters that are defined as strings
> but which can be empty. Currently, only 'lowerdir' parameter is in this
> situation for overlayfs. But since userspace can pass it in as 'flag'
> type (when it doesn't have a value), the parsing will fail because a
> 'string' type is assumed.
I don't really get why allowing a flag value instead of an empty
string value is fixing anything.
It just makes the API more liberal, but for what gain?
Thanks,
Miklos
On Mon, 11 Mar 2024 at 11:34, Luis Henriques <[email protected]> wrote:
>
> Miklos Szeredi <[email protected]> writes:
>
> > On Thu, 7 Mar 2024 at 19:17, Luis Henriques <[email protected]> wrote:
> >>
> >> This patch fixes the usage of mount parameters that are defined as strings
> >> but which can be empty. Currently, only 'lowerdir' parameter is in this
> >> situation for overlayfs. But since userspace can pass it in as 'flag'
> >> type (when it doesn't have a value), the parsing will fail because a
> >> 'string' type is assumed.
> >
> > I don't really get why allowing a flag value instead of an empty
> > string value is fixing anything.
> >
> > It just makes the API more liberal, but for what gain?
>
> The point is that userspace may be passing this parameter as a flag and
> not as a string. I came across this issue with ext4, by doing something
> as simple as:
>
> mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
>
> (actually, the trigger was fstest ext4/053)
>
> The above mount should succeed. But it fails because 'usrjquota' is set
> to a 'flag' type, not 'string'.
The above looks like a misparsing, since the equals sign clearly
indicates that this is not a flag.
> Note that I couldn't find a way to reproduce the same issue in overlayfs
> with this 'lowerdir' parameter. But looking at the code the issue is
> similar.
In overlayfs the empty lowerdir parameter has a special meaning when
lowerdirs are appended instead of parsed in one go. As such it won't
be used from /etc/fstab for example, as that would just result in a
failed mount.
I don't see a reason to allow it as a flag for overlayfs, since that
just add ambiguity to the API.
Thanks,
Miklos
Miklos Szeredi <[email protected]> writes:
> On Mon, 11 Mar 2024 at 11:34, Luis Henriques <[email protected]> wrote:
>>
>> Miklos Szeredi <[email protected]> writes:
>>
>> > On Thu, 7 Mar 2024 at 19:17, Luis Henriques <[email protected]> wrote:
>> >>
>> >> This patch fixes the usage of mount parameters that are defined as strings
>> >> but which can be empty. Currently, only 'lowerdir' parameter is in this
>> >> situation for overlayfs. But since userspace can pass it in as 'flag'
>> >> type (when it doesn't have a value), the parsing will fail because a
>> >> 'string' type is assumed.
>> >
>> > I don't really get why allowing a flag value instead of an empty
>> > string value is fixing anything.
>> >
>> > It just makes the API more liberal, but for what gain?
>>
>> The point is that userspace may be passing this parameter as a flag and
>> not as a string. I came across this issue with ext4, by doing something
>> as simple as:
>>
>> mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
>>
>> (actually, the trigger was fstest ext4/053)
>>
>> The above mount should succeed. But it fails because 'usrjquota' is set
>> to a 'flag' type, not 'string'.
>
> The above looks like a misparsing, since the equals sign clearly
> indicates that this is not a flag.
No, not really. The same thing happens without the '=':
mount -t ext4 -o usrjquota /dev/loop0p1 /mnt/
mount: /mnt: wrong fs type, bad option, bad superblock on /dev/loop0p1, missing codepage or helper program, or other error.
dmesg(1) may have more information after failed mount system call.
The parsing code gets a FSCONFIG_SET_FLAG instead of FSCONFIG_SET_STRING.
>> Note that I couldn't find a way to reproduce the same issue in overlayfs
>> with this 'lowerdir' parameter. But looking at the code the issue is
>> similar.
>
> In overlayfs the empty lowerdir parameter has a special meaning when
> lowerdirs are appended instead of parsed in one go. As such it won't
> be used from /etc/fstab for example, as that would just result in a
> failed mount.
>
> I don't see a reason to allow it as a flag for overlayfs, since that
> just add ambiguity to the API.
Fine with me. But it'd be nice to double-check (by testing) that when
overlayfs gets a 'lowerdir' without a value it really is doing what you'd
expect it to do.
Cheers,
--
Luís
On Mon, Mar 11, 2024 at 11:53:03AM +0100, Miklos Szeredi wrote:
> On Mon, 11 Mar 2024 at 11:34, Luis Henriques <[email protected]> wrote:
> >
> > Miklos Szeredi <[email protected]> writes:
> >
> > > On Thu, 7 Mar 2024 at 19:17, Luis Henriques <[email protected]> wrote:
> > >>
> > >> This patch fixes the usage of mount parameters that are defined as strings
> > >> but which can be empty. Currently, only 'lowerdir' parameter is in this
> > >> situation for overlayfs. But since userspace can pass it in as 'flag'
> > >> type (when it doesn't have a value), the parsing will fail because a
> > >> 'string' type is assumed.
> > >
> > > I don't really get why allowing a flag value instead of an empty
> > > string value is fixing anything.
> > >
> > > It just makes the API more liberal, but for what gain?
> >
> > The point is that userspace may be passing this parameter as a flag and
> > not as a string. I came across this issue with ext4, by doing something
> > as simple as:
> >
> > mount -t ext4 -o usrjquota= /dev/sda1 /mnt/
> >
> > (actually, the trigger was fstest ext4/053)
> >
> > The above mount should succeed. But it fails because 'usrjquota' is set
> > to a 'flag' type, not 'string'.
>
> The above looks like a misparsing, since the equals sign clearly
> indicates that this is not a flag.
Yeah, so with that I do agree. But have you read my reply to the other
thread? I'd like to hear your thoughs on that. The problem is that
mount(8) currently does:
fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
for both -o usrjquota and -o usrjquota=
So we need a clear contract with userspace or the in-kernel solution
proposed here. I see the following options:
(1) Userspace must know that mount options such as "usrjquota" that can
have no value must be specified as "usrjquota=" when passed to
mount(8). This in turn means we need to tell Karel to update
mount(8) to recognize this and infer from "usrjquota=" that it must
be passed as FSCONFIG_SET_STRING.
(2) We use the proposed in-kernel solution where relevant filesystems
get the ability to declare this both as a string or as a flag value
in their parameter parsing code. That's not a VFS generic thing.
It's a per-fs thing.
(3) We burden mount(8) with knowing what mount options are string
options that are allowed to be empty. This is clearly the least
preferable one, imho.
(4) We add a sentinel such as "usrjquota=default" or
"usrjquota=auto" as a VFS level keyword.
In any case, we need to document what we want:
https://github.com/brauner/man-pages-md/blob/main/fsconfig.md
On Mon, 11 Mar 2024 at 14:25, Christian Brauner <[email protected]> wrote:
> Yeah, so with that I do agree. But have you read my reply to the other
> thread? I'd like to hear your thoughs on that. The problem is that
> mount(8) currently does:
>
> fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
>
> for both -o usrjquota and -o usrjquota=
For "-o usrjquota" this seems right.
For "-o usrjquota=" it doesn't. Flags should never have that "=", so
this seems buggy in more than one ways.
> So we need a clear contract with userspace or the in-kernel solution
> proposed here. I see the following options:
>
> (1) Userspace must know that mount options such as "usrjquota" that can
> have no value must be specified as "usrjquota=" when passed to
> mount(8). This in turn means we need to tell Karel to update
> mount(8) to recognize this and infer from "usrjquota=" that it must
> be passed as FSCONFIG_SET_STRING.
Yes, this is what I'm thinking. Of course this only works if there
are no backward compatibility issues, if "-o usrjquota" worked in the
past and some systems out there relied on this, then this is not
sufficient.
>
> (2) We use the proposed in-kernel solution where relevant filesystems
> get the ability to declare this both as a string or as a flag value
> in their parameter parsing code. That's not a VFS generic thing.
> It's a per-fs thing.
This encourages inconsistency between filesystems, but if there's no
other way to preserve backward compatibility, then...
>
> (3) We burden mount(8) with knowing what mount options are string
> options that are allowed to be empty. This is clearly the least
> preferable one, imho.
>
> (4) We add a sentinel such as "usrjquota=default" or
> "usrjquota=auto" as a VFS level keyword.
I don't really understand how this last one is supposed to fix the issue.
> In any case, we need to document what we want:
>
> https://github.com/brauner/man-pages-md/blob/main/fsconfig.md
What's the plan with these? It would be good if "man fsconfig" would
finally work.
Thanks,
Miklos
On Mon, Mar 11, 2024 at 03:39:39PM +0100, Miklos Szeredi wrote:
> On Mon, 11 Mar 2024 at 14:25, Christian Brauner <[email protected]> wrote:
>
> > Yeah, so with that I do agree. But have you read my reply to the other
> > thread? I'd like to hear your thoughs on that. The problem is that
> > mount(8) currently does:
> >
> > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
> >
> > for both -o usrjquota and -o usrjquota=
>
> For "-o usrjquota" this seems right.
>
> For "-o usrjquota=" it doesn't. Flags should never have that "=", so
> this seems buggy in more than one ways.
>
> > So we need a clear contract with userspace or the in-kernel solution
> > proposed here. I see the following options:
> >
> > (1) Userspace must know that mount options such as "usrjquota" that can
> > have no value must be specified as "usrjquota=" when passed to
> > mount(8). This in turn means we need to tell Karel to update
> > mount(8) to recognize this and infer from "usrjquota=" that it must
> > be passed as FSCONFIG_SET_STRING.
>
> Yes, this is what I'm thinking. Of course this only works if there
> are no backward compatibility issues, if "-o usrjquota" worked in the
> past and some systems out there relied on this, then this is not
> sufficient.
Ok, I spoke to Karel and filed:
https://github.com/util-linux/util-linux/issues/2837
So this should get sorted soon.
> > https://github.com/brauner/man-pages-md/blob/main/fsconfig.md
>
> What's the plan with these? It would be good if "man fsconfig" would
> finally work.
Yeah, I have this on my todo list but it hasn't been high-prio for me
and it is just so so nice to update the manpages in markdown.
On Mon, Mar 11, 2024 at 07:01:27PM +0100, Jan Kara wrote:
> On Mon 11-03-24 15:39:39, Miklos Szeredi wrote:
> > On Mon, 11 Mar 2024 at 14:25, Christian Brauner <[email protected]> wrote:
> >
> > > Yeah, so with that I do agree. But have you read my reply to the other
> > > thread? I'd like to hear your thoughs on that. The problem is that
> > > mount(8) currently does:
> > >
> > > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
> > >
> > > for both -o usrjquota and -o usrjquota=
> >
> > For "-o usrjquota" this seems right.
> >
> > For "-o usrjquota=" it doesn't. Flags should never have that "=", so
> > this seems buggy in more than one ways.
> >
> > > So we need a clear contract with userspace or the in-kernel solution
> > > proposed here. I see the following options:
> > >
> > > (1) Userspace must know that mount options such as "usrjquota" that can
> > > have no value must be specified as "usrjquota=" when passed to
> > > mount(8). This in turn means we need to tell Karel to update
> > > mount(8) to recognize this and infer from "usrjquota=" that it must
> > > be passed as FSCONFIG_SET_STRING.
> >
> > Yes, this is what I'm thinking. Of course this only works if there
> > are no backward compatibility issues, if "-o usrjquota" worked in the
> > past and some systems out there relied on this, then this is not
> > sufficient.
>
> No, "-o usrjquota" never worked and I'm inclined to keep refusing this
> variant as IMHO it is confusing.
Tbh, I'm not too sure that having empty string options was a good idea
even though it can be useful. I think it would've been better if we had
used a specific phantom value to signify this. But yes, I just filed an
issue on util-linux to get this fixed. I think we should also
util-linux and Karel's up for handling this.
>
> > > In any case, we need to document what we want:
> > >
> > > https://github.com/brauner/man-pages-md/blob/main/fsconfig.md
> >
> > What's the plan with these? It would be good if "man fsconfig" would
> > finally work.
>
> Yes, merging these into official manpages would be nice.
I'll try to get around to it.
Christian Brauner <[email protected]> writes:
> On Mon, Mar 11, 2024 at 03:39:39PM +0100, Miklos Szeredi wrote:
>> On Mon, 11 Mar 2024 at 14:25, Christian Brauner <[email protected]> wrote:
>>
>> > Yeah, so with that I do agree. But have you read my reply to the other
>> > thread? I'd like to hear your thoughs on that. The problem is that
>> > mount(8) currently does:
>> >
>> > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
>> >
>> > for both -o usrjquota and -o usrjquota=
>>
>> For "-o usrjquota" this seems right.
>>
>> For "-o usrjquota=" it doesn't. Flags should never have that "=", so
>> this seems buggy in more than one ways.
>>
>> > So we need a clear contract with userspace or the in-kernel solution
>> > proposed here. I see the following options:
>> >
>> > (1) Userspace must know that mount options such as "usrjquota" that can
>> > have no value must be specified as "usrjquota=" when passed to
>> > mount(8). This in turn means we need to tell Karel to update
>> > mount(8) to recognize this and infer from "usrjquota=" that it must
>> > be passed as FSCONFIG_SET_STRING.
>>
>> Yes, this is what I'm thinking. Of course this only works if there
>> are no backward compatibility issues, if "-o usrjquota" worked in the
>> past and some systems out there relied on this, then this is not
>> sufficient.
>
> Ok, I spoke to Karel and filed:
>
> https://github.com/util-linux/util-linux/issues/2837
>
> So this should get sorted soon.
OK, so I if I understand it correctly I can drop all these changes as
there's nothing else to be done from the kernel, right?
(I'll still send out a patch to move the fsparam_string_empty() helper to
a generic header.)
And thanks everyone for your reviews.
Cheers,
--
Luís
Christian Brauner <[email protected]> writes:
> On Tue, Mar 12, 2024 at 10:31:08AM +0000, Luis Henriques wrote:
>> Christian Brauner <[email protected]> writes:
>>
>> > On Mon, Mar 11, 2024 at 03:39:39PM +0100, Miklos Szeredi wrote:
>> >> On Mon, 11 Mar 2024 at 14:25, Christian Brauner <[email protected]> wrote:
>> >>
>> >> > Yeah, so with that I do agree. But have you read my reply to the other
>> >> > thread? I'd like to hear your thoughs on that. The problem is that
>> >> > mount(8) currently does:
>> >> >
>> >> > fsconfig(3, FSCONFIG_SET_FLAG, "usrjquota", NULL, 0) = -1 EINVAL (Invalid argument)
>> >> >
>> >> > for both -o usrjquota and -o usrjquota=
>> >>
>> >> For "-o usrjquota" this seems right.
>> >>
>> >> For "-o usrjquota=" it doesn't. Flags should never have that "=", so
>> >> this seems buggy in more than one ways.
>> >>
>> >> > So we need a clear contract with userspace or the in-kernel solution
>> >> > proposed here. I see the following options:
>> >> >
>> >> > (1) Userspace must know that mount options such as "usrjquota" that can
>> >> > have no value must be specified as "usrjquota=" when passed to
>> >> > mount(8). This in turn means we need to tell Karel to update
>> >> > mount(8) to recognize this and infer from "usrjquota=" that it must
>> >> > be passed as FSCONFIG_SET_STRING.
>> >>
>> >> Yes, this is what I'm thinking. Of course this only works if there
>> >> are no backward compatibility issues, if "-o usrjquota" worked in the
>> >> past and some systems out there relied on this, then this is not
>> >> sufficient.
>> >
>> > Ok, I spoke to Karel and filed:
>> >
>> > https://github.com/util-linux/util-linux/issues/2837
>
> This is now merged as of today and backported to at least util-linux
> 2.40 which is the current release.
> https://github.com/util-linux/util-linux/pull/2849
>
> If your distros ship 2.39 and won't upgrade to 2.40 for a while it might
> be worth cherry-picking that fix.
That's awesome, thanks a lot for pushing this. I just gave it a try and
it looks good -- ext4/053 isn't failing any more with the next version.
Cheers,
--
Luis