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