[PATCH] vfs: Don't reject unknown parameters

[PATCH] vfs: Don't reject unknown parameters

[Laura Abbott]

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);
 
-- 
2.21.0

Re: [PATCH] vfs: Don't reject unknown parameters

[Ilya Dryomov]

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

Re: [PATCH] vfs: Don't reject unknown parameters

[Laura Abbott]

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

Re: [PATCH] vfs: Don't reject unknown parameters

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 788 bytes --]

 fs/squashfs/super.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index 0cc4ceec0562..fbd1e05b39aa 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -355,9 +355,18 @@ static int squashfs_reconfigure(struct fs_context *fc)
 	return 0;
 }
 
+/* Print the warning, but ignore it as an error */
+static int squashfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
+{
+	invalf(fc, "%s: Unknown parameter '%s'",
+		fc->fs_type->name, param->key);
+	return 0;
+}
+
 static const struct fs_context_operations squashfs_context_ops = {
 	.get_tree	= squashfs_get_tree,
 	.reconfigure	= squashfs_reconfigure,
+	.parse_param	= squashfs_parse_param,
 };
 
 static int squashfs_init_fs_context(struct fs_context *fc)

Re: [PATCH] vfs: Don't reject unknown parameters

[Laura Abbott]

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:
   *

Re: [PATCH] vfs: Don't reject unknown parameters

[Al Viro]

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.

Re: [PATCH] vfs: Don't reject unknown parameters

[Miklos Szeredi]

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

Re: [PATCH] vfs: Don't reject unknown parameters

[Miklos Szeredi]

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

Re: [PATCH] vfs: Don't reject unknown parameters

[Al Viro]

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.

Re: [PATCH] vfs: Don't reject unknown parameters

[David Howells]

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

Re: [PATCH] vfs: Don't reject unknown parameters

[Miklos Szeredi]

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