Hi,

It was reported[1] that 32 bit readv and writev AIO operations were
not functioning properly.  It turns out that the code to convert the
32bit io vectors to 64 bits was never written.  The results of that
can be pretty bad, but in my testing, it mostly ended up in generating
EFAULT as we walked off the list of I/O vectors provided.

This patch set fixes the problem in my environment.  Comments, as always,
are greatly appreciated.

Cheers,
Jeff

[1] http://lkml.org/lkml/2010/3/8/309

[PATCH 1/2] compat: factor out compat_rw_copy_check_uvector from compat_do_readv_writev
[PATCH 2/2] aio: fix the compat vectored operations

--

[ message continues ]

This patch factors out code that will be used by both compat_do_readv_writev
and the compat aio submission code paths.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
 fs/compat.c            |  130 ++++++++++++++++++++++++++++-------------------
 include/linux/compat.h |    4 ++
 2 files changed, 81 insertions(+), 53 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index 4b6ed03..ae0066d 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -568,6 +568,79 @@ out:
 	return ret;
 }
 
+/* A write operation does a read from user space and vice versa */
+#define vrfy_dir(type) ((type) == READ ? VERIFY_WRITE : VERIFY_READ)
+
+ssize_t compat_rw_copy_check_uvector(int type,
+		const struct compat_iovec __user *uvector, unsigned long nr_segs,
+		unsigned long fast_segs, struct iovec *fast_pointer,
+		struct iovec **ret_pointer)
+{
+	compat_ssize_t tot_len;
+	struct iovec *iov = *ret_pointer = fast_pointer;
+	ssize_t ret = 0;
+	int seg;
+
+	/*
+	 * SuS says "The readv() function *may* fail if the iovcnt argument
+	 * was less than or equal to 0, or greater than {IOV_MAX}.  Linux has
+	 * traditionally returned zero for zero segments, so...
+	 */
+	if (nr_segs == 0)
+		goto out;
+
+	ret = -EINVAL;
+	if (nr_segs > UIO_MAXIOV || nr_segs < 0)
+		goto out;
+	if (nr_segs > fast_segs) {
+		ret = -ENOMEM;
+		iov = kmalloc(nr_segs*sizeof(struct iovec), GFP_KERNEL);
+		if (iov == NULL) {
+			*ret_pointer = fast_pointer;
+			goto out;
+		}
+	}
+	*ret_pointer = iov;
+
+	/*
+	 * Single unix specification:
+	 * We should -EINVAL if an element length is not >= 0 and fitting an
+	 * ssize_t.  The total length is fitting an ssize_t
+	 *
+	 * Be careful here because iov_len is a size_t not an ssize_t
+	 */
+	tot_len = 0;
+	ret = -EINVAL;
+	for (seg = 0; seg < nr_segs; seg++) {
+		compat_ssize_t tmp = tot_len;
+		compat_uptr_t buf;
+		compat_ssize_t len;
+
+		if (__get_user(len, &uvector->iov_len) ||
+		   __get_user(buf, &uvector->iov_base)) {
+			ret = -EFAULT;
+			goto ...
[ message continues ]

The aio compat code was not converting the struct iovecs from 32bit to
64bit pointers, causing either EINVAL to be returned from io_getevents,
or EFAULT as the result of the I/O.  This patch passes a compat flag to
io_submit to signal that pointer conversion is necessary for a given iocb
array.

A variant of this was tested by Michael Tokarev.  I have also updated
the libaio test harness to exercise this code path with good success.
Further, I grabbed a copy of ltp and ran the testcases/kernel/syscall/readv
and writev tests there (compiled with -m32 on my 64bit system).  All
seems happy, but extra eyes on this would be welcome.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
 fs/aio.c            |   63 +++++++++++++++++++++++++++++++-------------------
 fs/compat.c         |    2 +-
 include/linux/aio.h |    5 ++++
 3 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 1cf12b3..5a38805 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -36,6 +36,7 @@
 #include <linux/blkdev.h>
 #include <linux/mempool.h>
 #include <linux/hash.h>
+#include <linux/compat.h>
 
 #include <asm/kmap_types.h>
 #include <asm/uaccess.h>
@@ -1384,13 +1385,20 @@ static ssize_t aio_fsync(struct kiocb *iocb)
 	return ret;
 }
 
-static ssize_t aio_setup_vectored_rw(int type, struct kiocb *kiocb)
+static ssize_t aio_setup_vectored_rw(int type, struct kiocb *kiocb, bool compat)
 {
 	ssize_t ret;
 
-	ret = rw_copy_check_uvector(type, (struct iovec __user *)kiocb->ki_buf,
-				    kiocb->ki_nbytes, 1,
-				    &kiocb->ki_inline_vec, &kiocb->ki_iovec);
+	if (compat)
+		ret = compat_rw_copy_check_uvector(type,
+				(struct compat_iovec __user *)kiocb->ki_buf,
+				kiocb->ki_nbytes, 1, &kiocb->ki_inline_vec,
+				&kiocb->ki_iovec);
+	else
+		ret = rw_copy_check_uvector(type,
+				(struct iovec __user *)kiocb->ki_buf,
+				kiocb->ki_nbytes, 1, &kiocb->ki_inline_vec,
+				&kiocb->ki_iovec);
 	if (ret < 0)
 		goto out;
 
@@ -1420,7 +1428,7 @@ static ssize_t ...
[ message continues ]

On Fri, 30 Apr 2010 16:48:58 -0400

The patches are large(ish) and we're at -rc6.  I'm inclined to merge
these into 2.6.35-rc1 with a -stable tag so they get backported into 2.6.34.1
after a bit of mainline testing, OK?

--

[ message continues ]

That's fine with me.

Cheers,
Jeff
--

[ message continues ]