[Xenomai-core] [PATCH] xnpipe: Fix racy callback handlers

[Jan Kiszka <jan.kiszka@domain.hid>]

Jan Kiszka wrote:
> Hi,
> 
> someone just dumped another kernel oops on my desk which points to the
> xnpipe subsystem: xnpipe_release was called, invoking
> __pipe_input_handler ie. the registered input_handler of the native pipe
> services. And that handler tried to call an invalid monitor callback
> (but no one ever set a monitor handler).
> 
> So I looked at how the nucleus deals with input_handler registration,
> deregistration, and invocation where it also passes some cookie that
> points to the native pipe object here. Looks like that code is racy /wrt
> concurrent cleanup of kernel and user side.
> 
> What is the intended locking policy when dereferencing the tuple of
> xnpipe_state_t.input_handler and xnpipe_state_t.cookie? Sometimes the
> handler is called under nklock, sometimes only both values are obtained
> and then nklock is dropped before invoking the handler (which is bogus
> as cookie may become invalid in the meantime). Even worse,
> xnpipe_release does not care at all about locking when calling
> input_handler as its last duty - and that's obviously where my customer
> just caught an oops.
> 
> In other words: Can't we always hold nklock while checking for
> input_handler != NULL and then invoking it with the corresponding
> cookie? This will just require us to properly clear input_handler on
> xnpipe_disconnect.
> 

Here is an attempt to fix the bugs. Compile-tested. The assumption is
still that we can (and must) call all handlers under nklock.

------------>

Invocation of input, output and alloc handler must take place under
nklock to properly synchronize with xnpipe_disconnect. Change all
callers to comply with this policy.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 ksrc/nucleus/pipe.c |   96 ++++++++++++++++++++++-----------------------------
 1 files changed, 42 insertions(+), 54 deletions(-)

diff --git a/ksrc/nucleus/pipe.c b/ksrc/nucleus/pipe.c
index b592437..1f8ebf7 100644
--- a/ksrc/nucleus/pipe.c
+++ b/ksrc/nucleus/pipe.c
@@ -331,6 +331,10 @@ int xnpipe_disconnect(int minor)
 
 	xnpipe_minor_free(minor);
 
+	state->output_handler = NULL;
+	state->input_handler = NULL;
+	state->alloc_handler = NULL;
+
 	xnlock_put_irqrestore(&nklock, s);
 
 	if (need_sched)
@@ -674,14 +678,14 @@ static int xnpipe_release(struct inode *inode, struct file *file)
 		}
 	}
 
-	xnlock_put_irqrestore(&nklock, s);
-
 	if (state->asyncq) {	/* Clear the async queue */
-		xnlock_get_irqsave(&nklock, s);
 		removeq(&xnpipe_asyncq, &state->alink);
 		__clrbits(state->status, XNPIPE_USER_SIGIO);
 		xnlock_put_irqrestore(&nklock, s);
+
 		fasync_helper(-1, file, 0, &state->asyncq);
+
+		xnlock_get_irqsave(&nklock, s);
 	}
 
 	xnpipe_cleanup_user_conn(state);
@@ -689,6 +693,9 @@ static int xnpipe_release(struct inode *inode, struct file *file)
 	if (state->input_handler)
 		state->input_handler(MINOR(inode->i_rdev),
 				     NULL, -EPIPE, state->cookie);
+
+	xnlock_put_irqrestore(&nklock, s);
+
 	return err;
 }
 
