aio: compat_ioctl issue?

aio: compat_ioctl issue?

[Michael Tokarev]

Hello.

I just come across a situation (next in a long row :)
when on x86, 32bit userspace does not work with 64bit
kernel.  This time this is about aio requests.

An application submits some aio job, and it is returned
immediately (from io_getevents()) with EINVAL error.
Here's what it does (it's a printf in the application -
actual arguments as passed to io_submit() and actual
result received in io_getevents()):

io_submit: lio_opcode=7 reqprio=0 iov=0x9cd7018{0xf5599000,4096}, niov=1, offset=0
io_getevents: expected 4096 got -22 (EINVAL)

Note that it is not io_submit() which fails (that one
returns success) but io_getevents(), so it has to be
down the pipeline somewhere.

Can someone comment this please?  Somehow I was thinking this
codepath works as - I think anyway - I ran 32bit Oracle database
with aio support on 64bit kernel, but I may be mistaken.

Kernel is 2.6.33 from kernel.org.

Thanks!

/mjt

Re: aio: compat_ioctl issue?

[Jeff Moyer]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Hello.
>
> I just come across a situation (next in a long row :)
> when on x86, 32bit userspace does not work with 64bit
> kernel.  This time this is about aio requests.
>
> An application submits some aio job, and it is returned
> immediately (from io_getevents()) with EINVAL error.
> Here's what it does (it's a printf in the application -
> actual arguments as passed to io_submit() and actual
> result received in io_getevents()):
>
> io_submit: lio_opcode=7 reqprio=0 iov=0x9cd7018{0xf5599000,4096}, niov=1, offset=0
> io_getevents: expected 4096 got -22 (EINVAL)
>
> Note that it is not io_submit() which fails (that one
> returns success) but io_getevents(), so it has to be
> down the pipeline somewhere.
>
> Can someone comment this please?  Somehow I was thinking this
> codepath works as - I think anyway - I ran 32bit Oracle database
> with aio support on 64bit kernel, but I may be mistaken.
>
> Kernel is 2.6.33 from kernel.org.

Can you post the program, please?

Thanks,
Jeff

Re: aio: compat_ioctl issue?

[Michael Tokarev]

Jeff Moyer wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> Hello.
>>
>> I just come across a situation (next in a long row :)
>> when on x86, 32bit userspace does not work with 64bit
>> kernel.  This time this is about aio requests.
>>
>> An application submits some aio job, and it is returned
>> immediately (from io_getevents()) with EINVAL error.
[]
> Can you post the program, please?

The program which I'm trying is quite big - it's qemu-kvm
v. 0.12.3 compiled with --enable-linux-aio.  I bugged
kvm folks about non-working aio support but immediately
realized it's 32/64bit issue in the kernel, since 64bit
kvm works just fine on the same kernel (which is 64bits).
So I added 2 printfs into its linux-aio.c and re-run it.

The diff against upstream qemu-kvm is this one:

---- cut -----
diff --git a/linux-aio.c b/linux-aio.c
index 5e892b0..e856873 100644
--- a/linux-aio.c
+++ b/linux-aio.c
@@ -60,6 +60,9 @@ static void qemu_laio_process_completion(struct qemu_laio_state *s,

     ret = laiocb->ret;
     if (ret != -ECANCELED) {
+if (ret != laiocb->nbytes)
+fprintf(stderr, "io_getevents: exp %d got %d %s\n", laiocb->nbytes, ret,
+strerror(ret));
         if (ret == laiocb->nbytes)
             ret = 0;
         else if (ret >= 0)
@@ -223,6 +226,12 @@ BlockDriverAIOCB *laio_submit(BlockDriverState *bs, void *aio_ctx, int fd,
     io_set_eventfd(&laiocb->iocb, s->efd);
     s->count++;

+{ struct iovec *v = (struct iovec*)iocbs->u.c.buf;
+printf("io_submit: lio_opcode=%d reqprio=%d iov=%p{%p,%d}, niov=%lu, offset=%Ld\n",
+iocbs->aio_lio_opcode, iocbs->aio_reqprio, iocbs->u.c.buf,
+v->iov_base, v->iov_len,
+iocbs->u.c.nbytes, iocbs->u.c.offset);
+}
     if (io_submit(s->ctx, 1, &iocbs) < 0)
         goto out_dec_count;
     return &laiocb->common;
---- cut -----

I'm not sure if it's useful or not.

Qemu-kvm git tree is at
git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git

I never dealt with aio before, so I'll need to familiarize
myself with it before trying to create a smaller testcase.

Thanks!

/mjt

Re: aio: compat_ioctl issue?

[Jeff Moyer]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Jeff Moyer wrote:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>> 
>>> Hello.
>>>
>>> I just come across a situation (next in a long row :)
>>> when on x86, 32bit userspace does not work with 64bit
>>> kernel.  This time this is about aio requests.
>>>
>>> An application submits some aio job, and it is returned
>>> immediately (from io_getevents()) with EINVAL error.
> []
>> Can you post the program, please?
>
> The program which I'm trying is quite big - it's qemu-kvm
> v. 0.12.3 compiled with --enable-linux-aio.  I bugged
> kvm folks about non-working aio support but immediately
> realized it's 32/64bit issue in the kernel, since 64bit
> kvm works just fine on the same kernel (which is 64bits).
> So I added 2 printfs into its linux-aio.c and re-run it.

[snip]

Well, I'm not experiencing such problems.  I can run a 32bit aio
application on a 64bit kernel just fine with 2.6.31, 2.6.32 and
2.6.34-rc1.

Could you maybe print out the values that are passed to io_getevents?

Thanks,
Jeff

Re: aio: compat_ioctl issue?

[Michael Tokarev]

Jeff Moyer wrote:
[]
>>>> I just come across a situation (next in a long row :)
>>>> when on x86, 32bit userspace does not work with 64bit
>>>> kernel.  This time this is about aio requests.
> [snip]

> Well, I'm not experiencing such problems.  I can run a 32bit aio
> application on a 64bit kernel just fine with 2.6.31, 2.6.32 and
> 2.6.34-rc1.

Interesting.  I mentioned that I _think_ I used Oracle 32bit with
AIO on 64bit kernel, but I'm not sure.

> Could you maybe print out the values that are passed to io_getevents?

They were in my first email, here it goes again:

io_submit: lio_opcode=7 reqprio=0 iov=0x9cd7018{0xf5599000,4096}, niov=1, offset=0
io_getevents: expected 4096 got -22 (EINVAL)

This is what gets passed to libaio -- strace here
does not decode the arguments unfortunately.

The file in question is raw partition open with O_DIRECT,
if that makes any difference.

64bit binary uses very similar arguments except that
addresses are different.

My *guess* is that it handles read/write correctly but
does not properly handle preadv/pwritev (opcode=7 is
IO_CMD_PREADV as far as I can see).  That'll explain
my "testcase" with Oracle which does not use preadv.

Thanks!

/mjt

Re: aio: compat_ioctl issue?

[Michael Tokarev]

Michael Tokarev wrote:
> Jeff Moyer wrote:
> []
>>>>> I just come across a situation (next in a long row :)
>>>>> when on x86, 32bit userspace does not work with 64bit
>>>>> kernel.  This time this is about aio requests.
>> [snip]
[]
>> Could you maybe print out the values that are passed to io_getevents?
> 
> They were in my first email, here it goes again:
> 
> io_submit: lio_opcode=7 reqprio=0 iov=0x9cd7018{0xf5599000,4096}, niov=1, offset=0
> io_getevents: expected 4096 got -22 (EINVAL)
> 
> This is what gets passed to libaio -- strace here
> does not decode the arguments unfortunately.
[]
> My *guess* is that it handles read/write correctly but
> does not properly handle preadv/pwritev (opcode=7 is
> IO_CMD_PREADV as far as I can see).  That'll explain
> my "testcase" with Oracle which does not use preadv.

Actually, looking at the code in fs/compat.c, I don't see
where it converts iovecs.  Yes it converts iocbs, but for
readv/writev it also needs to convert iovecs.  Oh well,
that expects to be quite painful... :(

> Thanks!
> 
> /mjt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: aio: compat_ioctl issue?

[Jeff Moyer]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Michael Tokarev wrote:
>> Jeff Moyer wrote:
>> []
>>>>>> I just come across a situation (next in a long row :)
>>>>>> when on x86, 32bit userspace does not work with 64bit
>>>>>> kernel.  This time this is about aio requests.
>>> [snip]
> []
>>> Could you maybe print out the values that are passed to io_getevents?
>> 
>> They were in my first email, here it goes again:
>> 
>> io_submit: lio_opcode=7 reqprio=0 iov=0x9cd7018{0xf5599000,4096}, niov=1, offset=0
>> io_getevents: expected 4096 got -22 (EINVAL)
>> 
>> This is what gets passed to libaio -- strace here
>> does not decode the arguments unfortunately.
> []
>> My *guess* is that it handles read/write correctly but
>> does not properly handle preadv/pwritev (opcode=7 is
>> IO_CMD_PREADV as far as I can see).  That'll explain
>> my "testcase" with Oracle which does not use preadv.
>
> Actually, looking at the code in fs/compat.c, I don't see
> where it converts iovecs.  Yes it converts iocbs, but for
> readv/writev it also needs to convert iovecs.  Oh well,
> that expects to be quite painful... :(

Yeah, whoops.  I built the libaio test harness using -m32 and this patch
works for me.  Would you mind giving it a try?

Thanks,
Jeff

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 aio_setup_single_vector(struct kiocb *kiocb)
  *	Performs the initial checks and aio retry method
  *	setup for the kiocb at the time of io submission.
  */
-static ssize_t aio_setup_iocb(struct kiocb *kiocb)
+static ssize_t aio_setup_iocb(struct kiocb *kiocb, bool compat)
 {
 	struct file *file = kiocb->ki_filp;
 	ssize_t ret = 0;
@@ -1469,7 +1477,7 @@ static ssize_t aio_setup_iocb(struct kiocb *kiocb)
 		ret = security_file_permission(file, MAY_READ);
 		if (unlikely(ret))
 			break;
-		ret = aio_setup_vectored_rw(READ, kiocb);
+		ret = aio_setup_vectored_rw(READ, kiocb, compat);
 		if (ret)
 			break;
 		ret = -EINVAL;
@@ -1483,7 +1491,7 @@ static ssize_t aio_setup_iocb(struct kiocb *kiocb)
 		ret = security_file_permission(file, MAY_WRITE);
 		if (unlikely(ret))
 			break;
-		ret = aio_setup_vectored_rw(WRITE, kiocb);
+		ret = aio_setup_vectored_rw(WRITE, kiocb, compat);
 		if (ret)
 			break;
 		ret = -EINVAL;
@@ -1548,7 +1556,8 @@ static void aio_batch_free(struct hlist_head *batch_hash)
 }
 
 static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
-			 struct iocb *iocb, struct hlist_head *batch_hash)
+			 struct iocb *iocb, struct hlist_head *batch_hash,
+			 bool compat)
 {
 	struct kiocb *req;
 	struct file *file;
@@ -1609,7 +1618,7 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 	req->ki_left = req->ki_nbytes = iocb->aio_nbytes;
 	req->ki_opcode = iocb->aio_lio_opcode;
 
-	ret = aio_setup_iocb(req);
+	ret = aio_setup_iocb(req, compat);
 
 	if (ret)
 		goto out_put_req;
@@ -1637,20 +1646,8 @@ out_put_req:
 	return ret;
 }
 
-/* sys_io_submit:
- *	Queue the nr iocbs pointed to by iocbpp for processing.  Returns
- *	the number of iocbs queued.  May return -EINVAL if the aio_context
- *	specified by ctx_id is invalid, if nr is < 0, if the iocb at
- *	*iocbpp[0] is not properly initialized, if the operation specified
- *	is invalid for the file descriptor in the iocb.  May fail with
- *	-EFAULT if any of the data structures point to invalid data.  May
- *	fail with -EBADF if the file descriptor specified in the first
- *	iocb is invalid.  May fail with -EAGAIN if insufficient resources
- *	are available to queue any iocbs.  Will return 0 if nr is 0.  Will
- *	fail with -ENOSYS if not implemented.
- */
-SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
-		struct iocb __user * __user *, iocbpp)
+long do_io_submit(aio_context_t ctx_id, long nr,
+		  struct iocb __user *__user * iocbpp, bool compat)
 {
 	struct kioctx *ctx;
 	long ret = 0;
@@ -1687,7 +1684,7 @@ SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
 			break;
 		}
 
-		ret = io_submit_one(ctx, user_iocb, &tmp, batch_hash);
+		ret = io_submit_one(ctx, user_iocb, &tmp, batch_hash, compat);
 		if (ret)
 			break;
 	}
@@ -1696,6 +1693,24 @@ SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
 	put_ioctx(ctx);
 	return i ? i : ret;
 }
+		  
+/* sys_io_submit:
+ *	Queue the nr iocbs pointed to by iocbpp for processing.  Returns
+ *	the number of iocbs queued.  May return -EINVAL if the aio_context
+ *	specified by ctx_id is invalid, if nr is < 0, if the iocb at
+ *	*iocbpp[0] is not properly initialized, if the operation specified
+ *	is invalid for the file descriptor in the iocb.  May fail with
+ *	-EFAULT if any of the data structures point to invalid data.  May
+ *	fail with -EBADF if the file descriptor specified in the first
+ *	iocb is invalid.  May fail with -EAGAIN if insufficient resources
+ *	are available to queue any iocbs.  Will return 0 if nr is 0.  Will
+ *	fail with -ENOSYS if not implemented.
+ */
+SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
+		struct iocb __user * __user *, iocbpp)
+{
+	return do_io_submit(ctx_id, nr, iocbpp, 0);
+}
 
 /* lookup_kiocb
  *	Finds a given iocb for cancellation.
diff --git a/fs/compat.c b/fs/compat.c
index 00d90c2..340f20d 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -567,6 +567,61 @@ 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 *uiov32, unsigned long niov,
+		unsigned long fast_segs, struct iovec *fast_pointer,
+		struct iovec **ret_pointer)
+{
+	ssize_t tot_len = 0;
+	struct iovec *kiov = fast_pointer;
+
+	if (niov == 0)
+		goto out;
+	if (niov > UIO_MAXIOV) {
+		tot_len = -EINVAL;
+		goto out;
+	}
+	if (niov > fast_segs) {
+		kiov = kmalloc(niov*sizeof(struct iovec), GFP_KERNEL);
+		if (kiov == NULL) {
+			tot_len = -ENOMEM;
+			goto out;
+		}
+		*ret_pointer = kiov;
+	}
+
+	while (niov > 0) {
+		compat_uptr_t buf;
+		compat_size_t len;
+
+		if (get_user(len, &uiov32->iov_len) ||
+		   get_user(buf, &uiov32->iov_base)) {
+			tot_len = -EFAULT;
+			goto out;
+		}
+		if (len < 0 || (tot_len + len < tot_len)) {
+			tot_len = -EINVAL;
+			goto out;
+		}
+		if (!access_ok(vrfy_dir(type), buf, len)) {
+			tot_len = -EFAULT;
+			goto out;
+		}
+		tot_len += len;
+		kiov->iov_base = compat_ptr(buf);
+		kiov->iov_len = (__kernel_size_t) len;
+		uiov32++;
+		kiov++;
+		niov--;
+	}
+
+out:
+	return tot_len;
+}
+
 static inline long
 copy_iocb(long nr, u32 __user *ptr32, struct iocb __user * __user *ptr64)
 {
@@ -599,7 +654,7 @@ compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)
 	iocb64 = compat_alloc_user_space(nr * sizeof(*iocb64));
 	ret = copy_iocb(nr, iocb, iocb64);
 	if (!ret)
-		ret = sys_io_submit(ctx_id, nr, iocb64);
+		ret = do_io_submit(ctx_id, nr, iocb64, 1);
 	return ret;
 }
 
diff --git a/include/linux/aio.h b/include/linux/aio.h
index 811dbb3..54b6ef8 100644
--- a/include/linux/aio.h
+++ b/include/linux/aio.h
@@ -212,6 +212,8 @@ extern void kick_iocb(struct kiocb *iocb);
 extern int aio_complete(struct kiocb *iocb, long res, long res2);
 struct mm_struct;
 extern void exit_aio(struct mm_struct *mm);
+extern long do_io_submit(aio_context_t ctx_id, long nr,
+			 struct iocb __user *__user * iocbpp, bool compat);
 #else
 static inline ssize_t wait_on_sync_kiocb(struct kiocb *iocb) { return 0; }
 static inline int aio_put_req(struct kiocb *iocb) { return 0; }
@@ -219,6 +221,9 @@ static inline void kick_iocb(struct kiocb *iocb) { }
 static inline int aio_complete(struct kiocb *iocb, long res, long res2) { return 0; }
 struct mm_struct;
 static inline void exit_aio(struct mm_struct *mm) { }
+static inline long do_io_submit(aio_context_t ctx_id, long nr,
+				struct iocb __user * __user * iocbpp,
+				bool compat) { }
 #endif /* CONFIG_AIO */
 
 static inline struct kiocb *list_kiocb(struct list_head *h)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index ef68119..8251f61 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -353,5 +353,9 @@ asmlinkage long compat_sys_newfstatat(unsigned int dfd, char __user * filename,
 asmlinkage long compat_sys_openat(unsigned int dfd, const char __user *filename,
 				  int flags, int mode);
 
+extern ssize_t compat_rw_copy_check_uvector(int type,
+		const struct compat_iovec __user *uiov32, unsigned long niov,
+		unsigned long fast_segs, struct iovec *fast_pointer,
+		struct iovec **ret_pointer);
 #endif /* CONFIG_COMPAT */
 #endif /* _LINUX_COMPAT_H */

Re: aio: compat_ioctl issue?

[Michael Tokarev]

Jeff Moyer wrote:
[]
> Yeah, whoops.  I built the libaio test harness using -m32 and this patch
> works for me.  Would you mind giving it a try?

It appears to work here so far.  I did not run massive i/o test suite
but it now passes basic multiple kernel unpacking in parallel tests
(before it didnt' even recognize the partition table).

So the patch - at least basically - works.

Except that we now have code duplication in kernel.  Look at
fs/compat.c:compat_do_readv_writev() - it does the same as
compat_rw_copy_check_uvector() plus a bit more.  I guess it
is a good idea to use compat_rw_copy_check_uvector() in
compat_do_readv_writev() to build the compat vectors...

Thank you for the patch and the fix!

/mjt

Re: aio: compat_ioctl issue?

[Jeff Moyer]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Jeff Moyer wrote:
> []
>> Yeah, whoops.  I built the libaio test harness using -m32 and this patch
>> works for me.  Would you mind giving it a try?
>
> It appears to work here so far.  I did not run massive i/o test suite
> but it now passes basic multiple kernel unpacking in parallel tests
> (before it didnt' even recognize the partition table).
>
> So the patch - at least basically - works.
>
> Except that we now have code duplication in kernel.  Look at
> fs/compat.c:compat_do_readv_writev() - it does the same as
> compat_rw_copy_check_uvector() plus a bit more.  I guess it
> is a good idea to use compat_rw_copy_check_uvector() in
> compat_do_readv_writev() to build the compat vectors...

I'm well aware of the code duplication, thanks.  You can even find
another copy in net/compat.c.  I'll see what I can do about it.

Cheers,
Jeff

Re: aio: compat_ioctl issue?

[Michael Tokarev]

Michael Tokarev ?????:
> Jeff Moyer wrote:
> []
>> Yeah, whoops.  I built the libaio test harness using -m32 and this patch
>> works for me.  Would you mind giving it a try?
> 
> It appears to work here so far.  I did not run massive i/o test suite
> but it now passes basic multiple kernel unpacking in parallel tests
> (before it didnt' even recognize the partition table).
> 
> So the patch - at least basically - works.

It looks like the conclusion was preliminary a bit.
I tested the wrong kvm binary which does not have
aio support...

And when running real thing it crashes.   I tested it
on 2.6.32 (trivial corrections to the patch required).

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff810c0e57>] generic_segment_checks+0x17/0xd0
PGD 18afe0067 PUD 18ace1067 PMD 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/devices/virtual/input/mice/uevent
CPU 0
Modules linked in: powernow_k8 rfcomm hidp l2cap bluetooth rfkill autofs4 nfsd exportfs nfs lockd fscache nfs_acl auth_rpcgss sunrpc tun ipv6 bridge stp llc quota_v2 quota_tree ext4 jbd2 crc16 md_mod asus_atk0110 thermal r8169 processor thermal_sys floppy mii psmouse edac_core shpchp pci_hotplug i2c_piix4 sg evdev radeon ttm drm_kms_helper drm i2c_algo_bit cfbcopyarea cfbimgblt cfbfillrect fbcon font bitblit softcursor fb saa7134_alsa kvm_amd kvm k8temp hwmon button tuner_simple tuner_types tea5767 tuner saa7134 ir_common v4l2_common videodev v4l1_compat v4l2_compat_ioctl32 videobuf_dma_sg videobuf_core tveeprom i2c_core lp parport_pc parport usb_storage ehci_hcd uhci_hcd snd_mixer_oss snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_timer snd soundcore
snd_page_alloc sr_mod cdrom ext3 jbd mbcache pata_atiixp ohci_hcd ahci libata usbhid hid usbcore nls_base sd_mod scsi_mod
Pid: 5135, comm: kvm Not tainted 2.6.32-amd64 #2.6.32.9 System Product Name
RIP: 0010:[<ffffffff810c0e57>]  [<ffffffff810c0e57>] generic_segment_checks+0x17/0xd0
RSP: 0000:ffff88018ac6bd80  EFLAGS: 00010202
RAX: ffff88018aebcf00 RBX: ffff88018ac6bdf0 RCX: 0000000000000001
RDX: ffff88018ac6be20 RSI: 0000000000000001 RDI: 0000000000000000
RBP: ffff88018ac6be20 R08: ffffffffffffffea R09: ffff88018aebcf30
R10: 0000000000000000 R11: 0000000000000010 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: ffff88018ad3d2c0
FS:  0000000000000000(0000) GS:ffff880028200000(0063) knlGS:00000000f6fc9b70
CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
CR2: 0000000000000008 CR3: 000000018af4e000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kvm (pid: 5135, threadinfo ffff88018ac6a000, task ffff88018aefb040)
Stack:
 ffff88018aebce80 ffff88018aebce80 ffffffff810c36b5 00000010813f8b00
<0> ffff88018aebcf00 0000000000013ad8 ffff88018ac6bfd8 ffff88018aefb040
<0> ffffffff813f7700 ffff88018aefb040 0000000000000000 0000000000000000
Call Trace:
 [<ffffffff810c36b5>] ? generic_file_aio_read+0x55/0x620
 [<ffffffff810f8696>] ? cache_alloc_refill+0x96/0x5d0
 [<ffffffff810c3660>] ? generic_file_aio_read+0x0/0x620
 [<ffffffff81139a6c>] ? aio_rw_vect_retry+0x7c/0x210
 [<ffffffff8113b252>] ? aio_run_iocb+0x82/0x150
 [<ffffffff8113bb8f>] ? do_io_submit+0x2bf/0x640
 [<ffffffff81038672>] ? ia32_sysret+0x0/0x5
Code: 82 c0 6c 3f 81 c3 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 45 31 d2 48 89 d5 53 48 89 f3 48 8b 36 48 85 f6 0f 84 84 00 00 00 <4c> 8b 57 08 4d 85 d2 0f 88 8c 00 00 00 48 8b 07 65 4c 8b 04 25
RIP  [<ffffffff810c0e57>] generic_segment_checks+0x17/0xd0
 RSP <ffff88018ac6bd80>
CR2: 0000000000000008
---[ end trace 3abdd573ab9bce13 ]---

I'm recompiling 2.6.33 now.  There, patch applied cleanly.

/mjt

Re: aio: compat_ioctl issue?

[Jeff Moyer]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Michael Tokarev ?????:
>> Jeff Moyer wrote:
>> []
>>> Yeah, whoops.  I built the libaio test harness using -m32 and this patch
>>> works for me.  Would you mind giving it a try?
>> 
>> It appears to work here so far.  I did not run massive i/o test suite
>> but it now passes basic multiple kernel unpacking in parallel tests
>> (before it didnt' even recognize the partition table).
>> 
>> So the patch - at least basically - works.
>
> It looks like the conclusion was preliminary a bit.
> I tested the wrong kvm binary which does not have
> aio support...
>
> And when running real thing it crashes.   I tested it
> on 2.6.32 (trivial corrections to the patch required).

Could you just post your version of the patch so I can have a look?

Thanks,
Jeff

Re: aio: compat_ioctl issue?

[Michael Tokarev]

Jeff Moyer wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
[]
>> And when running real thing it crashes.   I tested it
>> on 2.6.32 (trivial corrections to the patch required).
> 
> Could you just post your version of the patch so I can have a look?

I think there's no need since in 2.6.33 (where your patch applies
without any offsets) it crashes in exactly the same way:

------
BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff810bbf37>] generic_segment_checks+0x17/0xd0
PGD 184f1c067 PUD 184cd1067 PMD 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/devices/virtual/input/mice/uevent
CPU 1
Pid: 5182, comm: kvm Not tainted 2.6.33-amd64 #2.6.33.0 M3A78-EM/System Product Name
RIP: 0010:[<ffffffff810bbf37>]  [<ffffffff810bbf37>] generic_segment_checks+0x17/0xd0
RSP: 0000:ffff880184d71d30  EFLAGS: 00010202
RAX: ffff88018f2eeed8 RBX: ffff880184d71da0 RCX: 0000000000000001
RDX: ffff880184d71dd0 RSI: 0000000000000001 RDI: 0000000000000000
RBP: ffff880184d71dd0 R08: ffffffffffffffea R09: ffff88018f2eef08
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: ffff88018cff3e00 R15: ffff880184c49b00
FS:  0000000000000000(0000) GS:ffff880028280000(0063) knlGS:00000000f6f75b70
CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
CR2: 0000000000000008 CR3: 0000000184e9b000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kvm (pid: 5182, threadinfo ffff880184d70000, task ffff880184d6a080)
Stack:
 ffff88018f2eee80 ffff88018f2eee80 ffffffff810be7a2 ffff880184c90a70
<0> ffff88018f2eeed8 ffff880184c91680 ffff880184c91678 000000000000004f
<0> ffffffff81403680 fffffffe7ffbfeff 0000000000000000 0000000000000000
Call Trace:
 [<ffffffff810be7a2>] ? generic_file_aio_read+0x52/0x620
 [<ffffffff810f3296>] ? cache_alloc_refill+0x96/0x5e0
 [<ffffffff810be750>] ? generic_file_aio_read+0x0/0x620
 [<ffffffff81135634>] ? aio_rw_vect_retry+0x74/0x1f0
 [<ffffffff81136e02>] ? aio_run_iocb+0x82/0x140
 [<ffffffff8113777c>] ? do_io_submit+0x2cc/0x7b0
 [<ffffffff81031412>] ? ia32_sysret+0x0/0x5
Code: 82 40 2c 40 81 c3 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 45 31 d2 48 89 d5 53 48 89 f3 48 8b 36 48 85 f6 0f 84 84 00 00 00 <4c> 8b 57 08 4d 85 d2 0f 88 8c 00 00 00 48 8b 07 65 4c 8b 04 25
RIP  [<ffffffff810bbf37>] generic_segment_checks+0x17/0xd0
 RSP <ffff880184d71d30>
CR2: 0000000000000008
---[ end trace 78b5aba2446c4209 ]---

For 2.6.32, the differences were all in fs/aio.c, in 2
places: include file (obvious),

 static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
                          struct iocb *iocb, struct hlist_head *batch_hash)

in 2.6.32 it does not have the last 'batch_hash' parameter,
and the only caller of this routine in io_submit() down in
that file.  So the change is trivial and purely mechanical.

But it is not here yet.  I'm trying to dig further...

/mjt

Re: aio: compat_ioctl issue?

[Jeff Moyer]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Jeff Moyer wrote:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
> []
>>> And when running real thing it crashes.   I tested it
>>> on 2.6.32 (trivial corrections to the patch required).
>> 
>> Could you just post your version of the patch so I can have a look?
>
> I think there's no need since in 2.6.33 (where your patch applies
> without any offsets) it crashes in exactly the same way:

Sorry for taking so long on this.  I only tested the case where niovs >
fast_segs, and I missed an obvious thing: I didn't assign the return
pointer to the proper iovec.

So, this patch should get you going.

Cheers,
Jeff

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 aio_setup_single_vector(struct kiocb *kiocb)
  *	Performs the initial checks and aio retry method
  *	setup for the kiocb at the time of io submission.
  */
-static ssize_t aio_setup_iocb(struct kiocb *kiocb)
+static ssize_t aio_setup_iocb(struct kiocb *kiocb, bool compat)
 {
 	struct file *file = kiocb->ki_filp;
 	ssize_t ret = 0;
@@ -1469,7 +1477,7 @@ static ssize_t aio_setup_iocb(struct kiocb *kiocb)
 		ret = security_file_permission(file, MAY_READ);
 		if (unlikely(ret))
 			break;
-		ret = aio_setup_vectored_rw(READ, kiocb);
+		ret = aio_setup_vectored_rw(READ, kiocb, compat);
 		if (ret)
 			break;
 		ret = -EINVAL;
@@ -1483,7 +1491,7 @@ static ssize_t aio_setup_iocb(struct kiocb *kiocb)
 		ret = security_file_permission(file, MAY_WRITE);
 		if (unlikely(ret))
 			break;
-		ret = aio_setup_vectored_rw(WRITE, kiocb);
+		ret = aio_setup_vectored_rw(WRITE, kiocb, compat);
 		if (ret)
 			break;
 		ret = -EINVAL;
@@ -1548,7 +1556,8 @@ static void aio_batch_free(struct hlist_head *batch_hash)
 }
 
 static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
