[PATCH v2 0/2] copy_mount_options improvements

[PATCH v2 0/2] copy_mount_options improvements

[Andy Lutomirski]

Patch 1 fixes what is arguably a bug.  I suspect it could cause
crashes or very large latency under unusual circumstanes.  Since it
also deletes a bunch of code, I think it's a nice fix.

Patch 2 is an ABI change/fix.  Any user code that would be affected
is currently buggy (i.e. results in incorrect option parsing), so I
suspect it can be applied safely.

Changes from v1:
 - Fix some blatant memory corruption :(

Andy Lutomirski (2):
  fs: Improve and simplify copy_mount_options
  fs: Disallow mount options strings longer than PAGE_SIZE - 1

 fs/namespace.c | 62 ++++++++++++++--------------------------------------------
 1 file changed, 15 insertions(+), 47 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Andy Lutomirski]

copy_mount_options always tries to copy a full page even if the
string is shorter than a page.  If the string starts part-way into a
page and ends on the same page it started on, this means that
copy_mount_options can overrun the supplied buffer and read into the
next page.

If the buffer came from userspace (USER_DS), then this could be a
performance issue (reading across the page boundary could block).
If the buffer came from the kernel (KERNEL_DS), then this could read
an unrelated page, and the kernel can have pages mapped in that have
side-effects.

I noticed this due to a new sanity-check I'm working on that tries
to make sure that we don't try to access nonexistent pages under
KERNEL_DS.

This is the same issue that was fixed by commit eca6f534e619 ("fs:
fix overflow in sys_mount() for in-kernel calls"), but for
copy_mount_options instead of copy_mount_string.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 fs/namespace.c | 56 ++++++++++++--------------------------------------------
 1 file changed, 12 insertions(+), 44 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691b4355..8644f1961ca6 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2581,38 +2581,13 @@ static void shrink_submounts(struct mount *mnt)
 	}
 }
 
-/*
- * Some copy_from_user() implementations do not return the exact number of
- * bytes remaining to copy on a fault.  But copy_mount_options() requires that.
- * Note that this function differs from copy_from_user() in that it will oops
- * on bad values of `to', rather than returning a short copy.
+/* Copy the mount options string.  Always returns a full page padded
+ * with nulls.  If the input string is a full page or more, it may be
+ * truncated and the result will not be null-terminated.
  */
-static long exact_copy_from_user(void *to, const void __user * from,
-				 unsigned long n)
+void *copy_mount_options(const void __user *data)
 {
-	char *t = to;
-	const char __user *f = from;
-	char c;
-
-	if (!access_ok(VERIFY_READ, from, n))
-		return n;
-
-	while (n) {
-		if (__get_user(c, f)) {
-			memset(t, 0, n);
-			break;
-		}
-		*t++ = c;
-		f++;
-		n--;
-	}
-	return n;
-}
-
-void *copy_mount_options(const void __user * data)
-{
-	int i;
-	unsigned long size;
+	long size;
 	char *copy;
 
 	if (!data)
@@ -2622,22 +2597,15 @@ void *copy_mount_options(const void __user * data)
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
 
-	/* We only care that *some* data at the address the user
-	 * gave us is valid.  Just in case, we'll zero
-	 * the remainder of the page.
-	 */
-	/* copy_from_user cannot cross TASK_SIZE ! */
-	size = TASK_SIZE - (unsigned long)data;
-	if (size > PAGE_SIZE)
-		size = PAGE_SIZE;
-
-	i = size - exact_copy_from_user(copy, data, size);
-	if (!i) {
+	size = strncpy_from_user(copy, data, PAGE_SIZE);
+	if (size < 0) {
 		kfree(copy);
-		return ERR_PTR(-EFAULT);
+		return ERR_PTR(size);
 	}
-	if (i != PAGE_SIZE)
-		memset(copy + i, 0, PAGE_SIZE - i);
+
+	/* If we got less than PAGE_SIZE bytes, zero out the remainder. */
+	memset(copy + size, 0, PAGE_SIZE - size);
+
 	return copy;
 }
 
-- 
2.7.4

[PATCH v2 2/2] fs: Disallow mount options strings longer than PAGE_SIZE - 1

[Andy Lutomirski]

We used to truncate the string.  Make the behaviour of mount() more
predictable: return -EINVAL if the string is too long.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 fs/namespace.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 8644f1961ca6..96e6b09df7d1 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2582,8 +2582,7 @@ static void shrink_submounts(struct mount *mnt)
 }
 
 /* Copy the mount options string.  Always returns a full page padded
- * with nulls.  If the input string is a full page or more, it may be
- * truncated and the result will not be null-terminated.
+ * with nulls and guarantees that the result is null-terminated.
  */
 void *copy_mount_options(const void __user *data)
 {
@@ -2603,7 +2602,12 @@ void *copy_mount_options(const void __user *data)
 		return ERR_PTR(size);
 	}
 
-	/* If we got less than PAGE_SIZE bytes, zero out the remainder. */
+	if (size >= PAGE_SIZE) {
+		kfree(copy);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* Pad with zeros. */
 	memset(copy + size, 0, PAGE_SIZE - size);
 
 	return copy;
@@ -2639,10 +2643,6 @@ long do_mount(const char *dev_name, const char __user *dir_name,
 	if ((flags & MS_MGC_MSK) == MS_MGC_VAL)
 		flags &= ~MS_MGC_MSK;
 
-	/* Basic sanity checks */
-	if (data_page)
-		((char *)data_page)[PAGE_SIZE - 1] = 0;
-
 	/* ... and get the mountpoint */
 	retval = user_path(dir_name, &path);
 	if (retval)
-- 
2.7.4

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Al Viro]

On Mon, Jun 13, 2016 at 07:36:04PM -0700, Andy Lutomirski wrote:
> copy_mount_options always tries to copy a full page even if the
> string is shorter than a page.  If the string starts part-way into a
> page and ends on the same page it started on, this means that
> copy_mount_options can overrun the supplied buffer and read into the
> next page.

Have you considered the possibility that there might be a reason for
having separate copy_mount_option() and copy_mount_string()?  Such as
options not being a string, perhaps?

In some filesystems (including older NFS variants) it is not a string
at all - a binary data structure, with quite a few zero bytes in it.
And no, we fucking *can't* break mount.nfs(8), no matter how we'd like
to get rid of that wart of an ABI.

IOW, NAK with prejudice - don't bring that thing back, it's hard no-go.

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Andy Lutomirski]

On Wed, Jun 15, 2016 at 4:50 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Jun 13, 2016 at 07:36:04PM -0700, Andy Lutomirski wrote:
>> copy_mount_options always tries to copy a full page even if the
>> string is shorter than a page.  If the string starts part-way into a
>> page and ends on the same page it started on, this means that
>> copy_mount_options can overrun the supplied buffer and read into the
>> next page.
>
> Have you considered the possibility that there might be a reason for
> having separate copy_mount_option() and copy_mount_string()?  Such as
> options not being a string, perhaps?
>
> In some filesystems (including older NFS variants) it is not a string
> at all - a binary data structure, with quite a few zero bytes in it.
> And no, we fucking *can't* break mount.nfs(8), no matter how we'd like
> to get rid of that wart of an ABI.
>
> IOW, NAK with prejudice - don't bring that thing back, it's hard no-go.

Well, that sucks.  I suppose we could make it conditional on the fs
type being "nfs", but yuck.

If we don't fix this, though, then we have other problems:

devtmpfsd does:

        *err = sys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, options);

where options points to the kernel stack.  This is bad.  do_mount_root
is similarly broken.

Is there any reason that these things use sys_mount instead of do_mount?

--Andy

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Linus Torvalds]

On Wed, Jun 15, 2016 at 2:01 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> devtmpfsd does:
>
>         *err = sys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, options);
>
> where options points to the kernel stack.  This is bad.  do_mount_root
> is similarly broken.
>
> Is there any reason that these things use sys_mount instead of do_mount?

Not that I can see. But maybe copy_mount_options could also check for
KERNEL_DS, and use a strncpy instead of a copy_from_user() for that
case?

             Linus

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Willy Tarreau]

On Wed, Jun 15, 2016 at 02:42:33PM -1000, Linus Torvalds wrote:
> On Wed, Jun 15, 2016 at 2:01 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > devtmpfsd does:
> >
> >         *err = sys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, options);
> >
> > where options points to the kernel stack.  This is bad.  do_mount_root
> > is similarly broken.
> >
> > Is there any reason that these things use sys_mount instead of do_mount?
> 
> Not that I can see. But maybe copy_mount_options could also check for
> KERNEL_DS, and use a strncpy instead of a copy_from_user() for that
> case?

Well, strncpy() would make the function behave differently depending on
the FS being used if called from the kernel for the reason Al mentionned.
OK devtmpfsd() passes a string, but if it's the FS itself which decides
to stop on a zero when parsing mount options, we'd probably rather use
memcpy() instead to ensure a consistent behaviour, like this maybe ?

Willy

diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691..058b856 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2622,6 +2622,12 @@ void *copy_mount_options(const void __user * data)
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
 
+	/* do_mount() may be called from the kernel */
+	if (segment_eq(get_fs(), KERNEL_DS)) {
+		memcpy(copy, data, PAGE_SIZE);
+		return copy;
+	}
+
 	/* We only care that *some* data at the address the user
 	 * gave us is valid.  Just in case, we'll zero
 	 * the remainder of the page.

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Linus Torvalds]

On Wed, Jun 15, 2016 at 7:45 PM, Willy Tarreau <w@1wt.eu> wrote:
>
> Well, strncpy() would make the function behave differently depending on
> the FS being used if called from the kernel for the reason Al mentionned.
> OK devtmpfsd() passes a string, but if it's the FS itself which decides
> to stop on a zero when parsing mount options, we'd probably rather use
> memcpy() instead to ensure a consistent behaviour, like this maybe ?

.. but that is exactly what Andy considers to be a problem: now it
copies random kernel memory that is possibly security-critical.

The kernel users that use this just pass in a string - it doesn't
matter what the filesystem thinks it is getting, the uses were all
kernel strings,, so the "copy_mount_options": should copy that string
(and zero-fill the page that the filesystem may think it is getting).

              Linus

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Willy Tarreau]

On Wed, Jun 15, 2016 at 07:59:00PM -1000, Linus Torvalds wrote:
> On Wed, Jun 15, 2016 at 7:45 PM, Willy Tarreau <w@1wt.eu> wrote:
> >
> > Well, strncpy() would make the function behave differently depending on
> > the FS being used if called from the kernel for the reason Al mentionned.
> > OK devtmpfsd() passes a string, but if it's the FS itself which decides
> > to stop on a zero when parsing mount options, we'd probably rather use
> > memcpy() instead to ensure a consistent behaviour, like this maybe ?
> 
> .. but that is exactly what Andy considers to be a problem: now it
> copies random kernel memory that is possibly security-critical.
> 
> The kernel users that use this just pass in a string - it doesn't
> matter what the filesystem thinks it is getting, the uses were all
> kernel strings,, so the "copy_mount_options": should copy that string
> (and zero-fill the page that the filesystem may think it is getting).

But I still find it ugly to consider that if the options come from the
kernel they're a zero-terminated string otherwise they're a page :-/
Couldn't we instead look up the fstype before calling copy_mount_options() ?
>From what I'm seeing, we already have FS_BINARY_MOUNTDATA in the FS type
to indicate that it expects binary mount options, so probably we could
check it in copy_mount_options() if we pass it the fstype. Something
approximately like this (not even build tested).

Willy

diff --git a/fs/compat.c b/fs/compat.c
index be6e48b..8494766 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -806,7 +806,7 @@ COMPAT_SYSCALL_DEFINE5(mount, const char __user *, dev_name,
 	if (IS_ERR(kernel_dev))
 		goto out1;
 
-	options = copy_mount_options(data);
+	options = copy_mount_options(type, data);
 	retval = PTR_ERR(options);
 	if (IS_ERR(options))
 		goto out2;
diff --git a/fs/internal.h b/fs/internal.h
index b71deee..3c9bc7b 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -55,7 +55,7 @@ extern int vfs_path_lookup(struct dentry *, struct vfsmount *,
 /*
  * namespace.c
  */
