[RFC PATCH] Legacy mount option "sloppy" support

[RFC PATCH] Legacy mount option "sloppy" support

[Ian Kent]

There's been some recent discussion about support of the "sloppy"
mount option.

It's an option that people want to get rid of from time to time and
when we do we get complaints and end up having to re-instate it.

I think the (fairly) recent mount API changes are the best way to
eliminate the need for this option over time.

That's because this option doesn't quite make sense for fsconfig() and
the knowledge of how to handle invalid options for some file systems
needs be included in the user space logic when using the mount API.
At the very least there's no way to order the options since that's
determined by the order of fsconfig() calls.

Karel Zak is in the process of adding support for the mount API to
util-linux.

Karel do you find what I'm saying is accurate?
Do you think we will be able to get rid of the sloppy option over
time with the move to use the mount API?

Assuming I am correct, we need only implement handling of the sloppy
option in the monolithic option parser which is what the attached
patch does.

If it's decided we really do need to retain this option for fsconfig()
then we could remove LEGACY from the fs_flags bit field name and add
the handling to the parameter parsing code. In this case user space
code calling fsconfig() would still need to be mindful of option order.

At this stage I've not removed the sloppy option definition from
the file systems that allow it but with the file system type flag
that should be possible too, since this is more about handling the
parse return than an actual file system option.

I'm keen to hear what people think about how we should handle this
so any comments are welcome.
---

Ian Kent (1):
      vfs: handle sloppy option in fs context monolithic parser


 fs/cifs/fs_context.c |  2 +-
 fs/fs_context.c      | 31 ++++++++++++++++++++++++++++++-
 fs/nfs/fs_context.c  |  2 +-
 include/linux/fs.h   |  1 +
 4 files changed, 33 insertions(+), 3 deletions(-)

--
Ian

[RFC PATCH] vfs: handle sloppy option in fs context monolithic parser

[Ian Kent]

The sloppy option doesn't make sense for fsconfig() and knowedge of how
to handle this case needs to be present in the caller. It does make
sense in the legacy options parser, generic_parse_monolithic(), so it
should allow for it.

The sloppy option needs to be independent of the order in which it's
given.

The simplest way to do this in generic_parse_monolithic() is to check
for it's presence, check if the file system supports it, then skip
occurrances of it when walking the options string.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/cifs/fs_context.c |    2 +-
 fs/fs_context.c      |   31 ++++++++++++++++++++++++++++++-
 fs/nfs/fs_context.c  |    2 +-
 include/linux/fs.h   |    1 +
 4 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 6d13f8207e96..d7e9356797ab 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 opt;
 	}
 
 	switch (opt) {
diff --git a/fs/fs_context.c b/fs/fs_context.c
index 24ce12f0db32..fa179e1b8061 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -187,6 +187,28 @@ int vfs_parse_fs_string(struct fs_context *fc, const char *key,
 }
 EXPORT_SYMBOL(vfs_parse_fs_string);
 
+
+static bool check_for_sloppy_option(void *options)
+{
+	char *sloppy;
+	char last;
+
+	sloppy = strstr(options, "sloppy");
+	if (!sloppy)
+		return false;
+
+	last = sloppy[6];
+
+	if (sloppy == options) {
+		if (last == 0 || last == ',')
+			return true;
+	} else if (*(--sloppy) == ',') {
+		if (last == 0 || last == ',')
+			return true;
+	}
+	return false;
+}
+
 /**
  * generic_parse_monolithic - Parse key[=val][,key[=val]]* mount data
  * @ctx: The superblock configuration to fill in.
@@ -201,6 +223,7 @@ EXPORT_SYMBOL(vfs_parse_fs_string);
 int generic_parse_monolithic(struct fs_context *fc, void *data)
 {
 	char *options = data, *key;
+	bool sloppy = false;
 	int ret = 0;
 
 	if (!options)
@@ -210,11 +233,17 @@ int generic_parse_monolithic(struct fs_context *fc, void *data)
 	if (ret)
 		return ret;
 
+	if (fc->fs_type->fs_flags & FS_ALLOW_LEGACY_SLOPPY)
+		sloppy = check_for_sloppy_option(options);
+
 	while ((key = strsep(&options, ",")) != NULL) {
 		if (*key) {
 			size_t v_len = 0;
 			char *value = strchr(key, '=');
 
+			if (sloppy && !strcmp(key, "sloppy"))
+				continue;
+
 			if (value) {
 				if (value == key)
 					continue;
@@ -222,7 +251,7 @@ int generic_parse_monolithic(struct fs_context *fc, void *data)
 				v_len = strlen(value);
 			}
 			ret = vfs_parse_fs_string(fc, key, value, v_len);
-			if (ret < 0)
+			if (!sloppy && ret < 0)
 				break;
 		}
 	}
diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 9bcd53d5c7d4..e8818b68ce3f 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;
 
 	if (fc->security)
 		ctx->has_sec_mnt_opts = 1;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c1769a2c5d70..ca05111767cb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2533,6 +2533,7 @@ struct file_system_type {
 #define FS_USERNS_MOUNT		8	/* Can be mounted by userns root */
 #define FS_DISALLOW_NOTIFY_PERM	16	/* Disable fanotify permission events */
 #define FS_ALLOW_IDMAP         32      /* FS has been updated to handle vfs idmappings. */
+#define FS_ALLOW_LEGACY_SLOPPY	64	/* FS allows "sloppy" option handling behaviour */
 #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move() during rename() internally. */
 	int (*init_fs_context)(struct fs_context *);
 	const struct fs_parameter_spec *parameters;

Re: [RFC PATCH] Legacy mount option "sloppy" support

[Karel Zak]

On Fri, Mar 24, 2023 at 01:39:09PM +0800, Ian Kent wrote:
> Karel do you find what I'm saying is accurate?
> Do you think we will be able to get rid of the sloppy option over
> time with the move to use the mount API?

The question is what we're talking about :-)

For mount(8) and libmount, there is nothing like the "sloppy" mount option.

If you use it in your fstab or as "mount -o sloppy" on the command line,
then it's used as any other fs-specific mount option; the library copies
the string to mount(2) or fsconfig(2) syscall. The library has no clue 
what the string means (it's the same as "mount -o foobar").

But there is another "sloppy" :-) The command line argument, "mount -s".

This argument is not internally used by libmount or mount(8), but it's
repeated on /sbin/mount.<type> command lines (e.g., "mount -t nfs -s"
means "/sbin/mount.nfs -s").

I guess more interesting is mount.nfs, where both "-s" and "sloppy" lives
together. The mount.nfs uses this option to be tolerant when parsing mount
options string, and it also seems it converts "-s" to "sloppy" string for
mount syscall.

So, for mount(8)/libmount, digging a grave for the "sloppy" will be trivial.

All I need is to add a note about depreciation to the man page and later
remove "-s" from /sbin/mount.<type> command line.

SteveD (in CC:) will comment on it from the NFS point of view because the
real fun happens there :-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [RFC PATCH] Legacy mount option "sloppy" support

[Ian Kent]

On 29/3/23 02:48, Karel Zak wrote:
> On Fri, Mar 24, 2023 at 01:39:09PM +0800, Ian Kent wrote:
>> Karel do you find what I'm saying is accurate?
>> Do you think we will be able to get rid of the sloppy option over
>> time with the move to use the mount API?
> The question is what we're talking about :-)
>
> For mount(8) and libmount, there is nothing like the "sloppy" mount option.
>
> If you use it in your fstab or as "mount -o sloppy" on the command line,
> then it's used as any other fs-specific mount option; the library copies
> the string to mount(2) or fsconfig(2) syscall. The library has no clue
> what the string means (it's the same as "mount -o foobar").

Which is what the problem really is.


If anyone uses this option with a file system that has previously

allowed it then mounts fail if it isn't handled properly. Then the

intended purpose of it is irrelevant because it causes a fail.


I guess the notion of ignoring it for fsconfig(), assuming it isn't

actually needed for the option handling, might not be a viable idea

... although I haven't actually added that to fsconfig(), I probably

should add that to this series.


But first the question of whether the option is actually needed anymore

by those that allow it needs to be answered.


In case anyone has forgotten it was introduced because, at one time

different OSes supported slightly different options for for the same

thing and one could not include multiple options for the same thing

in automount map entries without causing the mount to fail.


So we also need to answer, is this option conflict still present for

any of the file systems that allow it, currently nfs, cifs and ntfs

(I'll need to look up the ntfs maintainer but lets answer this for

nfs and cifs first).


If it isn't actually needed ignoring it in fsconfig() (a deprecation

warning would be in order) and eventually getting rid of it would be

a good idea, yes?


>
> But there is another "sloppy" :-) The command line argument, "mount -s".
>
> This argument is not internally used by libmount or mount(8), but it's
> repeated on /sbin/mount.<type> command lines (e.g., "mount -t nfs -s"
> means "/sbin/mount.nfs -s").
>
> I guess more interesting is mount.nfs, where both "-s" and "sloppy" lives
> together. The mount.nfs uses this option to be tolerant when parsing mount
> options string, and it also seems it converts "-s" to "sloppy" string for
> mount syscall.

I don't think it makes any difference given the original purpose of

it but hopefully Steve will know/remember.


