xnpipe: Racy input_handler business

Xenomai-core New Jan Kiszka 1 participant 2 messages Dec 16, 2008

Share & tools

Jan Kiszka

author

2008-12-16 13:35 UTC #0 lore raw

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.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2008-12-16 14:21 UTC #1 lore raw

PATCH 1/1 xnpipe: Fix racy callback handlers

Quoted 22 lines · Jan Kiszka

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.

Quoted 5 lines

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 <[email protected]>

---

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

ksrc/nucleus/pipe.c +42 ~~-54~~

index b592437..1f8ebf7 100644
@@ -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: