LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit
       [not found] <20070117104601.36b2ab18@frecb000686>
@ 2007-01-17  9:48 ` Sébastien Dugué
  2007-01-17  9:48 ` [PATCH -mm 2/5][AIO] - fix aio.h includes Sébastien Dugué
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-17  9:48 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty


                 compat_sys_io_submit() cleanup


  Cleanup compat_sys_io_submit by duplicating some of the native syscall
logic in the compat layer and directly calling io_submit_one() instead
of fooling the syscall into thinking it is called from a native 64-bit
caller.

  This eliminates:

  - the overhead of copying the nr iocb pointers on the userspace stack

  - the PAGE_SIZE/(sizeof(void *)) limit on the number of iocbs that
    can be submitted.

  This is also needed for the completion notification patch to avoid having
to rewrite each iocb on the caller stack for io_submit_one() to find the
sigevents.


 compat.c |   61 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

Signed-off-by: Sébastien Dugué <sebastien.dugue@bull.net>

Index: linux-2.6.20-rc4-mm1/fs/compat.c
===================================================================
--- linux-2.6.20-rc4-mm1.orig/fs/compat.c	2007-01-12 11:40:28.000000000 +0100
+++ linux-2.6.20-rc4-mm1/fs/compat.c	2007-01-12 12:14:51.000000000 +0100
@@ -644,40 +644,47 @@ out:
 	return ret;
 }
 
