PERFORCE change 83418 for review

[Robert Watson]

http://perforce.freebsd.org/chv.cgi?CH=83418

Change 83418 by rwatson at rwatson_peppercorn on 2005/09/11 20:57:01

	Identify a number of minor and serious bugs in fifofs, and close
	some of them in an attempt to fix races and polling/select on
	fifos:
	
	- Add a run-time assertion that allocation of fifo state hasn't
	  raced with another thread doing the same.  In theory this
	  shouldn't happen due to vnode locking.
	
	- Use soconnect2() instead of reaching into the UNIX domain socket
	  internals using uipc_connect2().
	
	- Use sorwakeup() on the read endpoint of the socket pair, not the
	  write endpoint, or we wake up the wrong end.
	
	- Close two races: we must re-check the condition on wakeup and
	  re-sleep if other threads are also using the fifo and may have
	  been waiting simultaneously.  In particular, we have to do this
	  because we re-lock the vnode lock, which can sleep for extended
	  periods.
	
	- Check 'levents', not 'events' to decide whether or not to poll
	  for write state on a fifo: otherwise we poll the wrong socket
	  for read state if both read and write are requested, resulting
	  in general weirdness.

Affected files ...

.. //depot/projects/netsmp/src/sys/fs/fifofs/fifo_vnops.c#2 edit

Differences ...

==== //depot/projects/netsmp/src/sys/fs/fifofs/fifo_vnops.c#2 (text+ko) ====

@@ -172,6 +172,12 @@
 	struct file *fp;
 	int error;
 
+	/*
+	 * In theory, writes to vp->v_fifoinfo are serialized by the vnode
+	 * lock, so there can't be a race between multiple simultaneous opens
+	 * here.  We assert there hasn't been at the end of the allocation,
+	 * however, to be sure.
+	 */
 	if ((fip = vp->v_fifoinfo) == NULL) {
 		MALLOC(fip, struct fifoinfo *, sizeof(*fip), M_VNODE, M_WAITOK);
 		error = socreate(AF_LOCAL, &rso, SOCK_STREAM, 0, cred, td);
@@ -182,7 +188,7 @@
 		if (error)
 			goto fail2;
 		fip->fi_writesock = wso;
-		error = uipc_connect2(wso, rso);
+		error = soconnect2(wso, rso);
 		if (error) {
 			(void)soclose(wso);
 fail2:
@@ -196,6 +202,12 @@
 		SOCKBUF_LOCK(&rso->so_rcv);
 		rso->so_rcv.sb_state |= SBS_CANTRCVMORE;
 		SOCKBUF_UNLOCK(&rso->so_rcv);
+		/*
+		 * If the vnode lock is insufficiently serializing, we might
+		 * have to detect a race here and recover.
+		 */
+		KASSERT(vp->v_fifoinfo == NULL,
+		    ("fifo_open: v_fifoinfo race"));
 		vp->v_fifoinfo = fip;
 	}
 
@@ -234,12 +246,12 @@
 			SOCKBUF_UNLOCK(&fip->fi_writesock->so_rcv);
 			if (fip->fi_readers > 0) {
 				wakeup(&fip->fi_readers);
-				sorwakeup(fip->fi_writesock);
+				sorwakeup(fip->fi_readsock);
 			}
 		}
 	}
 	if ((ap->a_mode & O_NONBLOCK) == 0) {
-		if ((ap->a_mode & FREAD) && fip->fi_writers == 0) {
+		while ((ap->a_mode & FREAD) && fip->fi_writers == 0) {
 			VOP_UNLOCK(vp, 0, td);
 			error = msleep(&fip->fi_readers, &fifo_mtx,
 			    PDROP | PCATCH | PSOCK, "fifoor", 0);
@@ -253,13 +265,8 @@
 				return (error);
 			}
 			mtx_lock(&fifo_mtx);
-			/*
-			 * We must have got woken up because we had a writer.
-			 * That (and not still having one) is the condition
-			 * that we must wait for.
-			 */
 		}
-		if ((ap->a_mode & FWRITE) && fip->fi_readers == 0) {
+		while ((ap->a_mode & FWRITE) && fip->fi_readers == 0) {
 			VOP_UNLOCK(vp, 0, td);
 			error = msleep(&fip->fi_writers, &fifo_mtx,
 			    PDROP | PCATCH | PSOCK, "fifoow", 0);
@@ -272,11 +279,6 @@
 				}
 				return (error);
 			}
-			/*
-			 * We must have got woken up because we had
-			 * a reader.  That (and not still having one)
-			 * is the condition that we must wait for.
-			 */
 			mtx_lock(&fifo_mtx);
 		}
 	}
@@ -639,7 +641,7 @@
 		}
 	}
 	levents = events & (POLLOUT | POLLWRNORM | POLLWRBAND);
-	if (events) {
+	if (levents) {
 		filetmp.f_data = fip->fi_writesock;
 		filetmp.f_cred = cred;
 		if (filetmp.f_data) {