-extern void *copy_mount_options(const void __user *);
+extern void *copy_mount_options(const char __user *, const void __user *);
 extern char *copy_mount_string(const void __user *);
 
 extern struct vfsmount *lookup_mnt(struct path *);
diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691..cf28d08 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2609,19 +2609,30 @@ static long exact_copy_from_user(void *to, const void __user * from,
 	return n;
 }
 
-void *copy_mount_options(const void __user * data)
+void *copy_mount_options(const void __user *fstype, const void __user * data)
 {
 	int i;
 	unsigned long size;
 	char *copy;
+	struct file_system_type *type;
 
 	if (!data)
 		return NULL;
 
+	type = get_fs_type(fstype);
+	if (!type)
+		return NULL;
+
 	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
 
+	/* avoid reading a whole page if the FS only needs a string. */
+	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
+		strlcpy(copy, data, PAGE_SIZE);
+		return copy;
+	}
+
 	/* We only care that *some* data at the address the user
 	 * gave us is valid.  Just in case, we'll zero
 	 * the remainder of the page.
@@ -2917,7 +2928,7 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
 	if (IS_ERR(kernel_dev))
 		goto out_dev;
 
-	options = copy_mount_options(data);
+	options = copy_mount_options(type, data);
 	ret = PTR_ERR(options);
 	if (IS_ERR(options))
 		goto out_data;

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Willy Tarreau]

On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
(...)
> +	/* avoid reading a whole page if the FS only needs a string. */
> +	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
> +		strlcpy(copy, data, PAGE_SIZE);

BTW, I forgot that we're first supposed to come from user, make that
strndup_user() instead.

Willy

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Al Viro]

On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
> +	type = get_fs_type(fstype);
> +	if (!type)
> +		return NULL;
> +
>  	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
>  	if (!copy)
>  		return ERR_PTR(-ENOMEM);
>  
> +	/* avoid reading a whole page if the FS only needs a string. */
> +	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
> +		strlcpy(copy, data, PAGE_SIZE);
> +		return copy;

a) it leaks a file_system_type reference
b) data is a userland pointer, for crying out loud!

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Willy Tarreau]

On Thu, Jun 16, 2016 at 08:08:22AM +0100, Al Viro wrote:
> On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
> > +	type = get_fs_type(fstype);
> > +	if (!type)
> > +		return NULL;
> > +
> >  	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
> >  	if (!copy)
> >  		return ERR_PTR(-ENOMEM);
> >  
> > +	/* avoid reading a whole page if the FS only needs a string. */
> > +	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
> > +		strlcpy(copy, data, PAGE_SIZE);
> > +		return copy;
> 
> a) it leaks a file_system_type reference

I was not sure about this one, thanks for confirming.

> b) data is a userland pointer, for crying out loud!

Yep I noticed it and fixed it after sending. I was focused on the
data coming from kernel due to the discussion.

I also think that since there are only two call places for
copy_mount_options(), we may move the test there and switch
to copy_mount_string() instead depending on the fs type.

Thanks,
Willy

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Al Viro]

On Thu, Jun 16, 2016 at 09:25:57AM +0200, Willy Tarreau wrote:
> On Thu, Jun 16, 2016 at 08:08:22AM +0100, Al Viro wrote:
> > On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
> > > +	type = get_fs_type(fstype);
> > > +	if (!type)
> > > +		return NULL;
> > > +
> > >  	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > >  	if (!copy)
> > >  		return ERR_PTR(-ENOMEM);
> > >  
> > > +	/* avoid reading a whole page if the FS only needs a string. */
> > > +	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
> > > +		strlcpy(copy, data, PAGE_SIZE);
> > > +		return copy;
> > 
> > a) it leaks a file_system_type reference
> 
> I was not sure about this one, thanks for confirming.
> 
> > b) data is a userland pointer, for crying out loud!
> 
> Yep I noticed it and fixed it after sending. I was focused on the
> data coming from kernel due to the discussion.
> 
> I also think that since there are only two call places for
> copy_mount_options(), we may move the test there and switch
> to copy_mount_string() instead depending on the fs type.

Another problem is that it will oops with NULL fstype, which is
absolutely normal both for mount --bind *and* mount -o remount.
And while mount --bind doesn't care about string options,
mount -o remount certainly does.  IMO the latter makes that
approach hopeless - with remount you don't know the type
until well into do_mount() guts and I'd really hate to carry
the userland pointer all the way into it.

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Willy Tarreau]

On Thu, Jun 16, 2016 at 09:02:29AM +0100, Al Viro wrote:
> On Thu, Jun 16, 2016 at 09:25:57AM +0200, Willy Tarreau wrote:
> > On Thu, Jun 16, 2016 at 08:08:22AM +0100, Al Viro wrote:
> > > On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
> > > > +	type = get_fs_type(fstype);
> > > > +	if (!type)
> > > > +		return NULL;
> > > > +
> > > >  	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > > >  	if (!copy)
> > > >  		return ERR_PTR(-ENOMEM);
> > > >  
> > > > +	/* avoid reading a whole page if the FS only needs a string. */
> > > > +	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
> > > > +		strlcpy(copy, data, PAGE_SIZE);
> > > > +		return copy;
> > > 
> > > a) it leaks a file_system_type reference
> > 
> > I was not sure about this one, thanks for confirming.
> > 
> > > b) data is a userland pointer, for crying out loud!
> > 
> > Yep I noticed it and fixed it after sending. I was focused on the
> > data coming from kernel due to the discussion.
> > 
> > I also think that since there are only two call places for
> > copy_mount_options(), we may move the test there and switch
> > to copy_mount_string() instead depending on the fs type.
> 
> Another problem is that it will oops with NULL fstype, which is
> absolutely normal both for mount --bind *and* mount -o remount.

OK thanks for explaining, I didn't know.

> And while mount --bind doesn't care about string options,
> mount -o remount certainly does.  IMO the latter makes that
> approach hopeless - with remount you don't know the type
> until well into do_mount() guts and I'd really hate to carry
> the userland pointer all the way into it.

Agreed. However if the initial point was to avoid reading extra
pages most of the time, we could possibly consider that the string
copy is an optimization for the case where we have the information
available. Thus if !fstype || !(type->fs_flags & FS_BINARY_MOUNTDATA)
then we use copy_mount_options() otherwise we use copy_mount_string().
It will only leave the full page copy for mount --bind or -o remount
then.

Willy

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Willy Tarreau]

On Thu, Jun 16, 2016 at 10:20:38AM +0200, Willy Tarreau wrote:
> However if the initial point was to avoid reading extra
> pages most of the time, we could possibly consider that the string
> copy is an optimization for the case where we have the information
> available. Thus if !fstype || !(type->fs_flags & FS_BINARY_MOUNTDATA)
> then we use copy_mount_options() otherwise we use copy_mount_string().
> It will only leave the full page copy for mount --bind or -o remount
> then.

For example like this (not compile-tested either) :

diff --git a/fs/compat.c b/fs/compat.c
index be6e48b..6407d40 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -795,18 +795,26 @@ COMPAT_SYSCALL_DEFINE5(mount, const char __user *, dev_name,
 	void *options;
 	char *kernel_dev;
 	int retval;
+	bool use_str = false;
+	struct file_system_type *fstype;
 
 	kernel_type = copy_mount_string(type);
 	retval = PTR_ERR(kernel_type);
 	if (IS_ERR(kernel_type))
 		goto out;
 
+	if (kernel_type && (fstype = get_fs_type(kernel_type)) {
+		if (!(fstype->fs_flags & FS_BINARY_MOUNTDATA))
+			use_str = true;
+		put_filesystem(fstype);
+	}
+
 	kernel_dev = copy_mount_string(dev_name);
 	retval = PTR_ERR(kernel_dev);
 	if (IS_ERR(kernel_dev))
 		goto out1;
 
-	options = copy_mount_options(data);
+	options = use_str ? copy_mount_string(data) : copy_mount_options(data);
 	retval = PTR_ERR(options);
 	if (IS_ERR(options))
 		goto out2;
diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691..f14089d 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2906,18 +2906,26 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
 	char *kernel_type;
 	char *kernel_dev;
 	void *options;
+	bool use_str = false;
+	struct file_system_type *fstype;
 
 	kernel_type = copy_mount_string(type);
 	ret = PTR_ERR(kernel_type);
 	if (IS_ERR(kernel_type))
 		goto out_type;
 
+	if (kernel_type && (fstype = get_fs_type(kernel_type)) {
+		if (!(fstype->fs_flags & FS_BINARY_MOUNTDATA))
+			use_str = true;
+		put_filesystem(fstype);
+	}
+
 	kernel_dev = copy_mount_string(dev_name);
 	ret = PTR_ERR(kernel_dev);
 	if (IS_ERR(kernel_dev))
 		goto out_dev;
 
-	options = copy_mount_options(data);
+	options = use_str ? copy_mount_string(data) : copy_mount_options(data);
 	ret = PTR_ERR(options);
 	if (IS_ERR(options))
 		goto out_data;

Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Andy Lutomirski]

On Thu, Jun 16, 2016 at 1:38 AM, Willy Tarreau <w@1wt.eu> wrote:
> On Thu, Jun 16, 2016 at 10:20:38AM +0200, Willy Tarreau wrote:
>> However if the initial point was to avoid reading extra
>> pages most of the time, we could possibly consider that the string
>> copy is an optimization for the case where we have the information
>> available. Thus if !fstype || !(type->fs_flags & FS_BINARY_MOUNTDATA)
>> then we use copy_mount_options() otherwise we use copy_mount_string().
>> It will only leave the full page copy for mount --bind or -o remount
>> then.
>
> For example like this (not compile-tested either) :

I like this approach.  Al, is this okay with you?

--Andy

>
> diff --git a/fs/compat.c b/fs/compat.c
> index be6e48b..6407d40 100644
> --- a/fs/compat.c
> +++ b/fs/compat.c
> @@ -795,18 +795,26 @@ COMPAT_SYSCALL_DEFINE5(mount, const char __user *, dev_name,
>         void *options;
>         char *kernel_dev;
>         int retval;
> +       bool use_str = false;
> +       struct file_system_type *fstype;
>
>         kernel_type = copy_mount_string(type);
>         retval = PTR_ERR(kernel_type);
>         if (IS_ERR(kernel_type))
>                 goto out;
>
> +       if (kernel_type && (fstype = get_fs_type(kernel_type)) {
> +               if (!(fstype->fs_flags & FS_BINARY_MOUNTDATA))
> +                       use_str = true;
> +               put_filesystem(fstype);
> +       }
> +
>         kernel_dev = copy_mount_string(dev_name);
>         retval = PTR_ERR(kernel_dev);
>         if (IS_ERR(kernel_dev))
>                 goto out1;
>
> -       options = copy_mount_options(data);
> +       options = use_str ? copy_mount_string(data) : copy_mount_options(data);
>         retval = PTR_ERR(options);
>         if (IS_ERR(options))
>                 goto out2;
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 4fb1691..f14089d 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2906,18 +2906,26 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
>         char *kernel_type;
>         char *kernel_dev;
>         void *options;
> +       bool use_str = false;
> +       struct file_system_type *fstype;
>
>         kernel_type = copy_mount_string(type);
>         ret = PTR_ERR(kernel_type);
>         if (IS_ERR(kernel_type))
>                 goto out_type;
>
> +       if (kernel_type && (fstype = get_fs_type(kernel_type)) {
> +               if (!(fstype->fs_flags & FS_BINARY_MOUNTDATA))
> +                       use_str = true;
> +               put_filesystem(fstype);
> +       }
> +
>         kernel_dev = copy_mount_string(dev_name);
>         ret = PTR_ERR(kernel_dev);
>         if (IS_ERR(kernel_dev))
>                 goto out_dev;
>
> -       options = copy_mount_options(data);
> +       options = use_str ? copy_mount_string(data) : copy_mount_options(data);
>         ret = PTR_ERR(options);
>         if (IS_ERR(options))
>                 goto out_data;



-- 
Andy Lutomirski
AMA Capital Management, LLC