Re: [PATCH] vfs: fix vmplice_to_user()

Re: [PATCH] vfs: fix vmplice_to_user()

[George Spelvin]

You could also get rid of the separate ret/count variables
and use ssize_t everywhere; that's the declared return type
of rw_copy_check_uvector and __splice_from_pipe, after all.

Hunks after the first two are optional extras to make types
consistent in other functions in that file, although I stopped
once I realized how big a job it owuld be to do it all.

Signed-off-by: George Spelvin <linux@horizon.com>

diff --git a/fs/splice.c b/fs/splice.c
index 6b111500..2fce8f44 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1534,18 +1534,17 @@ static long vmsplice_to_user(struct file *file, const struct iovec __user *uiov,
 	struct pipe_inode_info *pipe;
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
-	long ret;
+	ssize_t count;
 
 	pipe = get_pipe_info(file);
 	if (!pipe)
 		return -EBADF;
 
-	ret = rw_copy_check_uvector(READ, uiov, nr_segs,
+	count = rw_copy_check_uvector(READ, uiov, nr_segs,
 				    ARRAY_SIZE(iovstack), iovstack, &iov);
-	if (ret > 0) {
+	if (count > 0) {
 		struct splice_desc sd;
 		struct iov_iter iter;
-		ssize_t count = ret;
 
 		iov_iter_init(&iter, iov, nr_segs, count, 0);
 
@@ -1556,14 +1555,14 @@ static long vmsplice_to_user(struct file *file, const struct iovec __user *uiov,
 		sd.pos = 0;
 
 		pipe_lock(pipe);
-		ret = __splice_from_pipe(pipe, &sd, pipe_to_user);
+		count = __splice_from_pipe(pipe, &sd, pipe_to_user);
 		pipe_unlock(pipe);
 	}
 
 	if (iov != iovstack)
 		kfree(iov);
 
-	return ret;
+	return count;
 }
 
 /*
@@ -1571,7 +1570,7 @@ static long vmsplice_to_user(struct file *file, const struct iovec __user *uiov,
  * as splice-from-memory, where the regular splice is splice-from-file (or
  * to file). In both cases the output is a pipe, naturally.
  */
-static long vmsplice_to_pipe(struct file *file, const struct iovec __user *iov,
+static ssize_t vmsplice_to_pipe(struct file *file, const struct iovec __user *iov,
 			     unsigned long nr_segs, unsigned int flags)
 {
 	struct pipe_inode_info *pipe;
@@ -1585,7 +1584,7 @@ static long vmsplice_to_pipe(struct file *file, const struct iovec __user *iov,
 		.ops = &user_page_pipe_buf_ops,
 		.spd_release = spd_release_page,
 	};
-	long ret;
+	ssize_t ret;
 
 	pipe = get_pipe_info(file);
 	if (!pipe)
@@ -1626,25 +1625,25 @@ SYSCALL_DEFINE4(vmsplice, int, fd, const struct iovec __user *, iov,
 		unsigned long, nr_segs, unsigned int, flags)
 {
 	struct fd f;
-	long error;
+	ssize_t ret;
 
 	if (unlikely(nr_segs > UIO_MAXIOV))
 		return -EINVAL;
 	else if (unlikely(!nr_segs))
 		return 0;
 
-	error = -EBADF;
+	ret = -EBADF;
 	f = fdget(fd);
 	if (f.file) {
 		if (f.file->f_mode & FMODE_WRITE)
-			error = vmsplice_to_pipe(f.file, iov, nr_segs, flags);
+			ret = vmsplice_to_pipe(f.file, iov, nr_segs, flags);
 		else if (f.file->f_mode & FMODE_READ)
-			error = vmsplice_to_user(f.file, iov, nr_segs, flags);
+			ret = vmsplice_to_user(f.file, iov, nr_segs, flags);
 
 		fdput(f);
 	}
 
-	return error;
+	return ret;
 }
 
 #ifdef CONFIG_COMPAT

[PATCH] vfs: fix vmplice_to_user()

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

Commit 6130f5315ee8 "switch vmsplice_to_user() to copy_page_to_iter()" in
v3.15-rc1 broke vmsplice(2).

This patch fixes two bugs:

 - count is not initialized to a proper value, which resulted in no data
   being copied

 - if rw_copy_check_uvector() returns negative then the iov might be leaked.

Tested OK.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/splice.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1537,7 +1537,7 @@ static long vmsplice_to_user(struct file
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
 	struct iov_iter iter;
-	ssize_t count = 0;
+	ssize_t count;
 
 	pipe = get_pipe_info(file);
 	if (!pipe)
@@ -1546,8 +1546,9 @@ static long vmsplice_to_user(struct file
 	ret = rw_copy_check_uvector(READ, uiov, nr_segs,
 				    ARRAY_SIZE(iovstack), iovstack, &iov);
 	if (ret <= 0)
-		return ret;
+		goto out;
 
+	count = ret;
 	iov_iter_init(&iter, iov, nr_segs, count, 0);
 
 	sd.len = 0;
@@ -1560,6 +1561,7 @@ static long vmsplice_to_user(struct file
 	ret = __splice_from_pipe(pipe, &sd, pipe_to_user);
 	pipe_unlock(pipe);
 
+out:
 	if (iov != iovstack)
 		kfree(iov);
 

Re: [PATCH] vfs: fix vmplice_to_user()

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

On Tue, May 27, 2014 at 7:41 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> This patch fixes two bugs:

Mind if I change it to avoid the "goto"? I don't think goto is evil,
but in this case it doesn't seem to buy anything, except to perhaps
make the patch a bit smaller at the expense of making the result a bit
harder to read.

Replacing the

   if (ret <= 0)
      goto out;

with

   if (ret > 0) {
       ...
   }

also allows moving the various variable declarations closer to their use.

So something like the attached..

             Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1472 bytes --]

 fs/splice.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/splice.c b/fs/splice.c
index 9bc07d2b53cf..6b1115005150 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1532,12 +1532,9 @@ static long vmsplice_to_user(struct file *file, const struct iovec __user *uiov,
 			     unsigned long nr_segs, unsigned int flags)
 {
 	struct pipe_inode_info *pipe;
-	struct splice_desc sd;
-	long ret;
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
-	struct iov_iter iter;
-	ssize_t count = 0;
+	long ret;
 
 	pipe = get_pipe_info(file);
 	if (!pipe)
@@ -1545,20 +1542,23 @@ static long vmsplice_to_user(struct file *file, const struct iovec __user *uiov,
 
 	ret = rw_copy_check_uvector(READ, uiov, nr_segs,
 				    ARRAY_SIZE(iovstack), iovstack, &iov);
-	if (ret <= 0)
-		return ret;
+	if (ret > 0) {
+		struct splice_desc sd;
+		struct iov_iter iter;
+		ssize_t count = ret;
 
-	iov_iter_init(&iter, iov, nr_segs, count, 0);
+		iov_iter_init(&iter, iov, nr_segs, count, 0);
 
-	sd.len = 0;
-	sd.total_len = count;
-	sd.flags = flags;
-	sd.u.data = &iter;
-	sd.pos = 0;
+		sd.len = 0;
+		sd.total_len = count;
+		sd.flags = flags;
+		sd.u.data = &iter;
+		sd.pos = 0;
 
-	pipe_lock(pipe);
-	ret = __splice_from_pipe(pipe, &sd, pipe_to_user);
-	pipe_unlock(pipe);
+		pipe_lock(pipe);
+		ret = __splice_from_pipe(pipe, &sd, pipe_to_user);
+		pipe_unlock(pipe);
+	}
 
 	if (iov != iovstack)
 		kfree(iov);

Re: [PATCH] vfs: fix vmplice_to_user()

[Al Viro]

On Tue, May 27, 2014 at 10:04:58AM -0700, Linus Torvalds wrote:
> On Tue, May 27, 2014 at 7:41 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > This patch fixes two bugs:
> 
> Mind if I change it to avoid the "goto"? I don't think goto is evil,
> but in this case it doesn't seem to buy anything, except to perhaps
> make the patch a bit smaller at the expense of making the result a bit
> harder to read.
> 
> Replacing the
> 
>    if (ret <= 0)
>       goto out;
> 
> with
> 
>    if (ret > 0) {
>        ...
>    }
> 
> also allows moving the various variable declarations closer to their use.
> 
> So something like the attached..

I'm not sure it's better; I've a pending patch that does combination of
rw_copy_check_uvector() with iov_iter_init(), which means that iov
array will be going to the outer scope anyway...

Re: [PATCH] vfs: fix vmplice_to_user()

[Al Viro]

On Tue, May 27, 2014 at 04:41:16PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Commit 6130f5315ee8 "switch vmsplice_to_user() to copy_page_to_iter()" in
> v3.15-rc1 broke vmsplice(2).
> 
> This patch fixes two bugs:
> 
>  - count is not initialized to a proper value, which resulted in no data
>    being copied
> 
>  - if rw_copy_check_uvector() returns negative then the iov might be leaked.
> 
> Tested OK.

Applied, will push tonight.