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

[Laura Abbott <labbott@redhat.com>]

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