Re: [PATCH 32/32] aio: implement io_pgetevents

Subsystems: abi/api, aio, filesystems (vfs and infrastructure), the rest

4 messages, 3 authors, 2018-01-17 · open the first message on its own page

Re: [PATCH 32/32] aio: implement io_pgetevents

From: Jeff Moyer <hidden>
Date: 2018-01-17 00:41:24 

Hi, Christoph,

Christoph Hellwig [off-list ref] writes:

On Mon, Jan 15, 2018 at 09:53:10AM +0100, Christoph Hellwig wrote:
quoted
quoted
pselect, as an example, crams the sigmask and size together.  Why not
just do that?  libaio can take care of setting that up.
Yes, I could try that.  It's just another double indirection for no
good reason.
I cna't get this to work - for some reason I always end up with a NULL
sigmask in the kernel.  Nevermind that it leads to really crap code
generation.  I guess for select the latter doesn't matter too much as
everyone sane uses ppoll anyway.
I'd be willing to bet the issue is in your io_syscall6 implementation.
You pass in arg5 where arg6 should be used.  Don't feel bad, it took me
the better part of today to figure that out.  :)

Here's an incremental diff on top of what you've posted.  Feel free to
fold it into your patch (and format however you like).  You can find the
libaio changes in my 'aio-poll' branch:
  https://pagure.io/libaio/commits/aio-poll

These changes were run through the libaio test harness, 64 bit and 32
bit, so the compat system call was tested.

Cheers,
Jeff

diff --git a/fs/aio.c b/fs/aio.c
index 57a4e8d..c6d67d0 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1991,9 +1991,11 @@ static long do_io_getevents(aio_context_t ctx_id,
 		long, nr,
 		struct io_event __user *, events,
 		struct timespec __user *, timeout,
-		const sigset_t __user *, sigmask)
+		void __user *, sigmask)
 {
 	sigset_t		ksigmask, sigsaved;
+	size_t			sigsetsize = 0;
+	sigset_t __user		*up = NULL;
 	struct timespec64	ts;
 	int ret;
 
@@ -2001,8 +2003,18 @@ static long do_io_getevents(aio_context_t ctx_id,
 		return -EFAULT;
 
 	if (sigmask) {
-		if (copy_from_user(&ksigmask, sigmask, sizeof(ksigmask)))
+		if (!access_ok(VERIFY_READ, sigmask,
+			       sizeof(void *) + sizeof(size_t)) ||
+		    __get_user(up, (sigset_t __user * __user *)sigmask) ||
+		    __get_user(sigsetsize,
+			       (size_t __user *)(sigmask + sizeof(void *))))
 			return -EFAULT;
+
+		if (sigsetsize != sizeof(sigset_t))
+			return -EINVAL;
+		if (copy_from_user(&ksigmask, up, sizeof(ksigmask)))
+			return -EFAULT;
+
 		sigdelsetmask(&ksigmask, sigmask(SIGKILL) | sigmask(SIGSTOP));
 		sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
 	}
@@ -2049,8 +2061,11 @@ static long do_io_getevents(aio_context_t ctx_id,
 		compat_long_t, nr,
 		struct io_event __user *, events,
 		struct compat_timespec __user *, timeout,
-		const compat_sigset_t __user *, sigmask)
+		void __user *, sig)
 {
+	compat_size_t sigsetsize = 0;
+	compat_sigset_t __user *sigmask;
+	compat_uptr_t up = 0;
 	sigset_t ksigmask, sigsaved;
 	struct timespec64 t;
 	int ret;
@@ -2058,8 +2073,17 @@ static long do_io_getevents(aio_context_t ctx_id,
 	if (timeout && compat_get_timespec64(&t, timeout))
 		return -EFAULT;
 
-	if (sigmask) {
-		if (get_compat_sigset(&ksigmask, sigmask))
+	if (sig) {
+		if (!access_ok(VERIFY_READ, sig,
+			sizeof(compat_uptr_t) + sizeof(compat_size_t)) ||
+		    __get_user(up, (compat_uptr_t __user *)sig) ||
+		    __get_user(sigsetsize,
+			       (compat_size_t __user *)(sig + sizeof(up))))
+			return -EFAULT;
+
+		if (sigsetsize != sizeof(compat_sigset_t))
+			return -EINVAL;
+		if (get_compat_sigset(&ksigmask, compat_ptr(up)))
 			return -EFAULT;
 		sigdelsetmask(&ksigmask, sigmask(SIGKILL) | sigmask(SIGSTOP));
 		sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
diff --git a/include/linux/compat.h b/include/linux/compat.h
index a4cda98..32412f8 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -541,7 +541,7 @@ asmlinkage long compat_sys_io_pgetevents(compat_aio_context_t ctx_id,
 					compat_long_t nr,
 					struct io_event __user *events,
 					struct compat_timespec __user *timeout,
-					const compat_sigset_t __user *sigmask);
+					void __user *sigmask);
 asmlinkage long compat_sys_io_submit(compat_aio_context_t ctx_id, int nr,
 				     u32 __user *iocb);
 asmlinkage long compat_sys_mount(const char __user *dev_name,
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 3bc9a13..bc79026 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -544,7 +544,7 @@ asmlinkage long sys_io_pgetevents(aio_context_t ctx_id,
 				long nr,
 				struct io_event __user *events,
 				struct timespec __user *timeout,
-				const sigset_t __user *sigmask);
+				void __user *sigmask);
 asmlinkage long sys_io_submit(aio_context_t, long,
 				struct iocb __user * __user *);
 asmlinkage long sys_io_cancel(aio_context_t ctx_id, struct iocb __user *iocb,

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [PATCH 32/32] aio: implement io_pgetevents

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: 2018-01-17 04:27:25 

On Tue, Jan 16, 2018 at 07:41:24PM -0500, Jeff Moyer wrote:
 	if (sigmask) {
-		if (copy_from_user(&ksigmask, sigmask, sizeof(ksigmask)))
+		if (!access_ok(VERIFY_READ, sigmask,
+			       sizeof(void *) + sizeof(size_t)) ||
+		    __get_user(up, (sigset_t __user * __user *)sigmask) ||
+		    __get_user(sigsetsize,
+			       (size_t __user *)(sigmask + sizeof(void *))))
 			return -EFAULT;
How about copy_from_user() on a struct?  Making eyes bleed is fun, but
people tend to get annoyed when you do it to them...

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [PATCH 32/32] aio: implement io_pgetevents

From: Christoph Hellwig <hch@lst.de>
Date: 2018-01-17 07:08:27 

On Wed, Jan 17, 2018 at 04:27:21AM +0000, Al Viro wrote:
On Tue, Jan 16, 2018 at 07:41:24PM -0500, Jeff Moyer wrote:
quoted
 	if (sigmask) {
-		if (copy_from_user(&ksigmask, sigmask, sizeof(ksigmask)))
+		if (!access_ok(VERIFY_READ, sigmask,
+			       sizeof(void *) + sizeof(size_t)) ||
+		    __get_user(up, (sigset_t __user * __user *)sigmask) ||
+		    __get_user(sigsetsize,
+			       (size_t __user *)(sigmask + sizeof(void *))))
 			return -EFAULT;
How about copy_from_user() on a struct?  Making eyes bleed is fun, but
people tend to get annoyed when you do it to them...
Above is the copy & paste version from pselect.  I've got both copy_from_user
and that horrible version in my tree, and if we really need this awfull
calling convention copy_from_user certainly is much better.  pselect
also should be switched to explicit struct + copy_from_user while
we're at it.  In fact glibc defines a struct for the userland version
to start with.

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [PATCH 32/32] aio: implement io_pgetevents

From: Christoph Hellwig <hch@lst.de>
Date: 2018-01-17 07:36:52 

On Tue, Jan 16, 2018 at 07:41:24PM -0500, Jeff Moyer wrote:
I'd be willing to bet the issue is in your io_syscall6 implementation.
You pass in arg5 where arg6 should be used.  Don't feel bad, it took me
the better part of today to figure that out.  :)

Here's an incremental diff on top of what you've posted.  Feel free to
fold it into your patch (and format however you like).  You can find the
libaio changes in my 'aio-poll' branch:
  https://pagure.io/libaio/commits/aio-poll

These changes were run through the libaio test harness, 64 bit and 32
bit, so the compat system call was tested.
Oops, yes.  Although I prefer the copy_from_user version, this is what
I had:


diff --git a/fs/aio.c b/fs/aio.c
index 9fe0a5539596..6c1bbfa9b06a 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1984,8 +1984,9 @@ SYSCALL_DEFINE6(io_pgetevents,
 		long, nr,
 		struct io_event __user *, events,
 		struct timespec __user *, timeout,
-		const sigset_t __user *, sigmask)
+		const struct __aio_sigset __user *, usig)
 {
+	struct __aio_sigset	ksig = { NULL, };
 	sigset_t		ksigmask, sigsaved;
 	struct timespec64	ts;
 	int ret;
@@ -1993,8 +1994,13 @@ SYSCALL_DEFINE6(io_pgetevents,
 	if (timeout && unlikely(get_timespec64(&ts, timeout)))
 		return -EFAULT;
 
-	if (sigmask) {
-		if (copy_from_user(&ksigmask, sigmask, sizeof(ksigmask)))
+	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
+		return -EFAULT;
+
+	if (ksig.sigmask) {
+		if (ksig.sigsetsize != sizeof(sigset_t))
+			return -EINVAL;
+		if (copy_from_user(&ksigmask, ksig.sigmask, sizeof(ksigmask)))
 			return -EFAULT;
 		sigdelsetmask(&ksigmask, sigmask(SIGKILL) | sigmask(SIGSTOP));
 		sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
@@ -2002,7 +2008,7 @@ SYSCALL_DEFINE6(io_pgetevents,
 
 	ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
 	if (signal_pending(current)) {
-		if (sigmask) {
+		if (ksig.sigmask) {
 			current->saved_sigmask = sigsaved;
 			set_restore_sigmask();
 		}
@@ -2010,7 +2016,7 @@ SYSCALL_DEFINE6(io_pgetevents,
 		if (!ret)
 			ret = -ERESTARTNOHAND;
 	} else {
-		if (sigmask)
+		if (ksig.sigmask)
 			sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 	}
 
@@ -2036,14 +2042,21 @@ COMPAT_SYSCALL_DEFINE5(io_getevents, compat_aio_context_t, ctx_id,
 	return ret;
 }
 
+
+struct __compat_aio_sigset {
+	compat_sigset_t __user	*sigmask;
+	compat_size_t		sigsetsize;
+};
+
 COMPAT_SYSCALL_DEFINE6(io_pgetevents,
 		compat_aio_context_t, ctx_id,
 		compat_long_t, min_nr,
 		compat_long_t, nr,
 		struct io_event __user *, events,
 		struct compat_timespec __user *, timeout,
-		const compat_sigset_t __user *, sigmask)
+		const struct __compat_aio_sigset __user *, usig)
 {
+	struct __compat_aio_sigset ksig = { NULL, };
 	sigset_t ksigmask, sigsaved;
 	struct timespec64 t;
 	int ret;
@@ -2051,8 +2064,13 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
 	if (timeout && compat_get_timespec64(&t, timeout))
 		return -EFAULT;
 
-	if (sigmask) {
-		if (get_compat_sigset(&ksigmask, sigmask))
+	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
+		return -EFAULT;
+
+	if (ksig.sigmask) {
+		if (ksig.sigsetsize != sizeof(compat_sigset_t))
+			return -EINVAL;
+		if (get_compat_sigset(&ksigmask, ksig.sigmask))
 			return -EFAULT;
 		sigdelsetmask(&ksigmask, sigmask(SIGKILL) | sigmask(SIGSTOP));
 		sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
@@ -2060,14 +2078,14 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
 
 	ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
 	if (signal_pending(current)) {
-		if (sigmask) {
+		if (ksig.sigmask) {
 			current->saved_sigmask = sigsaved;
 			set_restore_sigmask();
 		}
 		if (!ret)
 			ret = -ERESTARTNOHAND;
 	} else {
-		if (sigmask)
+		if (ksig.sigmask)
 			sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 	}
 
diff --git a/include/linux/compat.h b/include/linux/compat.h
index a4cda98073f1..6c04450e961f 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -205,6 +205,7 @@ extern int put_compat_rusage(const struct rusage *,
 			     struct compat_rusage __user *);
 
 struct compat_siginfo;
+struct __compat_aio_sigset;
 
 extern asmlinkage long compat_sys_waitid(int, compat_pid_t,
 		struct compat_siginfo __user *, int,
@@ -541,7 +542,7 @@ asmlinkage long compat_sys_io_pgetevents(compat_aio_context_t ctx_id,
 					compat_long_t nr,
 					struct io_event __user *events,
 					struct compat_timespec __user *timeout,
-					const compat_sigset_t __user *sigmask);
+					const struct __compat_aio_sigset __user *usig);
 asmlinkage long compat_sys_io_submit(compat_aio_context_t ctx_id, int nr,
 				     u32 __user *iocb);
 asmlinkage long compat_sys_mount(const char __user *dev_name,
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 3bc9a130f4a9..8515ec53c81b 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -544,7 +544,7 @@ asmlinkage long sys_io_pgetevents(aio_context_t ctx_id,
 				long nr,
 				struct io_event __user *events,
 				struct timespec __user *timeout,
-				const sigset_t __user *sigmask);
+				const struct __aio_sigset *sig);
 asmlinkage long sys_io_submit(aio_context_t, long,
 				struct iocb __user * __user *);
 asmlinkage long sys_io_cancel(aio_context_t ctx_id, struct iocb __user *iocb,
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index 28330105a4b6..ed0185945bb2 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -29,6 +29,7 @@
 
 #include <linux/types.h>
 #include <linux/fs.h>
+#include <linux/signal.h>
 #include <asm/byteorder.h>
 
 typedef __kernel_ulong_t aio_context_t;
@@ -106,5 +107,10 @@ struct iocb {
 #undef IFBIG
 #undef IFLITTLE
 
+struct __aio_sigset {
+	sigset_t __user	*sigmask;
+	size_t		sigsetsize;
+};
+
 #endif /* __LINUX__AIO_ABI_H */
 

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help