>
> So, for mount(8)/libmount, digging a grave for the "sloppy" will be trivial.
>
> All I need is to add a note about depreciation to the man page and later
> remove "-s" from /sbin/mount.<type> command line.

Right.


>
> SteveD (in CC:) will comment on it from the NFS point of view because the
> real fun happens there :-)

Yep.


Thanks,

Ian

Re: [RFC PATCH] Legacy mount option "sloppy" support

[Christian Brauner]

On Wed, Mar 29, 2023 at 09:03:51AM +0800, Ian Kent wrote:
> On 29/3/23 02:48, Karel Zak wrote:
> > On Fri, Mar 24, 2023 at 01:39:09PM +0800, Ian Kent wrote:
> > > Karel do you find what I'm saying is accurate?
> > > Do you think we will be able to get rid of the sloppy option over
> > > time with the move to use the mount API?
> > The question is what we're talking about :-)
> > 
> > For mount(8) and libmount, there is nothing like the "sloppy" mount option.
> > 
> > If you use it in your fstab or as "mount -o sloppy" on the command line,
> > then it's used as any other fs-specific mount option; the library copies
> > the string to mount(2) or fsconfig(2) syscall. The library has no clue
> > what the string means (it's the same as "mount -o foobar").
> 
> Which is what the problem really is.
> 
> 
> If anyone uses this option with a file system that has previously
> 
> allowed it then mounts fail if it isn't handled properly. Then the
> 
> intended purpose of it is irrelevant because it causes a fail.
> 
> 
> I guess the notion of ignoring it for fsconfig(), assuming it isn't
> 
> actually needed for the option handling, might not be a viable idea
> 
> ... although I haven't actually added that to fsconfig(), I probably
> 
> should add that to this series.
> 
> 
> But first the question of whether the option is actually needed anymore
> 
> by those that allow it needs to be answered.
> 
> 
> In case anyone has forgotten it was introduced because, at one time
> 
> different OSes supported slightly different options for for the same
> 
> thing and one could not include multiple options for the same thing
> 
> in automount map entries without causing the mount to fail.
> 
> 
> So we also need to answer, is this option conflict still present for
> 
> any of the file systems that allow it, currently nfs, cifs and ntfs
> 
> (I'll need to look up the ntfs maintainer but lets answer this for
> 
> nfs and cifs first).
> 
> 
> If it isn't actually needed ignoring it in fsconfig() (a deprecation
> 
> warning would be in order) and eventually getting rid of it would be
> 
> a good idea, yes?

Yes, I think this is a good idea.
The whole reason for this mount option seems a bit hacky tbh so getting
rid of it would be great.

Re: [RFC PATCH] Legacy mount option "sloppy" support

[Ian Kent]

On 3/4/23 21:08, Christian Brauner wrote:
> On Wed, Mar 29, 2023 at 09:03:51AM +0800, Ian Kent wrote:
>> On 29/3/23 02:48, Karel Zak wrote:
>>> On Fri, Mar 24, 2023 at 01:39:09PM +0800, Ian Kent wrote:
>>>> Karel do you find what I'm saying is accurate?
>>>> Do you think we will be able to get rid of the sloppy option over
>>>> time with the move to use the mount API?
>>> The question is what we're talking about :-)
>>>
>>> For mount(8) and libmount, there is nothing like the "sloppy" mount option.
>>>
>>> If you use it in your fstab or as "mount -o sloppy" on the command line,
>>> then it's used as any other fs-specific mount option; the library copies
>>> the string to mount(2) or fsconfig(2) syscall. The library has no clue
>>> what the string means (it's the same as "mount -o foobar").
>> Which is what the problem really is.
>>
>>
>> If anyone uses this option with a file system that has previously
>>
>> allowed it then mounts fail if it isn't handled properly. Then the
>>
>> intended purpose of it is irrelevant because it causes a fail.
>>
>>
>> I guess the notion of ignoring it for fsconfig(), assuming it isn't
>>
>> actually needed for the option handling, might not be a viable idea
>>
>> ... although I haven't actually added that to fsconfig(), I probably
>>
>> should add that to this series.
>>
>>
>> But first the question of whether the option is actually needed anymore
>>
>> by those that allow it needs to be answered.
>>
>>
>> In case anyone has forgotten it was introduced because, at one time
>>
>> different OSes supported slightly different options for for the same
>>
>> thing and one could not include multiple options for the same thing
>>
>> in automount map entries without causing the mount to fail.
>>
>>
>> So we also need to answer, is this option conflict still present for
>>
>> any of the file systems that allow it, currently nfs, cifs and ntfs
>>
>> (I'll need to look up the ntfs maintainer but lets answer this for
>>
>> nfs and cifs first).
>>
>>
>> If it isn't actually needed ignoring it in fsconfig() (a deprecation
>>
>> warning would be in order) and eventually getting rid of it would be
>>
>> a good idea, yes?
> Yes, I think this is a good idea.
> The whole reason for this mount option seems a bit hacky tbh so getting
> rid of it would be great.