-			 struct iocb *iocb, struct hlist_head *batch_hash)
+			 struct iocb *iocb, struct hlist_head *batch_hash,
+			 bool compat)
 {
 	struct kiocb *req;
 	struct file *file;
@@ -1609,7 +1618,7 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 	req->ki_left = req->ki_nbytes = iocb->aio_nbytes;
 	req->ki_opcode = iocb->aio_lio_opcode;
 
-	ret = aio_setup_iocb(req);
+	ret = aio_setup_iocb(req, compat);
 
 	if (ret)
 		goto out_put_req;
@@ -1637,20 +1646,8 @@ out_put_req:
 	return ret;
 }
 
-/* sys_io_submit:
- *	Queue the nr iocbs pointed to by iocbpp for processing.  Returns
- *	the number of iocbs queued.  May return -EINVAL if the aio_context
- *	specified by ctx_id is invalid, if nr is < 0, if the iocb at
- *	*iocbpp[0] is not properly initialized, if the operation specified
- *	is invalid for the file descriptor in the iocb.  May fail with
- *	-EFAULT if any of the data structures point to invalid data.  May
- *	fail with -EBADF if the file descriptor specified in the first
- *	iocb is invalid.  May fail with -EAGAIN if insufficient resources
- *	are available to queue any iocbs.  Will return 0 if nr is 0.  Will
- *	fail with -ENOSYS if not implemented.
- */
-SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
-		struct iocb __user * __user *, iocbpp)
+long do_io_submit(aio_context_t ctx_id, long nr,
+		  struct iocb __user *__user * iocbpp, bool compat)
 {
 	struct kioctx *ctx;
 	long ret = 0;
@@ -1687,7 +1684,7 @@ SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
 			break;
 		}
 
-		ret = io_submit_one(ctx, user_iocb, &tmp, batch_hash);
+		ret = io_submit_one(ctx, user_iocb, &tmp, batch_hash, compat);
 		if (ret)
 			break;
 	}
@@ -1696,6 +1693,24 @@ SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
 	put_ioctx(ctx);
 	return i ? i : ret;
 }
+		  
+/* sys_io_submit:
+ *	Queue the nr iocbs pointed to by iocbpp for processing.  Returns
+ *	the number of iocbs queued.  May return -EINVAL if the aio_context
+ *	specified by ctx_id is invalid, if nr is < 0, if the iocb at
+ *	*iocbpp[0] is not properly initialized, if the operation specified
+ *	is invalid for the file descriptor in the iocb.  May fail with
+ *	-EFAULT if any of the data structures point to invalid data.  May
+ *	fail with -EBADF if the file descriptor specified in the first
+ *	iocb is invalid.  May fail with -EAGAIN if insufficient resources
+ *	are available to queue any iocbs.  Will return 0 if nr is 0.  Will
+ *	fail with -ENOSYS if not implemented.
+ */
+SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
+		struct iocb __user * __user *, iocbpp)
+{
+	return do_io_submit(ctx_id, nr, iocbpp, 0);
+}
 
 /* lookup_kiocb
  *	Finds a given iocb for cancellation.
diff --git a/fs/compat.c b/fs/compat.c
index 00d90c2..efbcb31 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -567,6 +567,61 @@ 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 *uiov32, unsigned long niov,
+		unsigned long fast_segs, struct iovec *fast_pointer,
+		struct iovec **ret_pointer)
+{
+	ssize_t tot_len = 0;
+	struct iovec *kiov = fast_pointer;
+
+	if (niov == 0)
+		goto out;
+	if (niov > UIO_MAXIOV) {
+		tot_len = -EINVAL;
+		goto out;
+	}
+	if (niov > fast_segs) {
+		kiov = kmalloc(niov*sizeof(struct iovec), GFP_KERNEL);
+		if (kiov == NULL) {
+			tot_len = -ENOMEM;
+			goto out;
+		}
+	}
+	*ret_pointer = kiov;
+
+	while (niov > 0) {
+		compat_uptr_t buf;
+		compat_size_t len;
+
+		if (get_user(len, &uiov32->iov_len) ||
+		   get_user(buf, &uiov32->iov_base)) {
+			tot_len = -EFAULT;
+			goto out;
+		}
+		if (len < 0 || (tot_len + len < tot_len)) {
+			tot_len = -EINVAL;
+			goto out;
+		}
+		if (!access_ok(vrfy_dir(type), buf, len)) {
+			tot_len = -EFAULT;
+			goto out;
+		}
+		tot_len += len;
+		kiov->iov_base = compat_ptr(buf);
+		kiov->iov_len = (__kernel_size_t) len;
+		uiov32++;
+		kiov++;
+		niov--;
+	}
+
+out:
+	return tot_len;
+}
+
 static inline long
 copy_iocb(long nr, u32 __user *ptr32, struct iocb __user * __user *ptr64)
 {
@@ -599,7 +654,7 @@ compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)
 	iocb64 = compat_alloc_user_space(nr * sizeof(*iocb64));
 	ret = copy_iocb(nr, iocb, iocb64);
 	if (!ret)
-		ret = sys_io_submit(ctx_id, nr, iocb64);
+		ret = do_io_submit(ctx_id, nr, iocb64, 1);
 	return ret;
 }
 
diff --git a/include/linux/aio.h b/include/linux/aio.h
index 811dbb3..54b6ef8 100644
--- a/include/linux/aio.h
+++ b/include/linux/aio.h
@@ -212,6 +212,8 @@ extern void kick_iocb(struct kiocb *iocb);
 extern int aio_complete(struct kiocb *iocb, long res, long res2);
 struct mm_struct;
 extern void exit_aio(struct mm_struct *mm);
+extern long do_io_submit(aio_context_t ctx_id, long nr,
+			 struct iocb __user *__user * iocbpp, bool compat);
 #else
 static inline ssize_t wait_on_sync_kiocb(struct kiocb *iocb) { return 0; }
 static inline int aio_put_req(struct kiocb *iocb) { return 0; }
@@ -219,6 +221,9 @@ static inline void kick_iocb(struct kiocb *iocb) { }
 static inline int aio_complete(struct kiocb *iocb, long res, long res2) { return 0; }
 struct mm_struct;
 static inline void exit_aio(struct mm_struct *mm) { }
+static inline long do_io_submit(aio_context_t ctx_id, long nr,
+				struct iocb __user * __user * iocbpp,
+				bool compat) { }
 #endif /* CONFIG_AIO */
 
 static inline struct kiocb *list_kiocb(struct list_head *h)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index ef68119..8251f61 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -353,5 +353,9 @@ asmlinkage long compat_sys_newfstatat(unsigned int dfd, char __user * filename,
 asmlinkage long compat_sys_openat(unsigned int dfd, const char __user *filename,
 				  int flags, int mode);
 
+extern ssize_t compat_rw_copy_check_uvector(int type,
+		const struct compat_iovec __user *uiov32, unsigned long niov,
+		unsigned long fast_segs, struct iovec *fast_pointer,
+		struct iovec **ret_pointer);
 #endif /* CONFIG_COMPAT */
 #endif /* _LINUX_COMPAT_H */

Re: aio: compat_ioctl issue?

[Michael Tokarev]

Jeff Moyer wrote:
[]
> Sorry for taking so long on this.  I only tested the case where niovs >
> fast_segs, and I missed an obvious thing: I didn't assign the return
> pointer to the proper iovec.

There's no need to be sorry really.  Because, well, the whole thing isn't
quite useful anyway: running proper 64bit code is preferable ;)

I actually tried the thing, running a guest right now, which in turn is
running a quick benchmark and appears to perform quite good at it too.

> So, this patch should get you going.

Well, I already switched to 64bit kvm binary for my case, and actually
that one makes alot more sense anyway: there's no conversion like this
needed, and no 32<=>64bit mode switching either.  (Actually 32bit code
in this my case is slower elsewhere too).

By the way, how about the case when we've several {write,read}v in the
iocb array?  Will each use the same fast_segs array from the beginning,
overwriting data of previous iocb element? :)  Just... curious :)

Thank you for your support!

You can add my
Tested-By: Michael Tokarev <mjt@tls.msk.ru>
if you want.  Thanks!

/mjt

Re: aio: compat_ioctl issue?

[Jeff Moyer]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Jeff Moyer wrote:
> []
>> Sorry for taking so long on this.  I only tested the case where niovs >
>> fast_segs, and I missed an obvious thing: I didn't assign the return
>> pointer to the proper iovec.
>
> There's no need to be sorry really.  Because, well, the whole thing isn't
> quite useful anyway: running proper 64bit code is preferable ;)
>
> I actually tried the thing, running a guest right now, which in turn is
> running a quick benchmark and appears to perform quite good at it too.

OK, great.  I'm in the process of unifying the duplicated code, now, so
I might ask for one more sanity check if you have the time and patience
for it.

>> So, this patch should get you going.
>
> Well, I already switched to 64bit kvm binary for my case, and actually
> that one makes alot more sense anyway: there's no conversion like this
> needed, and no 32<=>64bit mode switching either.  (Actually 32bit code
> in this my case is slower elsewhere too).

OK, makes sense, but we should get this right.

> By the way, how about the case when we've several {write,read}v in the
> iocb array?  Will each use the same fast_segs array from the beginning,
> overwriting data of previous iocb element? :)  Just... curious :)

No, each iocb has a built-in iovec which gets specified for the
fast_iov.

> Thank you for your support!
>
> You can add my
> Tested-By: Michael Tokarev <mjt@tls.msk.ru>
> if you want.  Thanks!

Thanks!

-Jeff

Re: aio: compat_ioctl issue?

[Avi Kivity]

On 03/16/2010 10:44 PM, Jeff Moyer wrote:
>
>> Thank you for your support!
>>
>> You can add my
>> Tested-By: Michael Tokarev<mjt@tls.msk.ru>
>> if you want.  Thanks!
>>      
> Thanks!
>
>    

Is this patch getting upstreamed?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: aio: compat_ioctl issue?

[Jeff Moyer]

Avi Kivity <avi@redhat.com> writes:

> On 03/16/2010 10:44 PM, Jeff Moyer wrote:
>>
>>> Thank you for your support!
>>>
>>> You can add my
>>> Tested-By: Michael Tokarev<mjt@tls.msk.ru>
>>> if you want.  Thanks!
>>>      
>> Thanks!
>>
>>    
>
> Is this patch getting upstreamed?

Sorry, I've been inundated with a host of other issues.  I will post a
new version as soon as I can (hopefully this week).

Cheers,
Jeff