Re: [PATCH] vfs: parse sloppy mount option in correct order

[Roberto Bergantinos Corpas <rbergant@redhat.com>]

David,

  i came up with the following approach, i drop the sloppy booleans
from cifs/nfs and leave it at fs_context level

, let me know what you think and ill send as new version :

diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 7ec35f3f0a5f..5a8c074df74a 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -866,7 +866,7 @@ static int smb3_fs_context_parse_param(struct
fs_context *fc,
     if (!skip_parsing) {
         opt = fs_parse(fc, smb3_fs_parameters, param, &result);
         if (opt < 0)
-            return ctx->sloppy ? 1 : opt;
+            return fc->sloppy ? 1 : opt;
     }

     switch (opt) {
@@ -1412,7 +1412,7 @@ static int smb3_fs_context_parse_param(struct
fs_context *fc,
         ctx->multiuser = true;
         break;
     case Opt_sloppy:
-        ctx->sloppy = true;
+        fc->sloppy = true;
         break;
     case Opt_nosharesock:
         ctx->nosharesock = true;
diff --git a/fs/cifs/fs_context.h b/fs/cifs/fs_context.h
index e54090d9ef36..52a67a96fb67 100644
--- a/fs/cifs/fs_context.h
+++ b/fs/cifs/fs_context.h
@@ -155,7 +155,6 @@ struct smb3_fs_context {
     bool uid_specified;
     bool cruid_specified;
     bool gid_specified;
-    bool sloppy;
     bool got_ip;
     bool got_version;
     bool got_rsize;
diff --git a/fs/fs_context.c b/fs/fs_context.c
index 24ce12f0db32..3c1c999fc1fb 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -155,8 +155,14 @@ int vfs_parse_fs_param(struct fs_context *fc,
struct fs_parameter *param)
     if (ret != -ENOPARAM)
         return ret;

-    return invalf(fc, "%s: Unknown parameter '%s'",
-              fc->fs_type->name, param->key);
+    /* We got an invalid parameter, but sloppy may have been specified
+     * later on param string. Let's wait to process whole params to return
+     * EINVAL */
+
+    fc->param_inval = true;
+    errorf(fc, "%s: Unknown parameter '%s'",fc->fs_type->name, param->key);
+
+    return 0;
 }
 EXPORT_SYMBOL(vfs_parse_fs_param);

@@ -227,6 +233,9 @@ int generic_parse_monolithic(struct fs_context
*fc, void *data)
         }
     }

+    if (!fc->sloppy && fc->param_inval)
+        ret = -EINVAL;
+
     return ret;
 }
 EXPORT_SYMBOL(generic_parse_monolithic);
diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index ea17fa1f31ec..c9ff68e17b68 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -482,7 +482,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,

     opt = fs_parse(fc, nfs_fs_parameters, param, &result);
     if (opt < 0)
-        return ctx->sloppy ? 1 : opt;
+        return fc->sloppy ? 1 : opt;

     if (fc->security)
         ctx->has_sec_mnt_opts = 1;
@@ -837,7 +837,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
          * Special options
          */
     case Opt_sloppy:
-        ctx->sloppy = true;
+        fc->sloppy = true;
         dfprintk(MOUNT, "NFS:   relaxing parsing rules\n");
         break;
     }
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 12f6acb483bb..9febdc95b4d0 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -80,7 +80,6 @@ struct nfs_fs_context {
     bool            internal;
     bool            skip_reconfig_option_check;
     bool            need_mount;
-    bool            sloppy;
     unsigned int        flags;        /* NFS{,4}_MOUNT_* flags */
     unsigned int        rsize, wsize;
     unsigned int        timeo, retrans;
diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
index 13fa6f3df8e4..06a4b72a0f98 100644
--- a/include/linux/fs_context.h
+++ b/include/linux/fs_context.h
@@ -110,6 +110,8 @@ struct fs_context {
     bool            need_free:1;    /* Need to call ops->free() */
     bool            global:1;    /* Goes into &init_user_ns */
     bool            oldapi:1;    /* Coming from mount(2) */
+    bool                    sloppy:1;       /* If fs support it and
was specified */
+    bool                    param_inval:1;  /* If set, check sloppy value */
 };

 struct fs_context_operations {

On Thu, Aug 26, 2021 at 2:29 PM David Howells <dhowells@redhat.com> wrote:
>
> Roberto Bergantinos Corpas <rbergant@redhat.com> wrote:
>
> > With addition of fs_context support, options string is parsed
> > sequentially, if 'sloppy' option is not leftmost one, we may
> > return ENOPARAM to userland if a non-valid option preceeds sloopy
> > and mount will fail :
> >
> > host# mount -o quota,sloppy 172.23.1.225:/share /mnt
> > mount.nfs: an incorrect mount option was specified
> > host# mount -o sloppy,quota 172.23.1.225:/share /mnt
> > host#
> >
> > This patch correct that behaviour so that sloppy takes precedence
> > if specified anywhere on the string
>
> It's slightly overcorrected, but that probably doesn't matter.
>
> I wonder if we should put a "bool sloppy" in struct fs_context, put the
> handling in vfs_parse_fs_param() (ie. skip the error message).
>
> David
>