Thanks for thinking about this Christian.

It is something that has concerned me for a long time now.


I know the impression that people get is that it's hacky and it's

accurate to an extent but there was real need and value for it at

one point (although it was around before my time).


But now we get tripped up because trying to get rid of it causes

the problem of the option itself not working which tends to obscure

the actual use case of users.


I think the change to use the mount API is the best opportunity we've

had to clean this up in forever, particularly since the mount API

makes it particularly hard to continue to use it.


I'm still thinking about it and I'll post an updated patch and

accompanying discussion at some point. At the very least we need a

clear upstream position on it to allow those of us with customers

that think they need it to pass on the deprecation notice and reasoning.


It might end up we have to revisit it but at least if that's the case

we should have more detailed use cases that are current.


Ian

Re: [RFC PATCH] Legacy mount option "sloppy" support

[Christian Brauner]

On Tue, Apr 04, 2023 at 09:52:27AM +0800, Ian Kent wrote:
> On 3/4/23 21:08, Christian Brauner wrote:
> > On Wed, Mar 29, 2023 at 09:03:51AM +0800, Ian Kent wrote:
> > > On 29/3/23 02:48, Karel Zak wrote:
> > > > On Fri, Mar 24, 2023 at 01:39:09PM +0800, Ian Kent wrote:
> > > > > Karel do you find what I'm saying is accurate?
> > > > > Do you think we will be able to get rid of the sloppy option over
> > > > > time with the move to use the mount API?
> > > > The question is what we're talking about :-)
> > > > 
> > > > For mount(8) and libmount, there is nothing like the "sloppy" mount option.
> > > > 
> > > > If you use it in your fstab or as "mount -o sloppy" on the command line,
> > > > then it's used as any other fs-specific mount option; the library copies
> > > > the string to mount(2) or fsconfig(2) syscall. The library has no clue
> > > > what the string means (it's the same as "mount -o foobar").
> > > Which is what the problem really is.
> > > 
> > > 
> > > If anyone uses this option with a file system that has previously
> > > 
> > > allowed it then mounts fail if it isn't handled properly. Then the
> > > 
> > > intended purpose of it is irrelevant because it causes a fail.
> > > 
> > > 
> > > I guess the notion of ignoring it for fsconfig(), assuming it isn't
> > > 
> > > actually needed for the option handling, might not be a viable idea
> > > 
> > > ... although I haven't actually added that to fsconfig(), I probably
> > > 
> > > should add that to this series.
> > > 
> > > 
> > > But first the question of whether the option is actually needed anymore
> > > 
> > > by those that allow it needs to be answered.
> > > 
> > > 
> > > In case anyone has forgotten it was introduced because, at one time
> > > 
> > > different OSes supported slightly different options for for the same
> > > 
> > > thing and one could not include multiple options for the same thing
> > > 
> > > in automount map entries without causing the mount to fail.
> > > 
> > > 
> > > So we also need to answer, is this option conflict still present for
> > > 
> > > any of the file systems that allow it, currently nfs, cifs and ntfs
> > > 
> > > (I'll need to look up the ntfs maintainer but lets answer this for
> > > 
> > > nfs and cifs first).
> > > 
> > > 
> > > If it isn't actually needed ignoring it in fsconfig() (a deprecation
> > > 
> > > warning would be in order) and eventually getting rid of it would be
> > > 
> > > a good idea, yes?
> > Yes, I think this is a good idea.
> > The whole reason for this mount option seems a bit hacky tbh so getting
> > rid of it would be great.
> 
> Thanks for thinking about this Christian.
> 
> It is something that has concerned me for a long time now.
> 
> 
> I know the impression that people get is that it's hacky and it's
> 
> accurate to an extent but there was real need and value for it at
> 
> one point (although it was around before my time).

No, I get it and I understand that there was a rationale behind it.

> 
> 
> But now we get tripped up because trying to get rid of it causes
> 
> the problem of the option itself not working which tends to obscure
> 
> the actual use case of users.

Yeah, agreed.

> 
> 
> I think the change to use the mount API is the best opportunity we've
> 
> had to clean this up in forever, particularly since the mount API
> 
> makes it particularly hard to continue to use it.

Agreed.

> 
> 
> I'm still thinking about it and I'll post an updated patch and
> 
> accompanying discussion at some point. At the very least we need a
> 
> clear upstream position on it to allow those of us with customers
> 
> that think they need it to pass on the deprecation notice and reasoning.

I think that "sloppy" is exactly what it is "sloppy" so if we can purge
it from the new mount api then we should do it.