* [PATCH] vfs: parse sloppy mount option in correct order
@ 2021-07-21 11:30 Roberto Bergantinos Corpas
2021-08-26 12:29 ` David Howells
2021-08-26 12:46 ` Matthew Wilcox
0 siblings, 2 replies; 10+ messages in thread
From: Roberto Bergantinos Corpas @ 2021-07-21 11:30 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel
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
Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
---
fs/fs_context.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/fs_context.c b/fs/fs_context.c
index de1985eae535..f930808e9db8 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -210,8 +210,12 @@ int generic_parse_monolithic(struct fs_context *fc, void *data)
if (ret)
return ret;
+ /* 'sloppy' should be parsed first */
+ if (strstr((const char *)data, "sloppy") != NULL)
+ vfs_parse_fs_string(fc, "sloppy", NULL, 0);
+
while ((key = strsep(&options, ",")) != NULL) {
- if (*key) {
+ if (*key && strcmp(key, "sloppy")) {
size_t v_len = 0;
char *value = strchr(key, '=');
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] vfs: parse sloppy mount option in correct order
2021-07-21 11:30 [PATCH] vfs: parse sloppy mount option in correct order Roberto Bergantinos Corpas
@ 2021-08-26 12:29 ` David Howells
2022-01-27 14:42 ` Roberto Bergantinos Corpas
2021-08-26 12:46 ` Matthew Wilcox
1 sibling, 1 reply; 10+ messages in thread
From: David Howells @ 2021-08-26 12:29 UTC (permalink / raw)
To: Roberto Bergantinos Corpas; +Cc: dhowells, viro, linux-fsdevel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] vfs: parse sloppy mount option in correct order
2021-07-21 11:30 [PATCH] vfs: parse sloppy mount option in correct order Roberto Bergantinos Corpas
2021-08-26 12:29 ` David Howells
@ 2021-08-26 12:46 ` Matthew Wilcox
2021-08-26 13:05 ` Eric Sandeen
1 sibling, 1 reply; 10+ messages in thread
From: Matthew Wilcox @ 2021-08-26 12:46 UTC (permalink / raw)
To: Roberto Bergantinos Corpas; +Cc: viro, linux-fsdevel
On Wed, Jul 21, 2021 at 01:30:57PM +0200, Roberto Bergantinos Corpas 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
It isn't clear to me that this is incorrect behaviour. Perhaps the user
actually wants the options to the left parsed strictly and the options
to the right parsed sloppily?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] vfs: parse sloppy mount option in correct order
2021-08-26 12:46 ` Matthew Wilcox
@ 2021-08-26 13:05 ` Eric Sandeen
2021-08-26 13:32 ` Roberto Bergantinos Corpas
0 siblings, 1 reply; 10+ messages in thread
From: Eric Sandeen @ 2021-08-26 13:05 UTC (permalink / raw)
To: Matthew Wilcox, Roberto Bergantinos Corpas; +Cc: viro, linux-fsdevel
On 8/26/21 7:46 AM, Matthew Wilcox wrote:
> On Wed, Jul 21, 2021 at 01:30:57PM +0200, Roberto Bergantinos Corpas 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
>
> It isn't clear to me that this is incorrect behaviour. Perhaps the user
> actually wants the options to the left parsed strictly and the options
> to the right parsed sloppily?
I don't think mount options have ever been order-dependent, at least not
intentionally so, have they?
And what matters most here is surely "how did it work before the mount
API change?"
And it seems to me that previously, invalid options were noted, and whether the
mount would fail was left to the end, depending on whether sloppy was seen
anywhere in the mount options string. This is the old option parsing:
while ((p = strsep(&raw, ",")) != NULL) {
...
switch (token) {
...
case Opt_sloppy:
sloppy = 1;
dfprintk(MOUNT, "NFS: relaxing parsing rules\n");
break;
...
default:
invalid_option = 1;
dfprintk(MOUNT, "NFS: unrecognized mount option "
"'%s'\n", p);
}
}
if (!sloppy && invalid_option)
return 0;
-Eric
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] vfs: parse sloppy mount option in correct order
2021-08-26 13:05 ` Eric Sandeen
@ 2021-08-26 13:32 ` Roberto Bergantinos Corpas
2021-09-23 12:28 ` David Wysochanski
0 siblings, 1 reply; 10+ messages in thread
From: Roberto Bergantinos Corpas @ 2021-08-26 13:32 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Matthew Wilcox, viro, linux-fsdevel
On Thu, Aug 26, 2021 at 3:13 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>
> On 8/26/21 7:46 AM, Matthew Wilcox wrote:
> > On Wed, Jul 21, 2021 at 01:30:57PM +0200, Roberto Bergantinos Corpas 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
> >
> > It isn't clear to me that this is incorrect behaviour. Perhaps the user
> > actually wants the options to the left parsed strictly and the options
> > to the right parsed sloppily?
>
> I don't think mount options have ever been order-dependent, at least not
> intentionally so, have they?
>
> And what matters most here is surely "how did it work before the mount
> API change?"
>> And it seems to me that previously, invalid options were noted, and whether the
> mount would fail was left to the end, depending on whether sloppy was seen
> anywhere in the mount options string. This is the old option parsing:
>
> while ((p = strsep(&raw, ",")) != NULL) {
> ...
> switch (token) {
> ...
> case Opt_sloppy:
> sloppy = 1;
> dfprintk(MOUNT, "NFS: relaxing parsing rules\n");
> break;
> ...
> default:
> invalid_option = 1;
> dfprintk(MOUNT, "NFS: unrecognized mount option "
> "'%s'\n", p);
> }
> }
>
> if (!sloppy && invalid_option)
> return 0;
Agree, that's my main point. I think that breaks from previous
behaviour and indeed causes issues on the field.
My understanding too is that there's no order-dependency.
roberto
>
> -Eric
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] vfs: parse sloppy mount option in correct order
2021-08-26 13:32 ` Roberto Bergantinos Corpas
@ 2021-09-23 12:28 ` David Wysochanski
2021-09-23 14:13 ` Roberto Bergantinos Corpas
0 siblings, 1 reply; 10+ messages in thread
From: David Wysochanski @ 2021-09-23 12:28 UTC (permalink / raw)
To: Roberto Bergantinos Corpas
Cc: Eric Sandeen, Matthew Wilcox, Alexander Viro, linux-fsdevel
On Thu, Aug 26, 2021 at 9:32 AM Roberto Bergantinos Corpas
<rbergant@redhat.com> wrote:
>
> On Thu, Aug 26, 2021 at 3:13 PM Eric Sandeen <sandeen@sandeen.net> wrote:
> >
> > On 8/26/21 7:46 AM, Matthew Wilcox wrote:
> > > On Wed, Jul 21, 2021 at 01:30:57PM +0200, Roberto Bergantinos Corpas 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
> > >
> > > It isn't clear to me that this is incorrect behaviour. Perhaps the user
> > > actually wants the options to the left parsed strictly and the options
> > > to the right parsed sloppily?
> >
> > I don't think mount options have ever been order-dependent, at least not
> > intentionally so, have they?
> >
> > And what matters most here is surely "how did it work before the mount
> > API change?"
> >> And it seems to me that previously, invalid options were noted, and whether the
> > mount would fail was left to the end, depending on whether sloppy was seen
> > anywhere in the mount options string. This is the old option parsing:
> >
> > while ((p = strsep(&raw, ",")) != NULL) {
> > ...
> > switch (token) {
> > ...
> > case Opt_sloppy:
> > sloppy = 1;
> > dfprintk(MOUNT, "NFS: relaxing parsing rules\n");
> > break;
> > ...
> > default:
> > invalid_option = 1;
> > dfprintk(MOUNT, "NFS: unrecognized mount option "
> > "'%s'\n", p);
> > }
> > }
> >
> > if (!sloppy && invalid_option)
> > return 0;
>
> Agree, that's my main point. I think that breaks from previous
> behaviour and indeed causes issues on the field.
> My understanding too is that there's no order-dependency.
>
> roberto
>
Hi Roberto, Eric and David H, this seems to have stalled. What are
the next steps here?
Roberto, it sounded like David H was suggesting maybe an alternative
approach. Did you look into that?
Then there was a discussion about whether this change was intended
behavior or not, and the main point made was it was a change in
behavior, which I don't think was intended. I guess one could make
the argument order was never guaranteed, but then why make behavior
order dependent now if it was not intentional kernel change? I agree
if possible we should retain the order independent behavior in the
kernel.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] vfs: parse sloppy mount option in correct order
2021-09-23 12:28 ` David Wysochanski
@ 2021-09-23 14:13 ` Roberto Bergantinos Corpas
0 siblings, 0 replies; 10+ messages in thread
From: Roberto Bergantinos Corpas @ 2021-09-23 14:13 UTC (permalink / raw)
To: David Wysochanski
Cc: Eric Sandeen, Matthew Wilcox, Alexander Viro, linux-fsdevel
> Roberto, it sounded like David H was suggesting maybe an alternative
> approach. Did you look into that?
No, sorry about that, it is in my todo list. i'll take a look at it
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] vfs: parse sloppy mount option in correct order
2021-08-26 12:29 ` David Howells
@ 2022-01-27 14:42 ` Roberto Bergantinos Corpas
0 siblings, 0 replies; 10+ messages in thread
From: Roberto Bergantinos Corpas @ 2022-01-27 14:42 UTC (permalink / raw)
To: David Howells; +Cc: Alexander Viro, linux-fsdevel
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
>
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] vfs: parse sloppy mount option in correct order
2022-09-28 1:09 Ian Kent
@ 2023-01-27 11:13 ` Jeff Layton
0 siblings, 0 replies; 10+ messages in thread
From: Jeff Layton @ 2023-01-27 11:13 UTC (permalink / raw)
To: Ian Kent, Al Viro
Cc: Kernel Mailing List, linux-fsdevel, Steve French, linux-cifs,
Trond Myklebust, linux-nfs-list, David Wysochanski,
David Howells, Miklos Szeredi, Roberto Bergantinos Corpas
On Wed, 2022-09-28 at 09:09 +0800, Ian Kent wrote:
> From: Roberto Bergantinos Corpas <rbergant@redhat.com>
>
> 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
>
> changes since v1:
> - add a boolean to fs context and postpone error reporting until
> parsing is done.
>
> Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
> ---
> fs/cifs/fs_context.c | 4 ++--
> fs/cifs/fs_context.h | 1 -
> fs/fs_context.c | 14 ++++++++++++--
> fs/nfs/fs_context.c | 5 +++--
> fs/nfs/internal.h | 1 -
> include/linux/fs_context.h | 2 ++
> 6 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> index 0e13dec86b25..32c3fdd7d27a 100644
> --- a/fs/cifs/fs_context.c
> +++ b/fs/cifs/fs_context.c
> @@ -864,7 +864,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) {
> @@ -1420,7 +1420,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 bbaee4c2281f..75e4c41466fa 100644
> --- a/fs/cifs/fs_context.h
> +++ b/fs/cifs/fs_context.h
> @@ -157,7 +157,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 df04e5fc6d66..911a36bf2226 100644
> --- a/fs/fs_context.c
> +++ b/fs/fs_context.c
> @@ -157,8 +157,15 @@ 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);
Is it correct to store an error message when we don't know whether
"sloppy" has been specified yet?
> +
> + return 0;
> }
> EXPORT_SYMBOL(vfs_parse_fs_param);
>
> @@ -234,6 +241,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 4da701fd1424..09da63cc84f7 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -485,7 +485,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 (opt == -ENOPARAM && ctx->sloppy) ? 1 : opt;
> + return (opt == -ENOPARAM && fc->sloppy) ? 1 : opt;
>
> if (fc->security)
> ctx->has_sec_mnt_opts = 1;
> @@ -853,7 +853,8 @@ 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 898dd95bc7a7..83552def96f1 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -90,7 +90,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 ff1375a16c8c..d91d42bc06ce 100644
> --- a/include/linux/fs_context.h
> +++ b/include/linux/fs_context.h
> @@ -111,6 +111,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 {
>
>
Overall, the patch looks reasonable though.
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] vfs: parse sloppy mount option in correct order
@ 2022-09-28 1:09 Ian Kent
2023-01-27 11:13 ` Jeff Layton
0 siblings, 1 reply; 10+ messages in thread
From: Ian Kent @ 2022-09-28 1:09 UTC (permalink / raw)
To: Al Viro
Cc: Kernel Mailing List, linux-fsdevel, Steve French, linux-cifs,
Trond Myklebust, linux-nfs-list, Jeff Layton, David Wysochanski,
David Howells, Miklos Szeredi, Roberto Bergantinos Corpas
From: Roberto Bergantinos Corpas <rbergant@redhat.com>
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
changes since v1:
- add a boolean to fs context and postpone error reporting until
parsing is done.
Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
---
fs/cifs/fs_context.c | 4 ++--
fs/cifs/fs_context.h | 1 -
fs/fs_context.c | 14 ++++++++++++--
fs/nfs/fs_context.c | 5 +++--
fs/nfs/internal.h | 1 -
include/linux/fs_context.h | 2 ++
6 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 0e13dec86b25..32c3fdd7d27a 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -864,7 +864,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) {
@@ -1420,7 +1420,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 bbaee4c2281f..75e4c41466fa 100644
--- a/fs/cifs/fs_context.h
+++ b/fs/cifs/fs_context.h
@@ -157,7 +157,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 df04e5fc6d66..911a36bf2226 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -157,8 +157,15 @@ 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);
@@ -234,6 +241,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 4da701fd1424..09da63cc84f7 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -485,7 +485,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 (opt == -ENOPARAM && ctx->sloppy) ? 1 : opt;
+ return (opt == -ENOPARAM && fc->sloppy) ? 1 : opt;
if (fc->security)
ctx->has_sec_mnt_opts = 1;
@@ -853,7 +853,8 @@ 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 898dd95bc7a7..83552def96f1 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -90,7 +90,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 ff1375a16c8c..d91d42bc06ce 100644
--- a/include/linux/fs_context.h
+++ b/include/linux/fs_context.h
@@ -111,6 +111,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 {
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-01-27 11:14 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 11:30 [PATCH] vfs: parse sloppy mount option in correct order Roberto Bergantinos Corpas
2021-08-26 12:29 ` David Howells
2022-01-27 14:42 ` Roberto Bergantinos Corpas
2021-08-26 12:46 ` Matthew Wilcox
2021-08-26 13:05 ` Eric Sandeen
2021-08-26 13:32 ` Roberto Bergantinos Corpas
2021-09-23 12:28 ` David Wysochanski
2021-09-23 14:13 ` Roberto Bergantinos Corpas
2022-09-28 1:09 Ian Kent
2023-01-27 11:13 ` Jeff Layton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.