The new mount API relies on file systems to provide a ->parse_params
function to handle parsing of arguments. If a file system doesn't
have a ->parse_param function, it falls back to parsing the source
option and rejecting all other options. This is a change in behavior
for some file systems which would just quietly ignore extra options
and mount successfully. This was noticed by users as squashfs failing
to mount with extra options after the conversion to the new mount
API.
File systems with a ->parse_params function rely on the top level
to parse the "source" param so we can't easily move that around. To
get around this, introduce a default parsing functions for file
systems that take no arguments. This parses only the "source" option
and only logs an error for other arguments. Update the comment
to reflect this expected behavior for "source" parsing as well.
Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock
creation/configuration context")
Link: https://lore.kernel.org/lkml/[email protected]/
Reported-by: Jeremi Piotrowski <[email protected]>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1781863
Signed-off-by: Laura Abbott <[email protected]>
---
I cleaned up my original suggestion a bit as I realized things would
be easier if we just made this a default option if there's no parsing.
Lightly tested only.
---
fs/fs_context.c | 69 ++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 62 insertions(+), 7 deletions(-)
diff --git a/fs/fs_context.c b/fs/fs_context.c
index 138b5b4d621d..8c5dc131e29a 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -107,6 +107,55 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key)
return -ENOPARAM;
}
+enum {
+ GENERIC_FS_SOURCE,
+};
+
+static const struct fs_parameter_spec generic_fs_param_specs[] = {
+ fsparam_string ("source", GENERIC_FS_SOURCE),
+ {}
+};
+
+const struct fs_parameter_description generic_fs_parameters = {
+ .name = "generic_fs",
+ .specs = generic_fs_param_specs,
+};
+
+/**
+ * fs_generic_parse_param - ->parse_param function for a file system that
+ * takes no arguments
+ * @fc: The filesystem context
+ * @param: The parameter.
+ */
+static int fs_generic_parse_param(struct fs_context *fc, struct fs_parameter *param)
+{
+ struct fs_parse_result result;
+ int opt;
+
+ opt = fs_parse(fc, &generic_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 GENERIC_FS_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;
+}
+
/**
* vfs_parse_fs_param - Add a single parameter to a superblock config
* @fc: The filesystem context to modify
@@ -126,6 +175,7 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key)
int vfs_parse_fs_param(struct fs_context *fc, struct fs_parameter *param)
{
int ret;
+ int (*parse_param)(struct fs_context *, struct fs_parameter *);
if (!param->key)
return invalf(fc, "Unnamed parameter\n");
@@ -141,14 +191,19 @@ int vfs_parse_fs_param(struct fs_context *fc, struct fs_parameter *param)
*/
return ret;
- if (fc->ops->parse_param) {
- ret = fc->ops->parse_param(fc, param);
- if (ret != -ENOPARAM)
- return ret;
- }
+ parse_param = fc->ops->parse_param;
+ if (!parse_param)
+ parse_param = fs_generic_parse_param;
+
+ ret = parse_param(fc, param);
+ if (ret != -ENOPARAM)
+ return ret;
- /* If the filesystem doesn't take any arguments, give it the
- * default handling of source.
+ /*
+ * File systems may have a ->parse_param function but rely on
+ * the top level to parse the source function. File systems
+ * may have their own source parsing though so this needs
+ * to come after the call to parse_param above.
*/
if (strcmp(param->key, "source") == 0) {
if (param->type != fs_value_is_string)
--
2.21.0
On Thu, Dec 12, 2019 at 04:36:04PM -0500, Laura Abbott wrote:
> @@ -141,14 +191,19 @@ int vfs_parse_fs_param(struct fs_context *fc, struct fs_parameter *param)
> */
> return ret;
>
> - if (fc->ops->parse_param) {
> - ret = fc->ops->parse_param(fc, param);
> - if (ret != -ENOPARAM)
> - return ret;
> - }
> + parse_param = fc->ops->parse_param;
> + if (!parse_param)
> + parse_param = fs_generic_parse_param;
> +
> + ret = parse_param(fc, param);
> + if (ret != -ENOPARAM)
> + return ret;
>
> - /* If the filesystem doesn't take any arguments, give it the
> - * default handling of source.
> + /*
> + * File systems may have a ->parse_param function but rely on
> + * the top level to parse the source function. File systems
> + * may have their own source parsing though so this needs
> + * to come after the call to parse_param above.
> */
> if (strcmp(param->key, "source") == 0) {
> if (param->type != fs_value_is_string)
> --
> 2.21.0
No. Please, get rid of the boilerplate. About 80% of that thing
is an absolutely pointless dance around "but we need that to call
fs_parse()". We do *NOT* need to call fs_parse() here. We do
not need a struct fs_parameter_description instance. We do not
need struct fs_parameter_spec instances. We do not need a magical
global constant. And I'm not entirely convinced that we need
to make fs_generic_parse_param() default - filesystems that
want this behaviour can easily ask for it. A sane default is
to reject any bogus options.
I would call it ignore_unknowns_parse_param(), while we are at it.
On 12/12/19 4:47 PM, Al Viro wrote:
> On Thu, Dec 12, 2019 at 04:36:04PM -0500, Laura Abbott wrote:
>> @@ -141,14 +191,19 @@ int vfs_parse_fs_param(struct fs_context *fc, struct fs_parameter *param)
>> */
>> return ret;
>>
>> - if (fc->ops->parse_param) {
>> - ret = fc->ops->parse_param(fc, param);
>> - if (ret != -ENOPARAM)
>> - return ret;
>> - }
>> + parse_param = fc->ops->parse_param;
>> + if (!parse_param)
>> + parse_param = fs_generic_parse_param;
>> +
>> + ret = parse_param(fc, param);
>> + if (ret != -ENOPARAM)
>> + return ret;
>>
>> - /* If the filesystem doesn't take any arguments, give it the
>> - * default handling of source.
>> + /*
>> + * File systems may have a ->parse_param function but rely on
>> + * the top level to parse the source function. File systems
>> + * may have their own source parsing though so this needs
>> + * to come after the call to parse_param above.
>> */
>> if (strcmp(param->key, "source") == 0) {
>> if (param->type != fs_value_is_string)
>> --
>> 2.21.0
>
> No. Please, get rid of the boilerplate. About 80% of that thing
> is an absolutely pointless dance around "but we need that to call
> fs_parse()". We do *NOT* need to call fs_parse() here. We do
> not need a struct fs_parameter_description instance. We do not
> need struct fs_parameter_spec instances. We do not need a magical
> global constant. And I'm not entirely convinced that we need
> to make fs_generic_parse_param() default - filesystems that
> want this behaviour can easily ask for it. A sane default is
> to reject any bogus options.
>
Well the existing behavior was to silently ignore options by
default so this was an attempt to preserve that behavior for
everything rather than keep hitting user visible bugs. I'd
rather not have to deal this this for each file system.
> I would call it ignore_unknowns_parse_param(), while we are at it.
>
Hi Laura,
I love your patch! Perhaps something to improve:
[auto build test WARNING on vfs/for-next]
[cannot apply to dhowells-fs/fscache-next linus/master v5.5-rc2 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Laura-Abbott/vfs-Handle-file-systems-without-parse_params-better/20191216-053622
base: https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-next
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-129-g341daf20-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> fs/fs_context.c:119:39: sparse: sparse: symbol 'generic_fs_parameters' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Fixes: fa45c7e4862f ("vfs: Handle file systems without ->parse_params better")
Signed-off-by: kbuild test robot <[email protected]>
---
fs_context.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/fs_context.c b/fs/fs_context.c
index 8c5dc131e29ac..604f1a3d73aac 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -116,7 +116,7 @@ static const struct fs_parameter_spec generic_fs_param_specs[] = {
{}
};
-const struct fs_parameter_description generic_fs_parameters = {
+static const struct fs_parameter_description generic_fs_parameters = {
.name = "generic_fs",
.specs = generic_fs_param_specs,
};