Restructure pipe_read in order to eliminate several race conditions.

Submitted by:	Matthew Dillon <dillon@apollo.backplane.com> and myself

1 files changed, 46 insertions, 68 deletions

diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index cb1cd3048e65..a7266eb91ce0 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
@@ -16,7 +16,7 @@
  * 4. Modifications may be freely made to this file if the above conditions
  *    are met.
  *
- * $Id: sys_pipe.c,v 1.50 1999/02/04 23:50:49 dillon Exp $
+ * $Id: sys_pipe.c,v 1.51 1999/04/04 21:41:15 dt Exp $
  */
 
 /*
@@ -327,11 +327,15 @@ pipe_read(fp, uio, cred, flags)
 {
 
 	struct pipe *rpipe = (struct pipe *) fp->f_data;
-	int error = 0;
+	int error;
 	int nread = 0;
 	u_int size;
 
 	++rpipe->pipe_busy;
+	error = pipelock(rpipe, 1);
+	if (error)
+		goto unlocked_error;
+
 	while (uio->uio_resid) {
 		/*
 		 * normal pipe buffer receive
@@ -342,11 +346,9 @@ pipe_read(fp, uio, cred, flags)
 				size = rpipe->pipe_buffer.cnt;
 			if (size > (u_int) uio->uio_resid)
 				size = (u_int) uio->uio_resid;
-			if ((error = pipelock(rpipe,1)) == 0) {
-				error = uiomove( &rpipe->pipe_buffer.buffer[rpipe->pipe_buffer.out], 
+
+			error = uiomove(&rpipe->pipe_buffer.buffer[rpipe->pipe_buffer.out],
 					size, uio);
-				pipeunlock(rpipe);
-			}
 			if (error) {
 				break;
 			}
@@ -355,21 +357,29 @@ pipe_read(fp, uio, cred, flags)
 				rpipe->pipe_buffer.out = 0;
 
 			rpipe->pipe_buffer.cnt -= size;
+
+			/*
+			 * If there is no more to read in the pipe, reset
+			 * its pointers to the beginning.  This improves
+			 * cache hit stats.
+			 */
+			if (rpipe->pipe_buffer.cnt == 0) {
+				rpipe->pipe_buffer.in = 0;
+				rpipe->pipe_buffer.out = 0;
+			}
 			nread += size;
 #ifndef PIPE_NODIRECT
 		/*
 		 * Direct copy, bypassing a kernel buffer.
 		 */
 		} else if ((size = rpipe->pipe_map.cnt) &&
-			(rpipe->pipe_state & PIPE_DIRECTW)) {
-			caddr_t va;
+			   (rpipe->pipe_state & PIPE_DIRECTW)) {
+			caddr_t	va;
 			if (size > (u_int) uio->uio_resid)
 				size = (u_int) uio->uio_resid;
-			if ((error = pipelock(rpipe,1)) == 0) {
-				va = (caddr_t) rpipe->pipe_map.kva + rpipe->pipe_map.pos;
-				error = uiomove(va, size, uio);
-				pipeunlock(rpipe);
-			}
+
+			va = (caddr_t) rpipe->pipe_map.kva + rpipe->pipe_map.pos;
+			error = uiomove(va, size, uio);
 			if (error)
 				break;
 			nread += size;
@@ -382,32 +392,6 @@ pipe_read(fp, uio, cred, flags)
 #endif
 		} else {
 			/*
-			 * If there is no more to read in the pipe, reset
-			 * its pointers to the beginning.  This improves
-			 * cache hit stats.
-			 *
-			 * We get this over with now because it may block
-			 * and cause the state to change out from under us,
-			 * rather then have to re-test the state both before
-			 * and after this fragment.
-			 */
-		
-			if ((error = pipelock(rpipe,1)) == 0) {
-				if (rpipe->pipe_buffer.cnt == 0) {
-					rpipe->pipe_buffer.in = 0;
-					rpipe->pipe_buffer.out = 0;
-				}
-				pipeunlock(rpipe);
-
-				/*
-				 * If pipe filled up due to pipelock
-				 * blocking, loop back up.
-				 */
-				if (rpipe->pipe_buffer.cnt > 0)
-					continue;
-			}
-
-			/*
 			 * detect EOF condition
 			 */
 			if (rpipe->pipe_state & PIPE_EOF) {
@@ -424,55 +408,49 @@ pipe_read(fp, uio, cred, flags)
 			}
 
 			/*
-			 * break if error (signal via pipelock), or if some 
-			 * data was read
+			 * Break if some data was read.
 			 */
-			if (error || nread > 0)
+			if (nread > 0)
 				break;
 
 			/*
-			 * Handle non-blocking mode operation
+			 * Unlock the pipe buffer for our remaining processing.  We
+			 * will either break out with an error or we will sleep and
+			 * relock to loop.
 			 */
-
-			if (fp->f_flag & FNONBLOCK) {
-				error = EAGAIN;
-				break;
-			}
+			pipeunlock(rpipe);
 
 			/*
-			 * Wait for more data
+			 * Handle non-blocking mode operation or
+			 * wait for more data.
 			 */
-
-			rpipe->pipe_state |= PIPE_WANTR;
-			if ((error = tsleep(rpipe, PRIBIO|PCATCH, "piperd", 0)) != 0) {
-				break;
+			if (fp->f_flag & FNONBLOCK)
+				error = EAGAIN;
+			else {
+				rpipe->pipe_state |= PIPE_WANTR;
+				if ((error = tsleep(rpipe, PRIBIO|PCATCH, "piperd", 0)) == 0)
+					error = pipelock(rpipe, 1);
 			}
+			if (error)
+				goto unlocked_error;
 		}
 	}
+	pipeunlock(rpipe);
 
 	if (error == 0)
 		getnanotime(&rpipe->pipe_atime);
-
+unlocked_error:
 	--rpipe->pipe_busy;
+
+	/*
+	 * PIPE_WANT processing only makes sense if pipe_busy is 0.
+	 */
 	if ((rpipe->pipe_busy == 0) && (rpipe->pipe_state & PIPE_WANT)) {
 		rpipe->pipe_state &= ~(PIPE_WANT|PIPE_WANTW);
 		wakeup(rpipe);
 	} else if (rpipe->pipe_buffer.cnt < MINPIPESIZE) {
 		/*
-		 * If there is no more to read in the pipe, reset
-		 * its pointers to the beginning.  This improves
-		 * cache hit stats.
-		 */
-		if (rpipe->pipe_buffer.cnt == 0) {
-			if ((error == 0) && (error = pipelock(rpipe,1)) == 0) {
-				rpipe->pipe_buffer.in = 0;
-				rpipe->pipe_buffer.out = 0;
-				pipeunlock(rpipe);
-			}
-		}
-
-		/*
-		 * If the "write-side" has been blocked, wake it up now.
+		 * Handle write blocking hysteresis.
 		 */
 		if (rpipe->pipe_state & PIPE_WANTW) {
 			rpipe->pipe_state &= ~PIPE_WANTW;