@@ -798,11 +805,9 @@ static ssize_t xnpipe_write(struct file *file,
 			    const char *buf, size_t count, loff_t *ppos)
 {
 	xnpipe_state_t *state = (xnpipe_state_t *)file->private_data;
-	xnpipe_alloc_handler *alloc_handler;
-	xnpipe_io_handler *input_handler;
 	struct xnpipe_mh *mh;
-	void *cookie;
 	int pollnum;
+	int err;
 	spl_t s;
 
 	if (count == 0)
@@ -818,19 +823,16 @@ static ssize_t xnpipe_write(struct file *file,
 		return -EPIPE;
 	}
 
-	alloc_handler = state->alloc_handler;
-	input_handler = state->input_handler;
-	cookie = state->cookie;
-
       retry:
 	pollnum = countq(&state->inq) + countq(&state->outq);
-	xnlock_put_irqrestore(&nklock, s);
 
-	if (alloc_handler)
+	if (state->alloc_handler) {
 		mh = (struct xnpipe_mh *)
-		    alloc_handler(xnminor_from_state(state),
-				  count + sizeof(*mh), cookie);
-	else {
+		    state->alloc_handler(xnminor_from_state(state),
+					 count + sizeof(*mh), state->cookie);
+		xnlock_put_irqrestore(&nklock, s);
+	} else {
+		xnlock_put_irqrestore(&nklock, s);
 		mh = (struct xnpipe_mh *)xnmalloc(count + sizeof(*mh));
 		if (mh == NULL &&
 		    count + sizeof(*mh) > xnheap_max_contiguous(&kheap))
@@ -858,17 +860,22 @@ static ssize_t xnpipe_write(struct file *file,
 	xnpipe_m_size(mh) = count;
 	xnpipe_m_rdoff(mh) = 0;
 
-	if (copy_from_user(xnpipe_m_data(mh), buf, count)) {
-		if (alloc_handler == NULL)
+	err = copy_from_user(xnpipe_m_data(mh), buf, count);
+
+	xnlock_get_irqsave(&nklock, s);
+
+	if (err) {
+		if (state->alloc_handler == NULL)
 			xnfree(mh);
-		else if (input_handler)
-			input_handler(xnminor_from_state(state), mh,
-				      -EFAULT, state->cookie);
+		else if (state->input_handler)
+			state->input_handler(xnminor_from_state(state), mh,
+					     -EFAULT, state->cookie);
+
+		xnlock_put_irqrestore(&nklock, s);
+
 		return -EFAULT;
 	}
 
-	xnlock_get_irqsave(&nklock, s);
-
 	appendq(&state->inq, &mh->link);
 
 	/* If a Xenomai thread is waiting on this input queue, wake it
@@ -878,26 +885,20 @@ static ssize_t xnpipe_write(struct file *file,
 	    xnsynch_wakeup_one_sleeper(&state->synchbase) != NULL)
 		xnpod_schedule();
 
-	xnlock_put_irqrestore(&nklock, s);
-
-	if (input_handler) {
-		int err =
-		    input_handler(xnminor_from_state(state), mh, 0, cookie);
-
-		if (err != 0)
+	if (state->input_handler) {
+		err = state->input_handler(xnminor_from_state(state), mh, 0,
+					   state->cookie);
+		if (err)
 			count = (size_t) err;
 	}
 
-	if (file->f_flags & O_SYNC) {
-		xnlock_get_irqsave(&nklock, s);
-		if (!emptyq_p(&state->inq)) {
-			if (xnpipe_wait(state, XNPIPE_USER_WSYNC, s,
-					emptyq_p(&state->inq)))
-				count = -ERESTARTSYS;
-		}
-		xnlock_put_irqrestore(&nklock, s);
+	if (file->f_flags & O_SYNC && !emptyq_p(&state->inq)) {
+		if (xnpipe_wait(state, XNPIPE_USER_WSYNC, s, 0))
+			count = -ERESTARTSYS;
 	}
 
+	xnlock_put_irqrestore(&nklock, s);
+
 	return (ssize_t) count;
 }
 
@@ -905,11 +906,9 @@ static int xnpipe_ioctl(struct inode *inode,
 			struct file *file, unsigned int cmd, unsigned long arg)
 {
 	xnpipe_state_t *state = (xnpipe_state_t *)file->private_data;
-	xnpipe_io_handler *io_handler;
 	int err = 0, n, minor;
 	struct xnpipe_mh *mh;
 	xnholder_t *holder;
-	void *cookie;
 	spl_t s;
 
 	minor = xnminor_from_state(state);
@@ -936,21 +935,16 @@ static int xnpipe_ioctl(struct inode *inode,
 		   design problem anyway. */
 
 		n = 0;
-		io_handler = state->output_handler;
-		cookie = state->cookie;
 
 		xnlock_get_irqsave(&nklock, s);
 
 		while ((holder = getq(&state->outq)) != NULL) {
-			xnlock_put_irqrestore(&nklock, s);
-
 			mh = link2mh(holder);
 			n += xnpipe_m_size(mh);
 
-			if (io_handler)
-				io_handler(minor, mh, 0, cookie);
-
-			xnlock_get_irqsave(&nklock, s);
+			if (state->output_handler)
+				state->output_handler(minor, mh, 0,
+						      state->cookie);
 		}
 
 		state->ionrd -= n;
@@ -959,23 +953,17 @@ static int xnpipe_ioctl(struct inode *inode,
 	case XNPIPEIOC_IFLUSH:
 
 		n = 0;
-		io_handler = state->input_handler;
-		cookie = state->cookie;
 
 		xnlock_get_irqsave(&nklock, s);
 
 		while ((holder = getq(&state->inq)) != NULL) {
-			xnlock_put_irqrestore(&nklock, s);
-
 			mh = link2mh(holder);
 			n += xnpipe_m_size(mh);
 
 			if (state->input_handler)
-				state->input_handler(minor, mh, 0, cookie);
+				state->input_handler(minor, mh, 0, state->cookie);
 			else if (state->alloc_handler == NULL)
 				xnfree(mh);
-
-			xnlock_get_irqsave(&nklock, s);
 		}
 
 	kick_wsync: