| Message ID | 20191212145042.12694-1-labbott@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs: Don't reject unknown parameters | expand |
On Thu, Dec 12, 2019 at 3:51 PM Laura Abbott <labbott@redhat.com> wrote: > > The new mount API currently rejects unknown parameters if the > filesystem doesn't have doesn't take any arguments. This is > unfortunately a regression from the old API which silently > ignores extra arguments. This is easly seen with the squashfs > conversion (5a2be1288b51 ("vfs: Convert squashfs to use the new > mount API")) which now fails to mount with extra options. Just > get rid of the error. > > Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock > creation/configuration context") > Link: https://lore.kernel.org/lkml/20191130181548.GA28459@gentoo-tp.home/ > Reported-by: Jeremi Piotrowski <jeremi.piotrowski@gmail.com> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1781863 > Signed-off-by: Laura Abbott <labbott@redhat.com> > --- > fs/fs_context.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/fs_context.c b/fs/fs_context.c > index 138b5b4d621d..7ec20b1f8a53 100644 > --- a/fs/fs_context.c > +++ b/fs/fs_context.c > @@ -160,8 +160,7 @@ int vfs_parse_fs_param(struct fs_context *fc, struct fs_parameter *param) > return 0; > } > > - return invalf(fc, "%s: Unknown parameter '%s'", > - fc->fs_type->name, param->key); > + return 0; > } > EXPORT_SYMBOL(vfs_parse_fs_param); Hi Laura, I'm pretty sure this is a regression for all other filesystems that used to check for unknown tokens and return an error from their ->mount/fill_super/etc methods before the conversion. All filesystems that provide ->parse_param expect that ENOPARAM is turned into an error with an error message. I think we are going to need something a bit more involved in vfs_parse_fs_param(), or just a dummy ->parse_param for squashfs that would always return 0. Thanks, Ilya
On 12/12/19 12:13 PM, Ilya Dryomov wrote: > On Thu, Dec 12, 2019 at 3:51 PM Laura Abbott <labbott@redhat.com> wrote: >> >> The new mount API currently rejects unknown parameters if the >> filesystem doesn't have doesn't take any arguments. This is >> unfortunately a regression from the old API which silently >> ignores extra arguments. This is easly seen with the squashfs >> conversion (5a2be1288b51 ("vfs: Convert squashfs to use the new >> mount API")) which now fails to mount with extra options. Just >> get rid of the error. >> >> Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock >> creation/configuration context") >> Link: https://lore.kernel.org/lkml/20191130181548.GA28459@gentoo-tp.home/ >> Reported-by: Jeremi Piotrowski <jeremi.piotrowski@gmail.com> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1781863 >> Signed-off-by: Laura Abbott <labbott@redhat.com> >> --- >> fs/fs_context.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/fs/fs_context.c b/fs/fs_context.c >> index 138b5b4d621d..7ec20b1f8a53 100644 >> --- a/fs/fs_context.c >> +++ b/fs/fs_context.c >> @@ -160,8 +160,7 @@ int vfs_parse_fs_param(struct fs_context *fc, struct fs_parameter *param) >> return 0; >> } >> >> - return invalf(fc, "%s: Unknown parameter '%s'", >> - fc->fs_type->name, param->key); >> + return 0; >> } >> EXPORT_SYMBOL(vfs_parse_fs_param); > > Hi Laura, > > I'm pretty sure this is a regression for all other filesystems > that used to check for unknown tokens and return an error from their > ->mount/fill_super/etc methods before the conversion. > > All filesystems that provide ->parse_param expect that ENOPARAM is > turned into an error with an error message. I think we are going to > need something a bit more involved in vfs_parse_fs_param(), or just > a dummy ->parse_param for squashfs that would always return 0. > > Thanks, > > Ilya > Good point, I think I missed how that code flow worked for printing out the error. I debated putting in a dummy parse_param but I figured that squashfs wouldn't be the only fs that didn't take arguments (it's in the minority but certainly not the only one). I'll see about reworking the flow unless Al/David want to see the dummy parse_param or give a patch. Thanks, Laura
On Thu, Dec 12, 2019 at 9:47 AM Laura Abbott <labbott@redhat.com> wrote: > > Good point, I think I missed how that code flow worked for printing > out the error. I debated putting in a dummy parse_param but I > figured that squashfs wouldn't be the only fs that didn't take > arguments (it's in the minority but certainly not the only one). I think printing out the error part is actually fine - it would act as a warning for invalid parameters like this. So I think a dummy parse_param that prints out a warning is likely the right thing to do. Something like the attached, perhaps? Totally untested. Linus
On 12/12/19 12:56 PM, Linus Torvalds wrote: > On Thu, Dec 12, 2019 at 9:47 AM Laura Abbott <labbott@redhat.com> wrote: >> >> Good point, I think I missed how that code flow worked for printing >> out the error. I debated putting in a dummy parse_param but I >> figured that squashfs wouldn't be the only fs that didn't take >> arguments (it's in the minority but certainly not the only one). > > I think printing out the error part is actually fine - it would act as > a warning for invalid parameters like this. > > So I think a dummy parse_param that prints out a warning is likely the > right thing to do. > > Something like the attached, perhaps? Totally untested. > > Linus > That doesn't quite work. We can't just unconditionally return success because we rely on -ENOPARAM being returned to parse the source option back in vfs_parse_fs_param. I think ramfs may also be broken for the same reason right now as well from reading the code. We also rely on the fallback source parsing for file systems that do have ->parse_param. We could do all this in squashfs but given other file systems that don't have args will also hit this we could just make it generic. The following works for me (under commenting and poor name choices notwithstanding) diff --git a/fs/fs_parser.c b/fs/fs_parser.c index d1930adce68d..5e45e36d51e7 100644 --- a/fs/fs_parser.c +++ b/fs/fs_parser.c @@ -302,6 +302,50 @@ int fs_lookup_param(struct fs_context *fc, } EXPORT_SYMBOL(fs_lookup_param); +enum { + NO_OPT_SOURCE, +}; + +static const struct fs_parameter_spec no_opt_fs_param_specs[] = { + fsparam_string ("source", NO_OPT_SOURCE), + {} +}; + +const struct fs_parameter_description no_opt_fs_parameters = { + .name = "no_opt_fs", + .specs = no_opt_fs_param_specs, +}; + +int fs_no_opt_parse_param(struct fs_context *fc, struct fs_parameter *param) +{ + struct fs_parse_result result; + int opt; + + opt = fs_parse(fc, &no_opt_fs_parameters, param, &result); + if (opt < 0) { + /* Just log an error for backwards compatibility */ + errorf(fc, "%s: Unknown parameter '%s'", + fc->fs_type->name, param->key); + return 0; + } + + switch (opt) { + case NO_OPT_SOURCE: + if (param->type != fs_value_is_string) + return invalf(fc, "%s: Non-string source", + fc->fs_type->name); + if (fc->source) + return invalf(fc, "%s: Multiple sources specified", + fc->fs_type->name); + fc->source = param->string; + param->string = NULL; + break; + } + + return 0; +} +EXPORT_SYMBOL(fs_no_opt_parse_param); + #ifdef CONFIG_VALIDATE_FS_PARSER /** * validate_constant_table - Validate a constant table diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c index 0cc4ceec0562..07a9b38f7bf5 100644 --- a/fs/squashfs/super.c +++ b/fs/squashfs/super.c @@ -18,6 +18,7 @@ #include <linux/fs.h> #include <linux/fs_context.h> +#include <linux/fs_parser.h> #include <linux/vfs.h> #include <linux/slab.h> #include <linux/mutex.h> @@ -358,6 +359,7 @@ static int squashfs_reconfigure(struct fs_context *fc) static const struct fs_context_operations squashfs_context_ops = { .get_tree = squashfs_get_tree, .reconfigure = squashfs_reconfigure, + .parse_param = fs_no_opt_parse_param, }; static int squashfs_init_fs_context(struct fs_context *fc) diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h index dee140db6240..f67b2afcc491 100644 --- a/include/linux/fs_parser.h +++ b/include/linux/fs_parser.h @@ -106,6 +106,8 @@ static inline bool fs_validate_description(const struct fs_parameter_description { return true; } #endif +extern int fs_no_opt_parse_param(struct fs_context *fc, struct fs_parameter *param); + /* * Parameter type, name, index and flags element constructors. Use as: *
On Thu, Dec 12, 2019 at 03:01:56PM -0500, Laura Abbott wrote: > +static const struct fs_parameter_spec no_opt_fs_param_specs[] = { > + fsparam_string ("source", NO_OPT_SOURCE), > + {} > +}; > + > +const struct fs_parameter_description no_opt_fs_parameters = { > + .name = "no_opt_fs", > + .specs = no_opt_fs_param_specs, > +}; > + > +int fs_no_opt_parse_param(struct fs_context *fc, struct fs_parameter *param) > +{ > + struct fs_parse_result result; > + int opt; > + > + opt = fs_parse(fc, &no_opt_fs_parameters, param, &result); > + if (opt < 0) { > + /* Just log an error for backwards compatibility */ > + errorf(fc, "%s: Unknown parameter '%s'", > + fc->fs_type->name, param->key); > + return 0; > + } > + > + switch (opt) { > + case NO_OPT_SOURCE: > + if (param->type != fs_value_is_string) > + return invalf(fc, "%s: Non-string source", > + fc->fs_type->name); > + if (fc->source) > + return invalf(fc, "%s: Multiple sources specified", > + fc->fs_type->name); > + fc->source = param->string; > + param->string = NULL; > + break; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(fs_no_opt_parse_param); Yecchhhh... Could you explain why do you want to bother with fs_parse() here? Seriously, look at it. { const struct fs_parameter_spec *p; const struct fs_parameter_enum *e; int ret = -ENOPARAM, b; result->has_value = !!param->string; result->negated = false; result->uint_64 = 0; p = fs_lookup_key(desc, param->key); OK, that's if (strcmp(param->key, "source") == 0) p = no_opt_fs_param_specs; else p = NULL; if (!p) { not "source" /* If we didn't find something that looks like "noxxx", see if * "xxx" takes the "no"-form negative - but only if there * wasn't an value. */ if (result->has_value) goto unknown_parameter; if param->string is non-NULL - piss off if (param->key[0] != 'n' || param->key[1] != 'o' || !param->key[2]) goto unknown_parameter; if not "no"<something> - ditto p = fs_lookup_key(desc, param->key + 2); if (!p) goto unknown_parameter; if not "nosource" - ditto if (!(p->flags & fs_param_neg_with_no)) goto unknown_parameter; ... and since ->flags doesn't have that bit, the same for "nosource" anyway. result->boolean = false; result->negated = true; won't get here } OK, so the above is simply 'piss off unless param->key is "source"'. And p is no_opt_fs_param_specs. if (p->flags & fs_param_deprecated) nope warnf(fc, "%s: Deprecated parameter '%s'", desc->name, param->key); if (result->negated) goto okay; nope - set to false, never changed /* Certain parameter types only take a string and convert it. */ switch (p->type) { that'd be fs_param_is_string ... case fs_param_is_string: if (param->type != fs_value_is_string) goto bad_value; if (!result->has_value) { if param->string is NULL... if (p->flags & fs_param_v_optional) nope goto okay; goto bad_value; } /* Fall through */ default: break; } /* Try to turn the type we were given into the type desired by the * parameter and give an error if we can't. */ switch (p->type) { again, fs_param_is_string ... case fs_param_is_string: goto okay; ... okay: return p->opt; bad_value: return invalf(fc, "%s: Bad value for '%s'", desc->name, param->key); unknown_parameter: return -ENOPARAM; In other words, that thing is equivalent to if (strcmp(param->key, "source")) { errorf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key); return 0; } if (param->type != fs_value_is_string || !param->value) { invalf(fc, "%s: Bad value for '%s'", fc->fs_type->name, param->key); errorf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key); return 0; // almost certainly not what you intended for that case } if (fc->source) return invalf(fc, "%s: Multiple sources specified", fc->fs_type->name); fc->source = param->string; param->string = NULL; return 0; And that - without the boilerplate from hell. But if you look at the caller of that method, you'll see this: if (fc->ops->parse_param) { ret = fc->ops->parse_param(fc, param); if (ret != -ENOPARAM) return ret; } /* If the filesystem doesn't take any arguments, give it the * default handling of source. */ if (strcmp(param->key, "source") == 0) { if (param->type != fs_value_is_string) return invalf(fc, "VFS: Non-string source"); if (fc->source) return invalf(fc, "VFS: Multiple sources"); fc->source = param->string; param->string = NULL; return 0; } return invalf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key); } So you could bloody well just leave recognition (and handling) of "source" to the caller, leaving you with just this: if (strcmp(param->key, "source") == 0) return -ENOPARAM; /* Just log an error for backwards compatibility */ errorf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key); return 0; and that's it.
On Thu, Dec 12, 2019 at 10:36 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > So you could bloody well just leave recognition (and handling) of "source" > to the caller, leaving you with just this: > > if (strcmp(param->key, "source") == 0) > return -ENOPARAM; > /* Just log an error for backwards compatibility */ > errorf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key); > return 0; Which is fine for the old mount(2) interface. But we have a brand new API as well; do we really need to carry these backward compatibility issues forward? I mean checking if a param/flag is supported or not *is* useful and lacking that check is the source of numerous headaches in legacy interfaces (just take the open(2) example and the introduction of O_TMPFILE). Just need a flag in fc indicating if this option comes from the old interface: if (strcmp(param->key, "source") == 0) return -ENOPARAM; /* Just log an error for backwards compatibility */ errorf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key); return fc->legacy ? 0 : -ENOPARAM; And TBH, I think that logic applies to the common flags as well. Some of these simply make no sense on the new interface ("silent", "posixacl") and some are ignored for lots of filesystems ("sync", "dirsync", "mand", "lazytime"). It would also be nice to reject "rw" for read-only filesystems. I have sent patches for the above numerous times, all been ignored by DavidH and Al. While this seems minor now, I think getting this interface into a better shape as early as possible may save lots more headaches later... Thanks, Miklos
On Fri, Dec 13, 2019 at 10:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote: > I have sent patches for the above numerous times, all been ignored by > DavidH and Al. While this seems minor now, I think getting this > interface into a better shape as early as possible may save lots more > headaches later... Refs: https://lore.kernel.org/linux-fsdevel/20191128155940.17530-12-mszeredi@redhat.com/ https://lore.kernel.org/linux-fsdevel/20191128155940.17530-13-mszeredi@redhat.com/ https://lore.kernel.org/linux-fsdevel/20190619123019.30032-7-mszeredi@redhat.com/ etc... Thanks, Miklos
On Fri, Dec 13, 2019 at 10:15:03AM +0100, Miklos Szeredi wrote: > Just need a flag in fc indicating if this option comes from the old interface: > > if (strcmp(param->key, "source") == 0) > return -ENOPARAM; > /* Just log an error for backwards compatibility */ > errorf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, > param->key); > return fc->legacy ? 0 : -ENOPARAM; What the hell for? Just have a separate ->parse_param() instance for "promiscuous fs, will accept bullshit options" and have such filesystems use it explicitly. With default being not that, but rejecting unknowns.
Miklos Szeredi <miklos@szeredi.hu> wrote: > > So you could bloody well just leave recognition (and handling) of "source" > > to the caller, leaving you with just this: > > > > if (strcmp(param->key, "source") == 0) > > return -ENOPARAM; > > /* Just log an error for backwards compatibility */ > > errorf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key); > > return 0; > > Which is fine for the old mount(2) interface. > > But we have a brand new API as well; do we really need to carry these > backward compatibility issues forward? I mean checking if a > param/flag is supported or not *is* useful and lacking that check is > the source of numerous headaches in legacy interfaces (just take the > open(2) example and the introduction of O_TMPFILE). The problem with what you're suggesting is that you can't then make /sbin/mount to use the new syscalls because that would change userspace behaviour - unless you either teach /sbin/mount which filesystems discard which errors from unrecognised options or pass a flag to the kernel to shift into or out of 'strict' mode. David
On Tue, Dec 17, 2019 at 6:49 PM David Howells <dhowells@redhat.com> wrote: > > Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > So you could bloody well just leave recognition (and handling) of "source" > > > to the caller, leaving you with just this: > > > > > > if (strcmp(param->key, "source") == 0) > > > return -ENOPARAM; > > > /* Just log an error for backwards compatibility */ > > > errorf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key); > > > return 0; > > > > Which is fine for the old mount(2) interface. > > > > But we have a brand new API as well; do we really need to carry these > > backward compatibility issues forward? I mean checking if a > > param/flag is supported or not *is* useful and lacking that check is > > the source of numerous headaches in legacy interfaces (just take the > > open(2) example and the introduction of O_TMPFILE). > > The problem with what you're suggesting is that you can't then make > /sbin/mount to use the new syscalls because that would change userspace > behaviour - unless you either teach /sbin/mount which filesystems discard > which errors from unrecognised options or pass a flag to the kernel to shift > into or out of 'strict' mode. The latter has minor cost, so we can add it easily. Long term I think it makes sense to move this mess up to userspace, and hence let util-linux deal with it. Thanks, Miklos
diff --git a/fs/fs_context.c b/fs/fs_context.c index 138b5b4d621d..7ec20b1f8a53 100644 --- a/fs/fs_context.c +++ b/fs/fs_context.c @@ -160,8 +160,7 @@ int vfs_parse_fs_param(struct fs_context *fc, struct fs_parameter *param) return 0; } - return invalf(fc, "%s: Unknown parameter '%s'", - fc->fs_type->name, param->key); + return 0; } EXPORT_SYMBOL(vfs_parse_fs_param);
The new mount API currently rejects unknown parameters if the filesystem doesn't have doesn't take any arguments. This is unfortunately a regression from the old API which silently ignores extra arguments. This is easly seen with the squashfs conversion (5a2be1288b51 ("vfs: Convert squashfs to use the new mount API")) which now fails to mount with extra options. Just get rid of the error. Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context") Link: https://lore.kernel.org/lkml/20191130181548.GA28459@gentoo-tp.home/ Reported-by: Jeremi Piotrowski <jeremi.piotrowski@gmail.com> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1781863 Signed-off-by: Laura Abbott <labbott@redhat.com> --- fs/fs_context.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)