-static inline long
-copy_iocb(long nr, u32 __user *ptr32, struct iocb __user * __user *ptr64)
-{
-	compat_uptr_t uptr;
-	int i;
-
-	for (i = 0; i < nr; ++i) {
-		if (get_user(uptr, ptr32 + i))
-			return -EFAULT;
-		if (put_user(compat_ptr(uptr), ptr64 + i))
-			return -EFAULT;
-	}
-	return 0;
-}
-
-#define MAX_AIO_SUBMITS 	(PAGE_SIZE/sizeof(struct iocb *))
-
 asmlinkage long
 compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)
 {
-	struct iocb __user * __user *iocb64; 
-	long ret;
+	struct kioctx *ctx;
+	long ret = 0;
+	int i;
 
 	if (unlikely(nr < 0))
 		return -EINVAL;
 
-	if (nr > MAX_AIO_SUBMITS)
-		nr = MAX_AIO_SUBMITS;
-	
-	iocb64 = compat_alloc_user_space(nr * sizeof(*iocb64));
-	ret = copy_iocb(nr, iocb, iocb64);
-	if (!ret)
-		ret = sys_io_submit(ctx_id, nr, iocb64);
-	return ret;
+	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
+		return -EFAULT;
+
+	ctx = lookup_ioctx(ctx_id);
+	if (unlikely(!ctx))
+		return -EINVAL;
+
+	for (i=0; i<nr; i++) {
+		compat_uptr_t uptr;
+		struct iocb __user *user_iocb;
+		struct iocb tmp;
+
+		if (unlikely(get_user(uptr, iocb + i))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		user_iocb = compat_ptr(uptr);
+
+		if (unlikely(copy_from_user(&tmp, user_iocb, sizeof(tmp)))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		ret = io_submit_one(ctx, user_iocb, &tmp);
+		if (ret)
+			break;
+	}
+
+	put_ioctx(ctx);
+	return i ? i: ret;
 }
 
 struct compat_ncp_mount_data {

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH -mm 2/5][AIO] - fix aio.h includes
       [not found] <20070117104601.36b2ab18@frecb000686>
  2007-01-17  9:48 ` [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit Sébastien Dugué
@ 2007-01-17  9:48 ` Sébastien Dugué
  2007-01-17  9:49 ` [PATCH -mm 3/5][AIO] - Make good_sigevent non-static Sébastien Dugué
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-17  9:48 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty


  Fix the double inclusion of linux/uio.h in linux/aio.h



 aio.h |    1 -
 1 file changed, 1 deletion(-)

Signed-off-by: Sébastien Dugué <sebastien.dugue@bull.net>

Index: linux-2.6.20-rc4-mm1/include/linux/aio.h
===================================================================
--- linux-2.6.20-rc4-mm1.orig/include/linux/aio.h	2007-01-12 11:40:39.000000000 +0100
+++ linux-2.6.20-rc4-mm1/include/linux/aio.h	2007-01-12 12:15:01.000000000 +0100
@@ -7,7 +7,6 @@
 #include <linux/uio.h>
 
 #include <asm/atomic.h>
-#include <linux/uio.h>
 
 #define AIO_MAXSEGS		4
 #define AIO_KIOGRP_NR_ATOMIC	8

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH -mm 3/5][AIO] - Make good_sigevent non-static
       [not found] <20070117104601.36b2ab18@frecb000686>
  2007-01-17  9:48 ` [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit Sébastien Dugué
  2007-01-17  9:48 ` [PATCH -mm 2/5][AIO] - fix aio.h includes Sébastien Dugué
@ 2007-01-17  9:49 ` Sébastien Dugué
  2007-01-17  9:50 ` [PATCH -mm 4/5][AIO] - AIO completion signal notification Sébastien Dugué
  2007-01-17  9:55 ` [PATCH -mm 5/5][AIO] - Add listio syscall support Sébastien Dugué
  4 siblings, 0 replies; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-17  9:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty


                      Make good_sigevent() non-static


  Move good_sigevent() from posix-timers.c to signal.c where it belongs,
and make it non-static so that it can be used by other subsystems.


 include/linux/signal.h |    1 +
 kernel/posix-timers.c  |   17 -----------------
 kernel/signal.c        |   24 ++++++++++++++++++++++++
 3 files changed, 25 insertions(+), 17 deletions(-)

Signed-off-by: Sébastien Dugué <sebastien.dugue@bull.net>


Index: linux-2.6.20-rc4-mm1/include/linux/signal.h
===================================================================
--- linux-2.6.20-rc4-mm1.orig/include/linux/signal.h	2007-01-12 11:40:30.000000000 +0100
+++ linux-2.6.20-rc4-mm1/include/linux/signal.h	2007-01-12 12:15:06.000000000 +0100
@@ -240,6 +240,7 @@ extern int sigprocmask(int, sigset_t *, 
 
 struct pt_regs;
 extern int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka, struct pt_regs *regs, void *cookie);
+extern struct task_struct * good_sigevent(sigevent_t *);
 
 extern struct kmem_cache *sighand_cachep;
 
Index: linux-2.6.20-rc4-mm1/kernel/posix-timers.c
===================================================================
--- linux-2.6.20-rc4-mm1.orig/kernel/posix-timers.c	2007-01-12 11:40:39.000000000 +0100
+++ linux-2.6.20-rc4-mm1/kernel/posix-timers.c	2007-01-12 12:15:06.000000000 +0100
@@ -367,23 +367,6 @@ static enum hrtimer_restart posix_timer_
 	return ret;
 }
 
-static struct task_struct * good_sigevent(sigevent_t * event)
-{
-	struct task_struct *rtn = current->group_leader;
-
-	if ((event->sigev_notify & SIGEV_THREAD_ID ) &&
-		(!(rtn = find_task_by_pid(event->sigev_notify_thread_id)) ||
-		 rtn->tgid != current->tgid ||
-		 (event->sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_SIGNAL))
-		return NULL;
-
-	if (((event->sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE) &&
-	    ((event->sigev_signo <= 0) || (event->sigev_signo > SIGRTMAX)))
-		return NULL;
-
-	return rtn;
-}
-
 void register_posix_clock(const clockid_t clock_id, struct k_clock *new_clock)
 {
 	if ((unsigned) clock_id >= MAX_CLOCKS) {
Index: linux-2.6.20-rc4-mm1/kernel/signal.c
===================================================================
--- linux-2.6.20-rc4-mm1.orig/kernel/signal.c	2007-01-12 11:40:39.000000000 +0100
+++ linux-2.6.20-rc4-mm1/kernel/signal.c	2007-01-12 12:15:06.000000000 +0100
@@ -1213,6 +1213,30 @@ int group_send_sig_info(int sig, struct 
 	return ret;
 }
 
+/***
+ * good_sigevent - check and get target task from a sigevent.
+ * @event: the sigevent to be checked
+ *
+ * This function must be called with the tasklist_lock held for reading.
+ */
+struct task_struct * good_sigevent(sigevent_t * event)
+{
+	struct task_struct *task = current->group_leader;
+
+	if ((event->sigev_notify & SIGEV_THREAD_ID ) == SIGEV_THREAD_ID) {
+		task = find_task_by_pid(event->sigev_notify_thread_id);
+
+		if (!task || task->tgid != current->tgid)
+			return NULL;
+	} else if (event->sigev_notify == SIGEV_SIGNAL) {
+		if ((event->sigev_signo <= 0) || (event->sigev_signo > SIGRTMAX))
+			return NULL;
+	} else
+		return NULL;
+
+	return task;
+}
+
 /*
  * kill_pgrp_info() sends a signal to a process group: this is what the tty
  * control characters do (^C, ^Z etc)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH -mm 4/5][AIO] - AIO completion signal notification
       [not found] <20070117104601.36b2ab18@frecb000686>
                   ` (2 preceding siblings ...)
  2007-01-17  9:49 ` [PATCH -mm 3/5][AIO] - Make good_sigevent non-static Sébastien Dugué
@ 2007-01-17  9:50 ` Sébastien Dugué
  2007-01-24  5:35   ` Andrew Morton
  2007-01-17  9:55 ` [PATCH -mm 5/5][AIO] - Add listio syscall support Sébastien Dugué
  4 siblings, 1 reply; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-17  9:50 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty


                      AIO completion signal notification

  The current 2.6 kernel does not support notification of user space via
an RT signal upon an asynchronous IO completion. The POSIX specification
states that when an AIO request completes, a signal can be delivered to
the application as notification.

  This patch adds a struct sigevent *aio_sigeventp to the iocb.
The relevant fields (pid, signal number and value) are stored in the kiocb
for use when the request completes.

  That sigevent structure is filled by the application as part of the AIO
request preparation. Upon request completion, the kernel notifies the
application using those sigevent parameters. If SIGEV_NONE has been specified,
then the old behaviour is retained and the application must rely on polling
the completion queue using io_getevents().


  A struct sigevent *aio_sigeventp field is added to struct iocb in
include/linux/aio_abi.h

  A struct aio_notify containing the sigevent parameters is defined in aio.h:

  struct aio_notify {
	struct task_struct	*target;
	__u16			signo;
	__u16			notify;
	sigval_t		value;
  };

  A struct aio_notify ki_notify is added to struct kiocb in include/linux/aio.h

  In io_submit_one(), if the application provided a sigevent then
setup_sigevent() is called which does the following:

	- check access to the user sigevent and make a local copy

	- if the requested notification is SIGEV_NONE, then nothing to do

	- fill in the kiocb->ki_notify fields (notify, signo, value)

	- check sigevent consistency, get the signal target task and
	  save it in kiocb->ki_notify.target

	- preallocate a sigqueue for this event using sigqueue_alloc()

  Upon request completion, in aio_complete(), if notification is needed for
this request (iocb->ki_notify.notify != SIGEV_NONE), then aio_send_signal()
is called to signal the target task as follows:

	- fill in the siginfo struct to be sent to the task

	- if notify is SIGEV_THREAD_ID then send signal to specific task
	  using send_sigqueue()

	- else send signal to task group using send_5group_sigqueue()



Notes concerning sigqueue preallocation:

 To ensure reliable delivery of completion notification, the sigqueue is
preallocated in the submission path so that there is no chance it can fail
in the completion path.

 Unlike the posix-timers case (currently the single other user of sigqueue
preallocation), where the sigqueue is allocated for the lifetime of the timer
and freed at timer destruction time, the aio case is a bit more tricky due to
the async nature of the whole thing.

  In the aio case, the sigqueue exists for the lifetime of the request, therefore
it must be freed only once the signal for the request completion has been
delivered. This involves changing __sigqueue_free() to free the sigqueue when the
signal is collected if si_code is SI_ASYNCIO even if it was preallocated as well
as explicitly calling sigqueue_free() in submission and completion error paths.


 fs/aio.c                |  115 ++++++++++++++++++++++++++++++++++++++++++++++--
 fs/compat.c             |   18 +++++++
 include/linux/aio.h     |   12 +++++
 include/linux/aio_abi.h |    3 -
 kernel/signal.c         |    2
 5 files changed, 144 insertions(+), 6 deletions(-)

Signed-off-by: Sébastien Dugué <sebastien.dugue@bull.net>
Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>

Index: linux-2.6.20-rc4-mm1/fs/aio.c
===================================================================
--- linux-2.6.20-rc4-mm1.orig/fs/aio.c	2007-01-12 11:40:38.000000000 +0100
+++ linux-2.6.20-rc4-mm1/fs/aio.c	2007-01-12 12:32:55.000000000 +0100
@@ -419,6 +419,7 @@ static struct kiocb fastcall *__aio_get_
 	req->ki_dtor = NULL;
 	req->private = NULL;
 	req->ki_iovec = NULL;
+	req->ki_notify.sigq = NULL;
 	INIT_LIST_HEAD(&req->ki_run_list);
 
 	/* Check if the completion queue has enough free space to
@@ -465,6 +466,12 @@ static inline void really_put_req(struct
 		req->ki_dtor(req);
 	if (req->ki_iovec != &req->ki_inline_vec)
 		kfree(req->ki_iovec);
+
+	/* Release task ref */
+	if (req->ki_notify.notify == SIGEV_THREAD_ID ||
+	    req->ki_notify.notify == SIGEV_SIGNAL)
+		put_task_struct(req->ki_notify.target);
+
 	kmem_cache_free(kiocb_cachep, req);
 	ctx->reqs_active--;
 
@@ -916,6 +923,79 @@ void fastcall kick_iocb(struct kiocb *io
 }
 EXPORT_SYMBOL(kick_iocb);
 
+static int aio_send_signal(struct aio_notify *notify)
+{
+	struct sigqueue *sigq = notify->sigq;
+	struct siginfo *info = &sigq->info;
+	int ret;
+
+	memset(info, 0, sizeof(struct siginfo));
+
+	info->si_signo = notify->signo;
+	info->si_errno = 0;
+	info->si_code = SI_ASYNCIO;
+	info->si_pid = 0;
+	info->si_uid = 0;
+	info->si_value = notify->value;
+
+	if (notify->notify & SIGEV_THREAD_ID)
+		ret = send_sigqueue(notify->signo, sigq, notify->target);
+	else
+		ret = send_group_sigqueue(notify->signo, sigq, notify->target);
+
+	return ret;
+}
+
+static long aio_setup_sigevent(struct aio_notify *notify,
+			       struct sigevent __user *user_event)
+{
+	sigevent_t event;
+	struct task_struct *target;
+
+	if (copy_from_user(&event, user_event, sizeof (event)))
+		return -EFAULT;
+
+	if (event.sigev_notify == SIGEV_NONE)
+		return 0;
+
+	notify->notify = event.sigev_notify;
+	notify->signo = event.sigev_signo;
+	notify->value = event.sigev_value;
+
+	read_lock(&tasklist_lock);
+	target = good_sigevent(&event);
+
+	if (unlikely(!target || (target->flags & PF_EXITING)))
+		goto out_unlock;
+
+	/*
+	 * At this point, we know that notify is either SIGEV_SIGNAL or
+	 * SIGEV_THREAD_ID and the target task is valid. So get a reference
+	 * on the task, it will be dropped in really_put_req() when
+	 * we're done with the request.
+	 */
+	get_task_struct(target);
+	notify->target = target;
+	read_unlock(&tasklist_lock);
+
+	/*
+	 * NOTE: we cannot free the sigqueue in the completion path as
+	 * the signal may not have been delivered to the target task.
+	 * Therefore it has to be freed in __sigqueue_free() when the
+	 * signal is collected if si_code is SI_ASYNCIO.
+	 */
+	notify->sigq = sigqueue_alloc();
+
+	if (unlikely(!notify->sigq))
+		return -EAGAIN;
+
+	return 0;
+
+out_unlock:
+	read_unlock(&tasklist_lock);
+	return -EINVAL;
+}
+
 /* aio_complete
  *	Called when the io request on the given iocb is complete.
  *	Returns true if this is the last user of the request.  The 
@@ -963,8 +1043,11 @@ int fastcall aio_complete(struct kiocb *
 	 * cancelled requests don't get events, userland was given one
 	 * when the event got cancelled.
 	 */
-	if (kiocbIsCancelled(iocb))
+	if (kiocbIsCancelled(iocb)) {
+		if (iocb->ki_notify.sigq)
+			sigqueue_free(iocb->ki_notify.sigq);
 		goto put_rq;
+	}
 
 	ring = kmap_atomic(info->ring_pages[0], KM_IRQ1);
 
@@ -994,6 +1077,15 @@ int fastcall aio_complete(struct kiocb *
 	kunmap_atomic(ring, KM_IRQ1);
 
 	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
+
+	if (iocb->ki_notify.notify != SIGEV_NONE) {
+		ret = aio_send_signal(&iocb->ki_notify);
+
+		/* If signal generation failed, release the sigqueue */
+		if (ret)
+			sigqueue_free(iocb->ki_notify.sigq);
+	}
+
 put_rq:
 	/* everything turned out well, dispose of the aiocb. */
 	ret = __aio_put_req(ctx, iocb);
@@ -1545,8 +1637,7 @@ int fastcall io_submit_one(struct kioctx
 	ssize_t ret;
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(iocb->aio_reserved1 || iocb->aio_reserved2 ||
-		     iocb->aio_reserved3)) {
+	if (unlikely(iocb->aio_reserved1 || iocb->aio_reserved3)) {
 		pr_debug("EINVAL: io_submit: reserve field set\n");
 		return -EINVAL;
 	}
@@ -1555,6 +1646,7 @@ int fastcall io_submit_one(struct kioctx
 	if (unlikely(
 	    (iocb->aio_buf != (unsigned long)iocb->aio_buf) ||
 	    (iocb->aio_nbytes != (size_t)iocb->aio_nbytes) ||
+	    (iocb->aio_sigeventp != (unsigned long)iocb->aio_sigeventp) ||
 	    ((ssize_t)iocb->aio_nbytes < 0)
 	   )) {
 		pr_debug("EINVAL: io_submit: overflow check\n");
@@ -1588,11 +1680,21 @@ int fastcall io_submit_one(struct kioctx
 	init_waitqueue_func_entry(&req->ki_wait.wait, aio_wake_function);
 	INIT_LIST_HEAD(&req->ki_wait.wait.task_list);
  	req->ki_run_list.next = req->ki_run_list.prev = NULL;
+	/* handle setting up the sigevent for POSIX AIO signals */
+	req->ki_notify.notify = SIGEV_NONE;
+
+	if (iocb->aio_sigeventp) {
+		ret = aio_setup_sigevent(&req->ki_notify,
+					 (struct sigevent __user *)(unsigned long)
+					 iocb->aio_sigeventp);
+		if (ret)
+			goto out_put_req;
+	}
 
 	ret = aio_setup_iocb(req);
 
 	if (ret)
-		goto out_put_req;
+		goto out_sigqfree;
 
 	spin_lock_irq(&ctx->ctx_lock);
 	aio_run_iocb(req);
@@ -1605,6 +1707,11 @@ int fastcall io_submit_one(struct kioctx
 	aio_put_req(req);	/* drop extra ref to req */
 	return 0;
 
+out_sigqfree:
+	/* Undo the sigqueue alloc if someting went bad */
+	if (req->ki_notify.sigq)
+		sigqueue_free(req->ki_notify.sigq);
+
 out_put_req:
 	aio_put_req(req);	/* drop extra ref to req */
 	aio_put_req(req);	/* drop i/o ref to req */
Index: linux-2.6.20-rc4-mm1/include/linux/aio_abi.h
===================================================================
--- linux-2.6.20-rc4-mm1.orig/include/linux/aio_abi.h	2006-11-29 22:57:37.000000000 +0100
+++ linux-2.6.20-rc4-mm1/include/linux/aio_abi.h	2007-01-12 12:30:09.000000000 +0100
@@ -82,8 +82,9 @@ struct iocb {
 	__u64	aio_nbytes;
 	__s64	aio_offset;
 
+	__u64	aio_sigeventp;	/* pointer to struct sigevent */
+
 	/* extra parameters */
-	__u64	aio_reserved2;	/* TODO: use this for a (struct sigevent *) */
 	__u64	aio_reserved3;
 }; /* 64 bytes */
 
Index: linux-2.6.20-rc4-mm1/include/linux/aio.h
===================================================================
--- linux-2.6.20-rc4-mm1.orig/include/linux/aio.h	2007-01-12 12:15:01.000000000 +0100
+++ linux-2.6.20-rc4-mm1/include/linux/aio.h	2007-01-12 12:30:09.000000000 +0100
@@ -7,6 +7,7 @@
 #include <linux/uio.h>
 
 #include <asm/atomic.h>
+#include <asm/siginfo.h>
 
 #define AIO_MAXSEGS		4
 #define AIO_KIOGRP_NR_ATOMIC	8
@@ -54,6 +55,14 @@ struct kioctx;
 #define kiocbIsCancelled(iocb)	test_bit(KIF_CANCELLED, &(iocb)->ki_flags)
 #define kiocbIsRestarted(iocb)	test_bit(KIF_RESTARTED, &(iocb)->ki_flags)
 
+struct aio_notify {
+	struct task_struct	*target;
+	__u16			signo;
+	__u16			notify;
+	sigval_t		value;
+	struct sigqueue		*sigq;
+};
+
 /* is there a better place to document function pointer methods? */
 /**
  * ki_retry	-	iocb forward progress callback
@@ -123,6 +132,9 @@ struct kiocb {
 
 	struct list_head	ki_list;	/* the aio core uses this
 						 * for cancellation */
+
+	/* to notify a process on I/O event */
+	struct aio_notify	ki_notify;
 };
 
 #define is_sync_kiocb(iocb)	((iocb)->ki_key == KIOCB_SYNC_KEY)
Index: linux-2.6.20-rc4-mm1/kernel/signal.c
===================================================================
--- linux-2.6.20-rc4-mm1.orig/kernel/signal.c	2007-01-12 12:15:06.000000000 +0100
+++ linux-2.6.20-rc4-mm1/kernel/signal.c	2007-01-12 12:30:09.000000000 +0100
@@ -297,7 +297,7 @@ static struct sigqueue *__sigqueue_alloc
 
 static void __sigqueue_free(struct sigqueue *q)
 {
-	if (q->flags & SIGQUEUE_PREALLOC)
+	if (q->flags & SIGQUEUE_PREALLOC && q->info.si_code != SI_ASYNCIO)
 		return;
 	atomic_dec(&q->user->sigpending);
 	free_uid(q->user);
Index: linux-2.6.20-rc4-mm1/fs/compat.c
===================================================================
--- linux-2.6.20-rc4-mm1.orig/fs/compat.c	2007-01-12 12:14:51.000000000 +0100
+++ linux-2.6.20-rc4-mm1/fs/compat.c	2007-01-12 12:30:09.000000000 +0100
@@ -665,6 +665,7 @@ compat_sys_io_submit(aio_context_t ctx_i
 		compat_uptr_t uptr;
 		struct iocb __user *user_iocb;
 		struct iocb tmp;
+		struct compat_sigevent __user *uevent;
 
 		if (unlikely(get_user(uptr, iocb + i))) {
 			ret = -EFAULT;
@@ -678,6 +679,23 @@ compat_sys_io_submit(aio_context_t ctx_i
 			break;
 		}
 
+		uevent = (struct compat_sigevent __user *)tmp.aio_sigeventp;
+
+		if (uevent) {
+			struct sigevent __user *event = NULL;
+			struct sigevent kevent;
+
+			event = compat_alloc_user_space(sizeof(*event));
+
+			if (get_compat_sigevent(&kevent, uevent) ||
+			    copy_to_user(event, &kevent, sizeof(*event))) {
+				ret = -EFAULT;
+				break;
+			}
+
+			tmp.aio_sigeventp = (__u64)event;
+		}
+
 		ret = io_submit_one(ctx, user_iocb, &tmp);
 		if (ret)
 			break;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH -mm 5/5][AIO] - Add listio syscall support
       [not found] <20070117104601.36b2ab18@frecb000686>
                   ` (3 preceding siblings ...)
  2007-01-17  9:50 ` [PATCH -mm 4/5][AIO] - AIO completion signal notification Sébastien Dugué
@ 2007-01-17  9:55 ` Sébastien Dugué
  2007-01-24  5:50   ` Andrew Morton
                     ` (2 more replies)
  4 siblings, 3 replies; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-17  9:55 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty

From: Bharata B Rao <bharata@in.ibm.com>

This patch provides POSIX listio support by means of a new system call.

long lio_submit(aio_context_t ctx_id, int mode, long nr,
	struct iocb __user * __user *iocbpp, struct sigevent __user *event)

This system call is similar to the io_submit() system call, but takes
two more arguments.

'mode' argument can be LIO_WAIT or LIO_NOWAIT.

'event' argument specifies the signal notification mechanism.

This patch is built on support provided by the aio signal notification
patch by Sebastien. The following two structures together provide
the support for grouping iocbs belonging to a list (lio).

struct aio_notify {
	struct task_struct	*target;
	__u16			signo;
	__u16			notify;
	sigval_t		value;
	struct sigqueue		*sigq;
};

struct lio_event {
	atomic_t		lio_users;
	struct aio_notify	lio_notify;
};

A single lio_event struct is maintained for the list of iocbs.
lio_users holds the number of requests attached to this lio and lio_notify
has the necessary information for generating completion notification
signal.

If the mode is LIO_WAIT, the event argument is ignored and the system
call waits until all the requests of the lio are completed.

If the mode is LIO_NOWAIT, the system call returns immediately after
submitting the io requests and may optionally notify the process on
list io completion depending on the event argument.

Signed-off-by: Sébastien Dugué <sebastien.dugue@bull.net>
Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>
Signed-off-by: Bharata B Rao <bharata@in.ibm.com>
---

 arch/i386/kernel/syscall_table.S |    1 
 arch/x86_64/ia32/ia32entry.S     |    4 
 fs/aio.c                         |  175 ++++++++++++++++++++++++++++++++++-----
 fs/compat.c                      |  117 +++++++++++++++++++++-----
 include/asm-i386/unistd.h        |    3 
 include/asm-x86_64/unistd.h      |    4 
 include/linux/aio.h              |   14 ++-
 include/linux/aio_abi.h          |    5 +
 include/linux/syscalls.h         |    2 
 9 files changed, 280 insertions(+), 45 deletions(-)

Index: linux-2.6.20-rc4-mm1/arch/i386/kernel/syscall_table.S
===================================================================
--- linux-2.6.20-rc4-mm1.orig/arch/i386/kernel/syscall_table.S	2007-01-17 08:58:02.000000000 +0100
+++ linux-2.6.20-rc4-mm1/arch/i386/kernel/syscall_table.S	2007-01-17 08:59:16.000000000 +0100
@@ -319,3 +319,4 @@ ENTRY(sys_call_table)
 	.long sys_move_pages
 	.long sys_getcpu
 	.long sys_epoll_pwait
+	.long sys_lio_submit		/* 320 */
Index: linux-2.6.20-rc4-mm1/fs/aio.c
===================================================================
--- linux-2.6.20-rc4-mm1.orig/fs/aio.c	2007-01-17 08:58:02.000000000 +0100
+++ linux-2.6.20-rc4-mm1/fs/aio.c	2007-01-17 08:59:16.000000000 +0100
@@ -416,6 +416,7 @@ static struct kiocb fastcall *__aio_get_
 	req->ki_ctx = ctx;
 	req->ki_cancel = NULL;
 	req->ki_retry = NULL;
+	req->ki_lio = NULL;
 	req->ki_dtor = NULL;
 	req->private = NULL;
 	req->ki_iovec = NULL;
@@ -996,6 +997,59 @@ out_unlock:
 	return -EINVAL;
 }
 
+void lio_check(struct lio_event *lio)
+{
+	int ret;
+
+	ret = atomic_dec_and_test(&lio->lio_users);
+
+	if (unlikely(ret) && lio->lio_notify.notify != SIGEV_NONE) {
+		/* last one -> notify process */
+		if (aio_send_signal(&lio->lio_notify))
+			sigqueue_free(lio->lio_notify.sigq);
+		kfree(lio);
+	}
+}
+
+struct lio_event *lio_create(struct sigevent __user *user_event,
+			int mode)
+{
+	int ret = 0;
+	struct lio_event *lio = NULL;
+
+	if (unlikely((mode == LIO_NOWAIT) && !user_event))
+		return lio;
+
+	lio = kzalloc(sizeof(*lio), GFP_KERNEL);
+
+	if (!lio)
+		return ERR_PTR(-EAGAIN);
+
+	/*
+	 * Grab an initial ref on the lio to avoid races between
+	 * submission and completion.
+	 */
+	atomic_set(&lio->lio_users, 1);
+
+	lio->lio_notify.notify = SIGEV_NONE;
+
+	/* sigevent argument is ignored with LIO_WAIT */
+	if (user_event && (mode == LIO_NOWAIT)) {
+		/*
+		 * User specified an event for this lio,
+		 * he wants to be notified upon lio completion.
+		 */
+		ret = aio_setup_sigevent(&lio->lio_notify, user_event);
+
+		if (ret) {
+			kfree(lio);
+			return ERR_PTR(ret);
+		}
+	}
+
+	return lio;
+}
+
 /* aio_complete
  *	Called when the io request on the given iocb is complete.
  *	Returns true if this is the last user of the request.  The 
@@ -1044,6 +1098,9 @@ int fastcall aio_complete(struct kiocb *
 	 * when the event got cancelled.
 	 */
 	if (kiocbIsCancelled(iocb)) {
+		if (iocb->ki_lio)
+			lio_check(iocb->ki_lio);
+
 		if (iocb->ki_notify.sigq)
 			sigqueue_free(iocb->ki_notify.sigq);
 		goto put_rq;
@@ -1086,6 +1143,9 @@ int fastcall aio_complete(struct kiocb *
 			sigqueue_free(iocb->ki_notify.sigq);
 	}
 
+	if (iocb->ki_lio)
+		lio_check(iocb->ki_lio);
+
 put_rq:
 	/* everything turned out well, dispose of the aiocb. */
 	ret = __aio_put_req(ctx, iocb);
@@ -1630,7 +1690,7 @@ static int aio_wake_function(wait_queue_
 }
 
 int fastcall io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
-			 struct iocb *iocb)
+			 struct iocb *iocb, struct lio_event *lio)
 {
 	struct kiocb *req;
 	struct file *file;
@@ -1691,6 +1751,9 @@ int fastcall io_submit_one(struct kioctx
 			goto out_put_req;
 	}
 
+	/* Attach this iocb to its lio */
+	req->ki_lio = lio;
+
 	ret = aio_setup_iocb(req);
 
 	if (ret)
@@ -1718,6 +1781,48 @@ out_put_req:
 	return ret;
 }
 
+static int io_submit_group(struct kioctx *ctx, long nr,
+	struct iocb __user * __user *iocbpp, struct lio_event *lio)
+{
+	int i;
+	long ret = 0;
+
+	/*
+	 * AKPM: should this return a partial result if some of the IOs were
+	 * successfully submitted?
+	 */
+	for (i = 0; i < nr; i++) {
+		struct iocb __user *user_iocb;
+		struct iocb tmp;
+
+		if (unlikely(__get_user(user_iocb, iocbpp + i))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		if (unlikely(copy_from_user(&tmp, user_iocb, sizeof(tmp)))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		if (lio)
+			atomic_inc(&lio->lio_users);
+
+		ret = io_submit_one(ctx, user_iocb, &tmp, lio);
+		if (ret) {
+			if (lio) {
+				/*
+				 * In case of listio, continue with
+				 * the subsequent requests
+				 */
+				atomic_dec(&lio->lio_users);
+			} else
+				break;
+		}
+	}
+	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
@@ -1735,7 +1840,6 @@ asmlinkage long sys_io_submit(aio_contex
 {
 	struct kioctx *ctx;
 	long ret = 0;
-	int i;
 
 	if (unlikely(nr < 0))
 		return -EINVAL;
@@ -1749,31 +1853,60 @@ asmlinkage long sys_io_submit(aio_contex
 		return -EINVAL;
 	}
 
-	/*
-	 * AKPM: should this return a partial result if some of the IOs were
-	 * successfully submitted?
-	 */
-	for (i=0; i<nr; i++) {
-		struct iocb __user *user_iocb;
-		struct iocb tmp;
+	ret = io_submit_group(ctx, nr, iocbpp, NULL);
 
-		if (unlikely(__get_user(user_iocb, iocbpp + i))) {
-			ret = -EFAULT;
-			break;
-		}
+	put_ioctx(ctx);
+	return ret;
+}
 
-		if (unlikely(copy_from_user(&tmp, user_iocb, sizeof(tmp)))) {
-			ret = -EFAULT;
-			break;
-		}
+asmlinkage long sys_lio_submit(aio_context_t ctx_id, int mode, long nr,
+	struct iocb __user * __user *iocbpp, struct sigevent __user *event)
+{
+	struct kioctx *ctx;
+	struct lio_event *lio = NULL;
+	long ret = 0;
 
-		ret = io_submit_one(ctx, user_iocb, &tmp);
-		if (ret)
-			break;
+	if (unlikely(nr < 0))
+		return -EINVAL;
+
+	if (unlikely(!access_ok(VERIFY_READ, iocbpp, (nr*sizeof(*iocbpp)))))
+		return -EFAULT;
+
+	ctx = lookup_ioctx(ctx_id);
+	if (unlikely(!ctx)) {
+		pr_debug("EINVAL: lio_submit: invalid context id\n");
+		return -EINVAL;
+	}
+
+	lio = lio_create(event, mode);
+
+	ret = PTR_ERR(lio);
+	if (IS_ERR(lio))
+		goto out_put_ctx;
+
+	ret = io_submit_group(ctx, nr, iocbpp, lio);
+
+	/* If we failed to submit even one request just return */
+	if (ret < 0 ) {
+		if (lio)
+			kfree(lio);
+		goto out_put_ctx;
+	}
+
+	/*
+	 * Drop extra ref on the lio now that we're done submitting requests.
+	 */
+	if (lio)
+		lio_check(lio);
+
+	if (mode == LIO_WAIT) {
+		wait_event(ctx->wait, atomic_read(&lio->lio_users) == 0);
+		kfree(lio);
 	}
 
+out_put_ctx:
 	put_ioctx(ctx);
-	return i ? i : ret;
+	return ret;
 }
 
 /* lookup_kiocb
Index: linux-2.6.20-rc4-mm1/fs/compat.c
===================================================================
--- linux-2.6.20-rc4-mm1.orig/fs/compat.c	2007-01-17 08:58:02.000000000 +0100
+++ linux-2.6.20-rc4-mm1/fs/compat.c	2007-01-17 08:59:16.000000000 +0100
@@ -644,24 +644,13 @@ out:
 	return ret;
 }
 
-asmlinkage long
-compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)
+static int compat_io_submit_group(struct kioctx *ctx, long nr,
+	u32 __user *iocb, struct lio_event *lio)
 {
-	struct kioctx *ctx;
-	long ret = 0;
 	int i;
+	long ret = 0;
 
-	if (unlikely(nr < 0))
-		return -EINVAL;
-
-	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
-		return -EFAULT;
-
-	ctx = lookup_ioctx(ctx_id);
-	if (unlikely(!ctx))
-		return -EINVAL;
-
-	for (i=0; i<nr; i++) {
+	for (i = 0; i < nr; i++) {
 		compat_uptr_t uptr;
 		struct iocb __user *user_iocb;
 		struct iocb tmp;
@@ -696,13 +685,103 @@ compat_sys_io_submit(aio_context_t ctx_i
 			tmp.aio_sigeventp = (__u64)event;
 		}
 
-		ret = io_submit_one(ctx, user_iocb, &tmp);
-		if (ret)
-			break;
+		if (lio)
+			atomic_inc(&lio->lio_users);
+
+		ret = io_submit_one(ctx, user_iocb, &tmp, lio);
+		if (ret) {
+			if (lio) {
+				/*
+				 * In case of listio, continue with
+				 * the subsequent requests
+				 */
+				atomic_dec(&lio->lio_users);
+			} else
+				break;
+		}
+
+
 	}
+	return i ? i : ret;
+}
 
+asmlinkage long
+compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)
+{
+	struct kioctx *ctx;
+	long ret = 0;
+
+	if (unlikely(nr < 0))
+		return -EINVAL;
+
+	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
+		return -EFAULT;
+
+	ctx = lookup_ioctx(ctx_id);
+	if (unlikely(!ctx))
+		return -EINVAL;
+
+	ret = compat_io_submit_group(ctx, nr, iocb, NULL);
+	put_ioctx(ctx);
+	return ret;
+}
+
+asmlinkage long
+compat_sys_lio_submit(aio_context_t ctx_id, int mode, int nr, u32 __user *iocb,
+		struct compat_sigevent __user *sig_user)
+{
+	struct kioctx *ctx;
+	struct lio_event *lio = NULL;
+	struct sigevent __user *event = NULL;
+	long ret = 0;
+
+	if (unlikely(nr < 0))
+		return -EINVAL;
+
+	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
+		return -EFAULT;
+
+	ctx = lookup_ioctx(ctx_id);
+	if (unlikely(!ctx))
+		return -EINVAL;
+
+	if (sig_user) {
+		struct sigevent kevent;
+		event = compat_alloc_user_space(sizeof(struct sigevent));
+		if (get_compat_sigevent(&kevent, sig_user) ||
+			copy_to_user(event, &kevent, sizeof(struct sigevent)))
+			return -EFAULT;
+	}
+
+	lio = lio_create(event, mode);
+
+	ret = PTR_ERR(lio);
+	if (IS_ERR(lio))
+		goto out_put_ctx;
+
+	ret = compat_io_submit_group(ctx, nr, iocb, lio);
+
+	/* If we failed to submit even one request just return */
+	if (ret < 0) {
+		if (lio)
+			kfree(lio);
+		goto out_put_ctx;
+	}
+
+	/*
+	 * Drop extra ref on the lio now that we're done submitting requests.
+	 */
+	if (lio)
+		lio_check(lio);
+
+
+	if (mode == LIO_WAIT) {
+		wait_event(ctx->wait, atomic_read(&lio->lio_users) == 0);
+		kfree(lio);
+	}
+out_put_ctx:
 	put_ioctx(ctx);
-	return i ? i: ret;
+	return ret;
 }
 
 struct compat_ncp_mount_data {
Index: linux-2.6.20-rc4-mm1/include/asm-i386/unistd.h
===================================================================
--- linux-2.6.20-rc4-mm1.orig/include/asm-i386/unistd.h	2007-01-17 08:58:02.000000000 +0100
+++ linux-2.6.20-rc4-mm1/include/asm-i386/unistd.h	2007-01-17 08:59:16.000000000 +0100
@@ -325,10 +325,11 @@
 #define __NR_move_pages		317
 #define __NR_getcpu		318
 #define __NR_epoll_pwait	319
+#define __NR_lio_submit		320
 
 #ifdef __KERNEL__
 
-#define NR_syscalls 320
+#define NR_syscalls 321
 
 #define __ARCH_WANT_IPC_PARSE_VERSION
 #define __ARCH_WANT_OLD_READDIR
Index: linux-2.6.20-rc4-mm1/include/linux/aio_abi.h
===================================================================
--- linux-2.6.20-rc4-mm1.orig/include/linux/aio_abi.h	2007-01-17 08:58:02.000000000 +0100
+++ linux-2.6.20-rc4-mm1/include/linux/aio_abi.h	2007-01-17 08:59:16.000000000 +0100
@@ -45,6 +45,11 @@ enum {
 	IOCB_CMD_PWRITEV = 8,
 };
 
+enum {
+	LIO_WAIT = 0,
+	LIO_NOWAIT = 1,
+};
+
 /* read() from /dev/aio returns these structures. */
 struct io_event {
 	__u64		data;		/* the data field from the iocb */
Index: linux-2.6.20-rc4-mm1/include/linux/aio.h
===================================================================
--- linux-2.6.20-rc4-mm1.orig/include/linux/aio.h	2007-01-17 08:58:02.000000000 +0100
+++ linux-2.6.20-rc4-mm1/include/linux/aio.h	2007-01-17 08:59:16.000000000 +0100
@@ -63,6 +63,11 @@ struct aio_notify {
 	struct sigqueue		*sigq;
 };
 
+struct lio_event {
+	atomic_t		lio_users;
+	struct aio_notify	lio_notify;
+};
+
 /* is there a better place to document function pointer methods? */
 /**
  * ki_retry	-	iocb forward progress callback
@@ -117,6 +122,8 @@ struct kiocb {
 	__u64			ki_user_data;	/* user's data for completion */
 	struct wait_bit_queue	ki_wait;
 	loff_t			ki_pos;
+	/* lio this iocb might be attached to */
+	struct lio_event	*ki_lio;
 
 	atomic_t		ki_bio_count;	/* num bio used for this iocb */
 	void			*private;
@@ -225,12 +232,15 @@ struct mm_struct;
 extern void FASTCALL(exit_aio(struct mm_struct *mm));
 extern struct kioctx *lookup_ioctx(unsigned long ctx_id);
 extern int FASTCALL(io_submit_one(struct kioctx *ctx,
-			struct iocb __user *user_iocb, struct iocb *iocb));
+			struct iocb __user *user_iocb, struct iocb *iocb,
+			struct lio_event *lio));
+struct lio_event *lio_create(struct sigevent __user *user_event, int mode);
+void lio_check(struct lio_event *lio);
 
 /* semi private, but used by the 32bit emulations: */
 struct kioctx *lookup_ioctx(unsigned long ctx_id);
 int FASTCALL(io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
-				  struct iocb *iocb));
+				  struct iocb *iocb, struct lio_event *lio));
 
 #define get_ioctx(kioctx) do {						\
 	BUG_ON(atomic_read(&(kioctx)->users) <= 0);			\
Index: linux-2.6.20-rc4-mm1/include/linux/syscalls.h
===================================================================
--- linux-2.6.20-rc4-mm1.orig/include/linux/syscalls.h	2007-01-17 08:58:02.000000000 +0100
+++ linux-2.6.20-rc4-mm1/include/linux/syscalls.h	2007-01-17 08:59:16.000000000 +0100
@@ -317,6 +317,8 @@ asmlinkage long sys_io_getevents(aio_con
 				struct timespec __user *timeout);
 asmlinkage long sys_io_submit(aio_context_t, long,
 				struct iocb __user * __user *);
+asmlinkage long sys_lio_submit(aio_context_t, int, long,
+		struct iocb __user * __user *, struct sigevent __user *);
 asmlinkage long sys_io_cancel(aio_context_t ctx_id, struct iocb __user *iocb,
 			      struct io_event __user *result);
 asmlinkage ssize_t sys_sendfile(int out_fd, int in_fd,
Index: linux-2.6.20-rc4-mm1/include/asm-x86_64/unistd.h
===================================================================
--- linux-2.6.20-rc4-mm1.orig/include/asm-x86_64/unistd.h	2007-01-17 08:58:02.000000000 +0100
+++ linux-2.6.20-rc4-mm1/include/asm-x86_64/unistd.h	2007-01-17 08:59:16.000000000 +0100
@@ -619,8 +619,10 @@ __SYSCALL(__NR_sync_file_range, sys_sync
 __SYSCALL(__NR_vmsplice, sys_vmsplice)
 #define __NR_move_pages		279
 __SYSCALL(__NR_move_pages, sys_move_pages)
+#define __NR_lio_submit		280
+__SYSCALL(__NR_lio_submit, sys_lio_submit)
 
-#define __NR_syscall_max __NR_move_pages
+#define __NR_syscall_max __NR_lio_submit
 
 #ifndef __NO_STUBS
 #define __ARCH_WANT_OLD_READDIR
Index: linux-2.6.20-rc4-mm1/arch/x86_64/ia32/ia32entry.S
===================================================================
--- linux-2.6.20-rc4-mm1.orig/arch/x86_64/ia32/ia32entry.S	2007-01-17 08:58:02.000000000 +0100
+++ linux-2.6.20-rc4-mm1/arch/x86_64/ia32/ia32entry.S	2007-01-17 08:59:16.000000000 +0100
@@ -714,8 +714,10 @@ ia32_sys_call_table:
 	.quad compat_sys_get_robust_list
 	.quad sys_splice
 	.quad sys_sync_file_range
-	.quad sys_tee
+	.quad sys_tee			/* 315 */
 	.quad compat_sys_vmsplice
 	.quad compat_sys_move_pages
 	.quad sys_getcpu
+	.quad quiet_ni_syscall		/* sys_epoll_wait */
+	.quad compat_sys_lio_submit
 ia32_syscall_end:		

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 4/5][AIO] - AIO completion signal notification
  2007-01-17  9:50 ` [PATCH -mm 4/5][AIO] - AIO completion signal notification Sébastien Dugué
@ 2007-01-24  5:35   ` Andrew Morton
  2007-01-24 11:11     ` Sébastien Dugué
  0 siblings, 1 reply; 22+ messages in thread
From: Andrew Morton @ 2007-01-24  5:35 UTC (permalink / raw)
  To: Sébastien Dugué
  Cc: linux-kernel, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty

On Wed, 17 Jan 2007 10:50:18 +0100
Sébastien Dugué <sebastien.dugue@bull.net> wrote:

> +static long aio_setup_sigevent(struct aio_notify *notify,
> +			       struct sigevent __user *user_event)
> +{
> +	sigevent_t event;
> +	struct task_struct *target;
> +
> +	if (copy_from_user(&event, user_event, sizeof (event)))
> +		return -EFAULT;
> +
> +	if (event.sigev_notify == SIGEV_NONE)
> +		return 0;
> +
> +	notify->notify = event.sigev_notify;
> +	notify->signo = event.sigev_signo;
> +	notify->value = event.sigev_value;
> +
> +	read_lock(&tasklist_lock);
> +	target = good_sigevent(&event);
> +
> +	if (unlikely(!target || (target->flags & PF_EXITING)))
> +		goto out_unlock;
> +
> +	/*
> +	 * At this point, we know that notify is either SIGEV_SIGNAL or
> +	 * SIGEV_THREAD_ID and the target task is valid. So get a reference
> +	 * on the task, it will be dropped in really_put_req() when
> +	 * we're done with the request.
> +	 */
> +	get_task_struct(target);
> +	notify->target = target;
> +	read_unlock(&tasklist_lock);
> +
> +	/*
> +	 * NOTE: we cannot free the sigqueue in the completion path as
> +	 * the signal may not have been delivered to the target task.
> +	 * Therefore it has to be freed in __sigqueue_free() when the
> +	 * signal is collected if si_code is SI_ASYNCIO.
> +	 */
> +	notify->sigq = sigqueue_alloc();
> +
> +	if (unlikely(!notify->sigq))
> +		return -EAGAIN;

Did this just leak a ref on the task_struct?

> +
> +	return 0;
> +
> +out_unlock:
> +	read_unlock(&tasklist_lock);
> +	return -EINVAL;
> +}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 5/5][AIO] - Add listio syscall support
  2007-01-17  9:55 ` [PATCH -mm 5/5][AIO] - Add listio syscall support Sébastien Dugué
@ 2007-01-24  5:50   ` Andrew Morton
  2007-01-24 11:51     ` Sébastien Dugué
  2007-01-24  5:53   ` Andrew Morton
  2007-01-24  6:04   ` Andrew Morton
  2 siblings, 1 reply; 22+ messages in thread
From: Andrew Morton @ 2007-01-24  5:50 UTC (permalink / raw)
  To: Sébastien Dugué
  Cc: linux-kernel, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty

On Wed, 17 Jan 2007 10:55:54 +0100
Sébastien Dugué <sebastien.dugue@bull.net> wrote:

> +asmlinkage long
> +compat_sys_lio_submit(aio_context_t ctx_id, int mode, int nr, u32 __user *iocb,
> +		struct compat_sigevent __user *sig_user)
> +{
> +	struct kioctx *ctx;
> +	struct lio_event *lio = NULL;
> +	struct sigevent __user *event = NULL;
> +	long ret = 0;
> +
> +	if (unlikely(nr < 0))
> +		return -EINVAL;
> +
> +	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
> +		return -EFAULT;
> +
> +	ctx = lookup_ioctx(ctx_id);
> +	if (unlikely(!ctx))
> +		return -EINVAL;
> +
> +	if (sig_user) {
> +		struct sigevent kevent;
> +		event = compat_alloc_user_space(sizeof(struct sigevent));
> +		if (get_compat_sigevent(&kevent, sig_user) ||
> +			copy_to_user(event, &kevent, sizeof(struct sigevent)))
> +			return -EFAULT;

I think we just leaked a ref against the ioctx.

That's two.  Please re-review the whole patchset for leaks like this.

Please also do not do returns from the middle of functions like this.  It's just
asking for resource leaks, either now or in the future.


> +	}
> +
> +	lio = lio_create(event, mode);
> +
> +	ret = PTR_ERR(lio);
> +	if (IS_ERR(lio))
> +		goto out_put_ctx;
> +
> +	ret = compat_io_submit_group(ctx, nr, iocb, lio);
> +
> +	/* If we failed to submit even one request just return */
> +	if (ret < 0) {
> +		if (lio)
> +			kfree(lio);
> +		goto out_put_ctx;
> +	}
> +
> +	/*
> +	 * Drop extra ref on the lio now that we're done submitting requests.
> +	 */
> +	if (lio)
> +		lio_check(lio);
> +
> +
> +	if (mode == LIO_WAIT) {
> +		wait_event(ctx->wait, atomic_read(&lio->lio_users) == 0);
> +		kfree(lio);
> +	}
> +out_put_ctx:
>  	put_ioctx(ctx);
> -	return i ? i: ret;
> +	return ret;
>  }

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 5/5][AIO] - Add listio syscall support
  2007-01-17  9:55 ` [PATCH -mm 5/5][AIO] - Add listio syscall support Sébastien Dugué
  2007-01-24  5:50   ` Andrew Morton
@ 2007-01-24  5:53   ` Andrew Morton
  2007-01-24  9:52     ` Laurent Vivier
  2007-01-24  6:04   ` Andrew Morton
  2 siblings, 1 reply; 22+ messages in thread
From: Andrew Morton @ 2007-01-24  5:53 UTC (permalink / raw)
  To: Sébastien Dugué
  Cc: linux-kernel, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty

On Wed, 17 Jan 2007 10:55:54 +0100
Sébastien Dugué <sebastien.dugue@bull.net> wrote:

> +struct lio_event *lio_create(struct sigevent __user *user_event,
> +			int mode)
> +{
> +	int ret = 0;
> +	struct lio_event *lio = NULL;
> +
> +	if (unlikely((mode == LIO_NOWAIT) && !user_event))
> +		return lio;
> +
> +	lio = kzalloc(sizeof(*lio), GFP_KERNEL);
> +
> +	if (!lio)
> +		return ERR_PTR(-EAGAIN);
> +

Why EAGAIN and not ENOMEM?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 5/5][AIO] - Add listio syscall support
  2007-01-17  9:55 ` [PATCH -mm 5/5][AIO] - Add listio syscall support Sébastien Dugué
  2007-01-24  5:50   ` Andrew Morton
  2007-01-24  5:53   ` Andrew Morton
@ 2007-01-24  6:04   ` Andrew Morton
  2007-01-24 12:02     ` Sébastien Dugué
  2007-01-24 18:16     ` Bharata B Rao
  2 siblings, 2 replies; 22+ messages in thread
From: Andrew Morton @ 2007-01-24  6:04 UTC (permalink / raw)
  To: Sébastien Dugué
  Cc: linux-kernel, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty

On Wed, 17 Jan 2007 10:55:54 +0100
Sébastien Dugué <sebastien.dugue@bull.net> wrote:

> +void lio_check(struct lio_event *lio)
> +{
> +	int ret;
> +
> +	ret = atomic_dec_and_test(&lio->lio_users);
> +
> +	if (unlikely(ret) && lio->lio_notify.notify != SIGEV_NONE) {
> +		/* last one -> notify process */
> +		if (aio_send_signal(&lio->lio_notify))
> +			sigqueue_free(lio->lio_notify.sigq);
> +		kfree(lio);
> +	}
> +}

That's a scary function.  It may (or may not) free the memory at lio,
returning no indication to the caller whether or not that memory is still
allocated.  This is most peculiar - are you really sure there's no
potential for a use-after-free here?

The function is poorly named: I'd expect something called "foo_check" to
not have any side-effects.  This one has gross side-effects.  Want to think
up a better name, please?

And given that this function has global scope, perhaps a little explanatory
comment is in order?

> +struct lio_event *lio_create(struct sigevent __user *user_event,
> +			int mode)

Here too.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 5/5][AIO] - Add listio syscall support
  2007-01-24  5:53   ` Andrew Morton
@ 2007-01-24  9:52     ` Laurent Vivier
  2007-01-24 11:53       ` Sébastien Dugué
  0 siblings, 1 reply; 22+ messages in thread
From: Laurent Vivier @ 2007-01-24  9:52 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Sébastien Dugué,
	linux-kernel, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty

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

Andrew Morton wrote:
> On Wed, 17 Jan 2007 10:55:54 +0100
> Sébastien Dugué <sebastien.dugue@bull.net> wrote:
> 
>> +struct lio_event *lio_create(struct sigevent __user *user_event,
>> +			int mode)
>> +{
>> +	int ret = 0;
>> +	struct lio_event *lio = NULL;
>> +
>> +	if (unlikely((mode == LIO_NOWAIT) && !user_event))
>> +		return lio;
>> +
>> +	lio = kzalloc(sizeof(*lio), GFP_KERNEL);
>> +
>> +	if (!lio)
>> +		return ERR_PTR(-EAGAIN);
>> +
> 
> Why EAGAIN and not ENOMEM?

According to the POSIX AIO specifications:

"ERRORS:
...
[EAGAIN]
    The resources necessary to queue all the I/O requests were not available. "

I think memory is "a resource necessary to queue all the I/O requests"...

http://www.opengroup.org/onlinepubs/009695399/functions/lio_listio.html

Regards,
Laurent
-- 
------------- Laurent.Vivier@bull.net  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 4/5][AIO] - AIO completion signal notification
  2007-01-24  5:35   ` Andrew Morton
@ 2007-01-24 11:11     ` Sébastien Dugué
  2007-01-25  5:42       ` Christoph Hellwig
  0 siblings, 1 reply; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-24 11:11 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty

On Tue, 23 Jan 2007 21:35:13 -0800 Andrew Morton <akpm@osdl.org> wrote:

> On Wed, 17 Jan 2007 10:50:18 +0100
> Sébastien Dugué <sebastien.dugue@bull.net> wrote:
> 
> > +static long aio_setup_sigevent(struct aio_notify *notify,
> > +			       struct sigevent __user *user_event)
> > +{
> > +	sigevent_t event;
> > +	struct task_struct *target;
> > +
> > +	if (copy_from_user(&event, user_event, sizeof (event)))
> > +		return -EFAULT;
> > +
> > +	if (event.sigev_notify == SIGEV_NONE)
> > +		return 0;
> > +
> > +	notify->notify = event.sigev_notify;
> > +	notify->signo = event.sigev_signo;
> > +	notify->value = event.sigev_value;
> > +
> > +	read_lock(&tasklist_lock);
> > +	target = good_sigevent(&event);
> > +
> > +	if (unlikely(!target || (target->flags & PF_EXITING)))
> > +		goto out_unlock;
> > +
> > +	/*
> > +	 * At this point, we know that notify is either SIGEV_SIGNAL or
> > +	 * SIGEV_THREAD_ID and the target task is valid. So get a reference
> > +	 * on the task, it will be dropped in really_put_req() when
> > +	 * we're done with the request.
> > +	 */
> > +	get_task_struct(target);
> > +	notify->target = target;
> > +	read_unlock(&tasklist_lock);
> > +
> > +	/*
> > +	 * NOTE: we cannot free the sigqueue in the completion path as
> > +	 * the signal may not have been delivered to the target task.
> > +	 * Therefore it has to be freed in __sigqueue_free() when the
> > +	 * signal is collected if si_code is SI_ASYNCIO.
> > +	 */
> > +	notify->sigq = sigqueue_alloc();
> > +
> > +	if (unlikely(!notify->sigq))
> > +		return -EAGAIN;
> 
> Did this just leak a ref on the task_struct?
> 

  No, the ref is released in really_put_req() when we dispose of
the iocb.

  Thanks,

  Sébastien.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 5/5][AIO] - Add listio syscall support
  2007-01-24  5:50   ` Andrew Morton
@ 2007-01-24 11:51     ` Sébastien Dugué
  0 siblings, 0 replies; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-24 11:51 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty

On Tue, 23 Jan 2007 21:50:47 -0800 Andrew Morton <akpm@osdl.org> wrote:

> On Wed, 17 Jan 2007 10:55:54 +0100
> Sébastien Dugué <sebastien.dugue@bull.net> wrote:
> 
> > +asmlinkage long
> > +compat_sys_lio_submit(aio_context_t ctx_id, int mode, int nr, u32 __user *iocb,
> > +		struct compat_sigevent __user *sig_user)
> > +{
> > +	struct kioctx *ctx;
> > +	struct lio_event *lio = NULL;
> > +	struct sigevent __user *event = NULL;
> > +	long ret = 0;
> > +
> > +	if (unlikely(nr < 0))
> > +		return -EINVAL;
> > +
> > +	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
> > +		return -EFAULT;
> > +
> > +	ctx = lookup_ioctx(ctx_id);
> > +	if (unlikely(!ctx))
> > +		return -EINVAL;
> > +
> > +	if (sig_user) {
> > +		struct sigevent kevent;
> > +		event = compat_alloc_user_space(sizeof(struct sigevent));
> > +		if (get_compat_sigevent(&kevent, sig_user) ||
> > +			copy_to_user(event, &kevent, sizeof(struct sigevent)))
> > +			return -EFAULT;
> 
> I think we just leaked a ref against the ioctx.

  Right, will fix.

> 
> That's two.  Please re-review the whole patchset for leaks like this.

  Well, this particular patch did not have much review so far (contrary to the
the other patches in the patchset). That's why I posted it mainly
for comments and not expecting it to make it to -mm so soon.

> 
> Please also do not do returns from the middle of functions like this.  It's just
> asking for resource leaks, either now or in the future.
> 

  Argh, yes. Who wrote that code? ;-)

  Thanks,

  Sébastien.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 5/5][AIO] - Add listio syscall support
  2007-01-24  9:52     ` Laurent Vivier
@ 2007-01-24 11:53       ` Sébastien Dugué
  0 siblings, 0 replies; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-24 11:53 UTC (permalink / raw)
  To: Laurent Vivier
  Cc: Andrew Morton, linux-kernel, linux-aio, Bharata B Rao,
	Christoph Hellwig, Suparna Bhattacharya, Ulrich Drepper,
	Zach Brown, Jean Pierre Dion, Badari Pulavarty

On Wed, 24 Jan 2007 10:52:58 +0100 Laurent Vivier <Laurent.Vivier@bull.net> wrote:

> Andrew Morton wrote:
> > On Wed, 17 Jan 2007 10:55:54 +0100
> > Sébastien Dugué <sebastien.dugue@bull.net> wrote:
> > 
> >> +struct lio_event *lio_create(struct sigevent __user *user_event,
> >> +			int mode)
> >> +{
> >> +	int ret = 0;
> >> +	struct lio_event *lio = NULL;
> >> +
> >> +	if (unlikely((mode == LIO_NOWAIT) && !user_event))
> >> +		return lio;
> >> +
> >> +	lio = kzalloc(sizeof(*lio), GFP_KERNEL);
> >> +
> >> +	if (!lio)
> >> +		return ERR_PTR(-EAGAIN);
> >> +
> > 
> > Why EAGAIN and not ENOMEM?
> 
> According to the POSIX AIO specifications:
> 
> "ERRORS:
> ...
> [EAGAIN]
>     The resources necessary to queue all the I/O requests were not available. "
> 
> I think memory is "a resource necessary to queue all the I/O requests"...
> 
> http://www.opengroup.org/onlinepubs/009695399/functions/lio_listio.html
> 
> Regards,
> Laurent

  Thanks Laurent,

  just forgot the whys here.

  Sébastien.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 5/5][AIO] - Add listio syscall support
  2007-01-24  6:04   ` Andrew Morton
@ 2007-01-24 12:02     ` Sébastien Dugué
  2007-01-24 18:16     ` Bharata B Rao
  1 sibling, 0 replies; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-24 12:02 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, linux-aio, Bharata B Rao, Christoph Hellwig,
	Suparna Bhattacharya, Ulrich Drepper, Zach Brown,
	Jean Pierre Dion, Badari Pulavarty

On Tue, 23 Jan 2007 22:04:33 -0800 Andrew Morton <akpm@osdl.org> wrote:

> On Wed, 17 Jan 2007 10:55:54 +0100
> Sébastien Dugué <sebastien.dugue@bull.net> wrote:
> 
> > +void lio_check(struct lio_event *lio)
> > +{
> > +	int ret;
> > +
> > +	ret = atomic_dec_and_test(&lio->lio_users);
> > +
> > +	if (unlikely(ret) && lio->lio_notify.notify != SIGEV_NONE) {
> > +		/* last one -> notify process */
> > +		if (aio_send_signal(&lio->lio_notify))
> > +			sigqueue_free(lio->lio_notify.sigq);
> > +		kfree(lio);
> > +	}
> > +}
> 
> That's a scary function.  It may (or may not) free the memory at lio,
> returning no indication to the caller whether or not that memory is still
> allocated.  This is most peculiar - are you really sure there's no
> potential for a use-after-free here?

  Right again, this patch definitely needs more eyes peering over.

> 
> The function is poorly named: I'd expect something called "foo_check" to
> not have any side-effects.  This one has gross side-effects.  Want to think
> up a better name, please?
> 
> And given that this function has global scope, perhaps a little explanatory
> comment is in order?
> 
> > +struct lio_event *lio_create(struct sigevent __user *user_event,
> > +			int mode)
> 
> Here too.

  OK, will look into this. In the meantime, maybe you should drop this one
patch entirely.

  Thanks,

  Sébastien.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 5/5][AIO] - Add listio syscall support
  2007-01-24  6:04   ` Andrew Morton
  2007-01-24 12:02     ` Sébastien Dugué
@ 2007-01-24 18:16     ` Bharata B Rao
  1 sibling, 0 replies; 22+ messages in thread
From: Bharata B Rao @ 2007-01-24 18:16 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Sébastien Dugué,
	linux-kernel, linux-aio, Christoph Hellwig, Suparna Bhattacharya,
	Ulrich Drepper, Zach Brown, Jean Pierre Dion, Badari Pulavarty

On Tue, Jan 23, 2007 at 10:04:33PM -0800, Andrew Morton wrote:
> On Wed, 17 Jan 2007 10:55:54 +0100
> Sébastien Dugué <sebastien.dugue@bull.net> wrote:
> 
> > +void lio_check(struct lio_event *lio)
> > +{
> > +	int ret;
> > +
> > +	ret = atomic_dec_and_test(&lio->lio_users);
> > +
> > +	if (unlikely(ret) && lio->lio_notify.notify != SIGEV_NONE) {
> > +		/* last one -> notify process */
> > +		if (aio_send_signal(&lio->lio_notify))
> > +			sigqueue_free(lio->lio_notify.sigq);
> > +		kfree(lio);
> > +	}
> > +}
> 
> That's a scary function.  It may (or may not) free the memory at lio,
> returning no indication to the caller whether or not that memory is still
> allocated.  This is most peculiar - are you really sure there's no
> potential for a use-after-free here?

Yes, this function looks peculiar. Actually lio gets freed here only
for LIO_NOWAIT case. For LIO_WAIT case, it gets freed at the end
of sys_lio_submit() after it is done waiting for all io's.
But yes, all this is not very obvious.

> 
> The function is poorly named: I'd expect something called "foo_check" to
> not have any side-effects.  This one has gross side-effects.  Want to think
> up a better name, please?
> 
> And given that this function has global scope, perhaps a little explanatory
> comment is in order?
> 
> > +struct lio_event *lio_create(struct sigevent __user *user_event,
> > +			int mode)
> 
> Here too.

Ok, will try to take care of all these in the next iteration.

Thanks for your review.

Regards,
Bharata.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 4/5][AIO] - AIO completion signal notification
  2007-01-24 11:11     ` Sébastien Dugué
@ 2007-01-25  5:42       ` Christoph Hellwig
  2007-01-25  8:46         ` Sébastien Dugué
  0 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2007-01-25  5:42 UTC (permalink / raw)
  To: S?bastien Dugu?
  Cc: Andrew Morton, linux-kernel, linux-aio, Bharata B Rao,
	Christoph Hellwig, Suparna Bhattacharya, Ulrich Drepper,
	Zach Brown, Jean Pierre Dion, Badari Pulavarty

On Wed, Jan 24, 2007 at 12:11:30PM +0100, S?bastien Dugu? wrote:
> > > +	if (unlikely(!notify->sigq))
> > > +		return -EAGAIN;
> > 
> > Did this just leak a ref on the task_struct?
> > 
> 
>   No, the ref is released in really_put_req() when we dispose of
> the iocb.

And the code really needs a comment explaining this.  I tripped over
this before, and I think it's even already the second time Andrew
stumbled over it.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 4/5][AIO] - AIO completion signal notification
  2007-01-25  5:42       ` Christoph Hellwig
@ 2007-01-25  8:46         ` Sébastien Dugué
  0 siblings, 0 replies; 22+ messages in thread
From: Sébastien Dugué @ 2007-01-25  8:46 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Andrew Morton, linux-kernel, linux-aio, Bharata B Rao,
	Christoph Hellwig, Suparna Bhattacharya, Ulrich Drepper,
	Zach Brown, Jean Pierre Dion, Badari Pulavarty

On Thu, 25 Jan 2007 05:42:42 +0000 Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, Jan 24, 2007 at 12:11:30PM +0100, S?bastien Dugu? wrote:
> > > > +	if (unlikely(!notify->sigq))
> > > > +		return -EAGAIN;
> > > 
> > > Did this just leak a ref on the task_struct?
> > > 
> > 
> >   No, the ref is released in really_put_req() when we dispose of
> > the iocb.
> 
> And the code really needs a comment explaining this.  I tripped over
> this before, and I think it's even already the second time Andrew
> stumbled over it.
> 

  OK, will do.

  Thanks,

  Sébastien.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit
  2006-11-30  9:57     ` Sébastien Dugué
@ 2006-11-30 17:27       ` Zach Brown
  0 siblings, 0 replies; 22+ messages in thread
From: Zach Brown @ 2006-11-30 17:27 UTC (permalink / raw)
  To: Sébastien Dugué
  Cc: linux-kernel, linux-aio, Andrew Morton, Suparna Bhattacharya,
	Christoph Hellwig, Badari Pulavarty, Ulrich Drepper,
	Jean Pierre Dion

>>
>> sys_io_getevents() reads:
>
>  uh!     ^^^^^^^^^    you must be meaning sys_io_submit()?

Heh, yes, of course.  Damn these fingers!

- z

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit
  2006-11-30 15:38 [PATCH -mm 0/5][AIO] - AIO completion signal notification v4 Sébastien Dugué
@ 2006-11-30 15:49 ` Sébastien Dugué
  0 siblings, 0 replies; 22+ messages in thread
From: Sébastien Dugué @ 2006-11-30 15:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: Christoph Hellwig, linux-aio, Andrew Morton,
	Suparna Bhattacharya, Zach Brown, Badari Pulavarty,
	Ulrich Drepper, Jean Pierre Dion, Bharata B Rao


                 compat_sys_io_submit() cleanup


  Cleanup compat_sys_io_submit by duplicating some of the native syscall
logic in the compat layer and directly calling io_submit_one() instead
of fooling the syscall into thinking it is called from a native 64-bit
caller.

  This eliminates:

  - the overhead of copying the nr iocb pointers on the userspace stack

  - the PAGE_SIZE/(sizeof(void *)) limit on the number of iocbs that
    can be submitted.

  This is also needed for the completion notification patch to avoid having
to rewrite each iocb on the caller stack for io_submit_one() to find the
sigevents.


 compat.c |   61 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

Signed-off-by: Sébastien Dugué <sebastien.dugue@bull.net>

Index: linux-2.6.19-rc6-mm2/fs/compat.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/fs/compat.c	2006-11-30 10:00:18.000000000 +0100
+++ linux-2.6.19-rc6-mm2/fs/compat.c	2006-11-30 13:12:32.000000000 +0100
@@ -642,40 +642,47 @@ out:
 	return ret;
 }
 
-static inline long
-copy_iocb(long nr, u32 __user *ptr32, struct iocb __user * __user *ptr64)
-{
-	compat_uptr_t uptr;
-	int i;
-
-	for (i = 0; i < nr; ++i) {
-		if (get_user(uptr, ptr32 + i))
-			return -EFAULT;
-		if (put_user(compat_ptr(uptr), ptr64 + i))
-			return -EFAULT;
-	}
-	return 0;
-}
-
-#define MAX_AIO_SUBMITS 	(PAGE_SIZE/sizeof(struct iocb *))
-
 asmlinkage long
 compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)
 {
-	struct iocb __user * __user *iocb64; 
-	long ret;
+	struct kioctx *ctx;
+	long ret = 0;
+	int i;
 
 	if (unlikely(nr < 0))
 		return -EINVAL;
 
-	if (nr > MAX_AIO_SUBMITS)
-		nr = MAX_AIO_SUBMITS;
-	
-	iocb64 = compat_alloc_user_space(nr * sizeof(*iocb64));
-	ret = copy_iocb(nr, iocb, iocb64);
-	if (!ret)
-		ret = sys_io_submit(ctx_id, nr, iocb64);
-	return ret;
+	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
+		return -EFAULT;
+
+	ctx = lookup_ioctx(ctx_id);
+	if (unlikely(!ctx))
+		return -EINVAL;
+
+	for (i=0; i<nr; i++) {
+		compat_uptr_t uptr;
+		struct iocb __user *user_iocb;
+		struct iocb tmp;
+
+		if (unlikely(get_user(uptr, iocb + i))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		user_iocb = compat_ptr(uptr);
+
+		if (unlikely(copy_from_user(&tmp, user_iocb, sizeof(tmp)))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		ret = io_submit_one(ctx, user_iocb, &tmp);
+		if (ret)
+			break;
+	}
+
+	put_ioctx(ctx);
+	return i ? i: ret;
 }
 
 struct compat_ncp_mount_data {

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit
  2006-11-30  0:47   ` Zach Brown
@ 2006-11-30  9:57     ` Sébastien Dugué
  2006-11-30 17:27       ` Zach Brown
  0 siblings, 1 reply; 22+ messages in thread
From: Sébastien Dugué @ 2006-11-30  9:57 UTC (permalink / raw)
  To: Zach Brown
  Cc: linux-kernel, linux-aio, Andrew Morton, Suparna Bhattacharya,
	Christoph Hellwig, Badari Pulavarty, Ulrich Drepper,
	Jean Pierre Dion

On Wed, 29 Nov 2006 16:47:47 -0800, Zach Brown <zach.brown@oracle.com> wrote:

> On Nov 29, 2006, at 2:32 AM, Sébastien Dugué wrote:
> 
> >                  compat_sys_io_submit() cleanup
> >
> >
> >   Cleanup compat_sys_io_submit by duplicating some of the native  
> > syscall
> > logic in the compat layer and directly calling io_submit_one() instead
> > of fooling the syscall into thinking it is called from a native 64-bit
> > caller.
> >
> >   This is needed for the completion notification patch to avoid having
> > to rewrite each iocb on the caller stack for sys_io_submit() to  
> > find the
> > sigevents.
> 
> You could explicitly mention that this eliminates:
> 
>   - the overhead of copying nr pointers on the userspace caller's stack
> 
>   - the arbitrary PAGE_SIZE/(sizeof(void *)) limit on the number of  
> iocbs that can be submitted
> 
> Those alone make this worth merging.

  Right, will add.

> 
> > +	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
> > +		return -EFAULT;
> 
> I'm glad you got that right :)  I no doubt would have initially  
> hoisted these little checks into a shared helper function and missed  
> that detail of getting the size of the access_ok() right in the  
> compat case.

  Thanks.

> 
> > +	put_ioctx(ctx);
> > +
> > +	return i? i: ret;
> 
> sys_io_getevents() reads:

 uh!     ^^^^^^^^^    you must be meaning sys_io_submit()?

> 
>          put_ioctx(ctx);
>          return i ? i : ret;
> 
> So while this compat_sys_io_submit() logic seems fine and I would be  
> comfortable with it landing as-is, I'd also appreciate it if we  
> didn't introduce differences between the two functions when it seems  
> just as easy to make them the same.  (That chunk is just one  
> example.  There's whitespace, missing unlikely()s, etc).
> 

  OK, will fix.

  Thanks,

  Sébastien.


  

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit
  2006-11-29 10:32 ` [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit Sébastien Dugué
@ 2006-11-30  0:47   ` Zach Brown
  2006-11-30  9:57     ` Sébastien Dugué
  0 siblings, 1 reply; 22+ messages in thread
From: Zach Brown @ 2006-11-30  0:47 UTC (permalink / raw)
  To: Sébastien Dugué
  Cc: linux-kernel, linux-aio, Andrew Morton, Suparna Bhattacharya,
	Christoph Hellwig, Badari Pulavarty, Ulrich Drepper,
	Jean Pierre Dion

On Nov 29, 2006, at 2:32 AM, Sébastien Dugué wrote:

>                  compat_sys_io_submit() cleanup
>
>
>   Cleanup compat_sys_io_submit by duplicating some of the native  
> syscall
> logic in the compat layer and directly calling io_submit_one() instead
> of fooling the syscall into thinking it is called from a native 64-bit
> caller.
>
>   This is needed for the completion notification patch to avoid having
> to rewrite each iocb on the caller stack for sys_io_submit() to  
> find the
> sigevents.

You could explicitly mention that this eliminates:

  - the overhead of copying nr pointers on the userspace caller's stack

  - the arbitrary PAGE_SIZE/(sizeof(void *)) limit on the number of  
iocbs that can be submitted

Those alone make this worth merging.

> +	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
> +		return -EFAULT;

I'm glad you got that right :)  I no doubt would have initially  
hoisted these little checks into a shared helper function and missed  
that detail of getting the size of the access_ok() right in the  
compat case.

> +	put_ioctx(ctx);
> +
> +	return i? i: ret;

sys_io_getevents() reads:

         put_ioctx(ctx);
         return i ? i : ret;

So while this compat_sys_io_submit() logic seems fine and I would be  
comfortable with it landing as-is, I'd also appreciate it if we  
didn't introduce differences between the two functions when it seems  
just as easy to make them the same.  (That chunk is just one  
example.  There's whitespace, missing unlikely()s, etc).

- z

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit
  2006-11-29 10:24 [PATCH -mm 0/5][AIO] - AIO completion signal notification v3 Sébastien Dugué
@ 2006-11-29 10:32 ` Sébastien Dugué
  2006-11-30  0:47   ` Zach Brown
  0 siblings, 1 reply; 22+ messages in thread
From: Sébastien Dugué @ 2006-11-29 10:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-aio, Andrew Morton, Suparna Bhattacharya,
	Christoph Hellwig, Zach Brown, Badari Pulavarty, Ulrich Drepper,
	Jean Pierre Dion


                 compat_sys_io_submit() cleanup


  Cleanup compat_sys_io_submit by duplicating some of the native syscall
logic in the compat layer and directly calling io_submit_one() instead
of fooling the syscall into thinking it is called from a native 64-bit
caller.

  This is needed for the completion notification patch to avoid having
to rewrite each iocb on the caller stack for sys_io_submit() to find the
sigevents.



 compat.c |   61 +++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 26 deletions(-)

Signed-off-by: Sébastien Dugué <sebastien.dugue@bull.net>

Index: linux-2.6.19-rc6-mm2/fs/compat.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/fs/compat.c	2006-11-28 12:49:40.000000000
+0100 +++ linux-2.6.19-rc6-mm2/fs/compat.c	2006-11-28 12:51:35.000000000
+0100 @@ -642,40 +642,49 @@ out:
 	return ret;
 }
 
-static inline long
-copy_iocb(long nr, u32 __user *ptr32, struct iocb __user * __user *ptr64)
+asmlinkage long
+compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)
 {
-	compat_uptr_t uptr;
+	struct kioctx *ctx;
+	long ret = 0;
 	int i;
 
-	for (i = 0; i < nr; ++i) {
-		if (get_user(uptr, ptr32 + i))
-			return -EFAULT;
-		if (put_user(compat_ptr(uptr), ptr64 + i))
-			return -EFAULT;
-	}
-	return 0;
-}
+	if (unlikely(nr < 0))
+		return -EINVAL;
 
-#define MAX_AIO_SUBMITS 	(PAGE_SIZE/sizeof(struct iocb *))
+	if (unlikely(!access_ok(VERIFY_READ, iocb, (nr * sizeof(u32)))))
+		return -EFAULT;
 
-asmlinkage long
-compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)
-{
-	struct iocb __user * __user *iocb64; 
-	long ret;
+	ctx = lookup_ioctx(ctx_id);
 
-	if (unlikely(nr < 0))
+	if (unlikely(!ctx))
 		return -EINVAL;
+	for (i=0; i<nr; i++) {
+		compat_uptr_t uptr;
+		struct iocb __user *user_iocb;
+		struct iocb tmp;
 
-	if (nr > MAX_AIO_SUBMITS)
-		nr = MAX_AIO_SUBMITS;
-	
-	iocb64 = compat_alloc_user_space(nr * sizeof(*iocb64));
-	ret = copy_iocb(nr, iocb, iocb64);
-	if (!ret)
-		ret = sys_io_submit(ctx_id, nr, iocb64);
-	return ret;
+		if (get_user(uptr, iocb + i)) {
+			ret = -EFAULT;
+			break;
+		}
+
+		user_iocb = compat_ptr(uptr);
+
+		if (unlikely(copy_from_user(&tmp, user_iocb, sizeof(tmp)))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		ret = io_submit_one(ctx, user_iocb, &tmp);
+
+		if (ret)
+			break;
+	}
+
+	put_ioctx(ctx);
+
+	return i? i: ret;
 }
 
 struct compat_ncp_mount_data {

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2007-01-25  8:47 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20070117104601.36b2ab18@frecb000686>
2007-01-17  9:48 ` [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit Sébastien Dugué
2007-01-17  9:48 ` [PATCH -mm 2/5][AIO] - fix aio.h includes Sébastien Dugué
2007-01-17  9:49 ` [PATCH -mm 3/5][AIO] - Make good_sigevent non-static Sébastien Dugué
2007-01-17  9:50 ` [PATCH -mm 4/5][AIO] - AIO completion signal notification Sébastien Dugué
2007-01-24  5:35   ` Andrew Morton
2007-01-24 11:11     ` Sébastien Dugué
2007-01-25  5:42       ` Christoph Hellwig
2007-01-25  8:46         ` Sébastien Dugué
2007-01-17  9:55 ` [PATCH -mm 5/5][AIO] - Add listio syscall support Sébastien Dugué
2007-01-24  5:50   ` Andrew Morton
2007-01-24 11:51     ` Sébastien Dugué
2007-01-24  5:53   ` Andrew Morton
2007-01-24  9:52     ` Laurent Vivier
2007-01-24 11:53       ` Sébastien Dugué
2007-01-24  6:04   ` Andrew Morton
2007-01-24 12:02     ` Sébastien Dugué
2007-01-24 18:16     ` Bharata B Rao
2006-11-30 15:38 [PATCH -mm 0/5][AIO] - AIO completion signal notification v4 Sébastien Dugué
2006-11-30 15:49 ` [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit Sébastien Dugué
  -- strict thread matches above, loose matches on Subject: below --
2006-11-29 10:24 [PATCH -mm 0/5][AIO] - AIO completion signal notification v3 Sébastien Dugué
2006-11-29 10:32 ` [PATCH -mm 1/5][AIO] - Rework compat_sys_io_submit Sébastien Dugué
2006-11-30  0:47   ` Zach Brown
2006-11-30  9:57     ` Sébastien Dugué
2006-11-30 17:27       ` Zach Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).