CFR: fifo_open()/fifo_close() patch

There are a few problems in the fifo_open() and fifo_close()
implementations.

	fifo_open() calls VOP_CLOSE() with the vnode locked, whereas
        VOP_CLOSE() should be called with the vnode unlocked.

	There is a race condition that may cause wakeups to be missed.

	fifo_close() manipulates fip->fi_{readers,writers} without
        using any time of lock to serial access.

	The use of vrefcnt() in fifo_close is bogus, see the comment
	in the vrefcnt() implementation for the reason.

I'm planning to ask for RE approval to include this patch in 5.1 after
it has gotten some review.  After 5.1 release, I plan to restructure the
code in fifo_open(), but not make further functional changes.

This patch makes the following changes:

	Create fifo_inactive() and free the fifo data structures there
	instead of in fifo_close() to eliminate the need for fifo_open()
	call fifo_close() in some of the failure cases.  This also
	eliminates the need for the vrefcnt() call in fifo_close().

	Protect fip->fi_{readers,writers} with the vnode interlock in both
	fifo_open() and fifo_close().

	Convert from tsleep() to msleep() using the vnode interlock to
	eliminate the race condition.


Index: sys/fs/fifofs/fifo_vnops.c
===================================================================
RCS file: /home/ncvs/src/sys/fs/fifofs/fifo_vnops.c,v
retrieving revision 1.85
diff -u -r1.85 fifo_vnops.c
--- sys/fs/fifofs/fifo_vnops.c	24 Mar 2003 11:03:42 -0000	1.85
+++ sys/fs/fifofs/fifo_vnops.c	15 May 2003 20:23:34 -0000
_at__at_ -70,6 +70,7 _at__at_
 static int	fifo_lookup(struct vop_lookup_args *);
 static int	fifo_open(struct vop_open_args *);
 static int	fifo_close(struct vop_close_args *);
+static int	fifo_inactive(struct vop_inactive_args *);
 static int	fifo_read(struct vop_read_args *);
 static int	fifo_write(struct vop_write_args *);
 static int	fifo_ioctl(struct vop_ioctl_args *);
_at__at_ -97,6 +98,7 _at__at_
 	{ &vop_create_desc,		(vop_t *) vop_panic },
 	{ &vop_getattr_desc,		(vop_t *) vop_ebadf },
 	{ &vop_getwritemount_desc, 	(vop_t *) vop_stdgetwritemount },
+	{ &vop_inactive_desc,		(vop_t *) fifo_inactive },
 	{ &vop_ioctl_desc,		(vop_t *) fifo_ioctl },
 	{ &vop_kqfilter_desc,		(vop_t *) fifo_kqfilter },
 	{ &vop_lease_desc,		(vop_t *) vop_null },
_at__at_ -171,7 +173,7 _at__at_
 	struct fifoinfo *fip;
 	struct thread *td = ap->a_td;
 	struct socket *rso, *wso;
-	int error;
+	int error = 0;
 
 	if ((fip = vp->v_fifoinfo) == NULL) {
 		MALLOC(fip, struct fifoinfo *, sizeof(*fip), M_VNODE, M_WAITOK);
_at__at_ -205,6 +207,10 _at__at_
 		wso->so_snd.sb_lowat = PIPE_BUF;
 		rso->so_state |= SS_CANTRCVMORE;
 	}
+
+	/* Protect fi_readers and fi_writers with vnode interlock. */
+	VOP_UNLOCK(vp, 0, td);
+	VI_LOCK(vp);
 	if (ap->a_mode & FREAD) {
 		fip->fi_readers++;
 		if (fip->fi_readers == 1) {
_at__at_ -227,12 +233,14 _at__at_
 	}
 	if ((ap->a_mode & FREAD) && (ap->a_mode & O_NONBLOCK) == 0) {
 		if (fip->fi_writers == 0) {
-			VOP_UNLOCK(vp, 0, td);
-			error = tsleep(&fip->fi_readers,
+			error = msleep(&fip->fi_readers, VI_MTX(vp),
 			    PCATCH | PSOCK, "fifoor", 0);
-			vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td);
-			if (error)
+			if (error) {
+				fip->fi_readers--;
+				if (fip->fi_readers == 0)
+					socantsendmore(fip->fi_writesock);
 				goto bad;
+			}
 			/*
 			 * We must have got woken up because we had a writer.
 			 * That (and not still having one) is the condition
_at__at_ -243,17 +251,22 _at__at_
 	if (ap->a_mode & FWRITE) {
 		if (ap->a_mode & O_NONBLOCK) {
 			if (fip->fi_readers == 0) {
+				fip->fi_writers--;
+				if (fip->fi_writers == 0)
+					socantrcvmore(fip->fi_readsock);
 				error = ENXIO;
 				goto bad;
 			}
 		} else {
 			if (fip->fi_readers == 0) {
-				VOP_UNLOCK(vp, 0, td);
-				error = tsleep(&fip->fi_writers,
+				error = msleep(&fip->fi_writers, VI_MTX(vp),
 				    PCATCH | PSOCK, "fifoow", 0);
-				vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td);
-				if (error)
+				if (error) {
+					fip->fi_writers--;
+					if (fip->fi_writers == 0)
+						socantrcvmore(fip->fi_readsock);
 					goto bad;
+				}
 				/*
 				 * We must have got woken up because we had
 				 * a reader.  That (and not still having one)
_at__at_ -262,9 +275,8 _at__at_
 			}
 		}
 	}
-	return (0);
 bad:
-	VOP_CLOSE(vp, ap->a_mode, ap->a_cred, td);
+	vn_lock(vp, LK_EXCLUSIVE | LK_RETRY | LK_INTERLOCK, td);
 	return (error);
 }
 
_at__at_ -525,10 +537,11 _at__at_
 		struct thread *a_td;
 	} */ *ap;
 {
-	register struct vnode *vp = ap->a_vp;
-	register struct fifoinfo *fip = vp->v_fifoinfo;
-	int error1, error2;
+	struct vnode *vp = ap->a_vp;
+	struct fifoinfo *fip = vp->v_fifoinfo;
 
+	/* Protect fi_readers and fi_writers with vnode interlock. */
+	VI_LOCK(vp);
 	if (ap->a_fflag & FREAD) {
 		fip->fi_readers--;
 		if (fip->fi_readers == 0)
_at__at_ -539,15 +552,33 _at__at_
 		if (fip->fi_writers == 0)
 			socantrcvmore(fip->fi_readsock);
 	}
-	if (vrefcnt(vp) > 1)
-		return (0);
-	error1 = soclose(fip->fi_readsock);
-	error2 = soclose(fip->fi_writesock);
-	FREE(fip, M_VNODE);
-	vp->v_fifoinfo = NULL;
-	if (error1)
-		return (error1);
-	return (error2);
+	VI_UNLOCK(vp);
+	return (0);
+}
+
+/*
+ * Device inactive routine
+ */
+/* ARGSUSED */
+static int
+fifo_inactive(ap)
+	struct vop_inactive_args /* {
+		struct vnode *a_vp;
+		struct thread *a_td;
+	} */ *ap;
+{
+	struct vnode *vp = ap->a_vp;
+	struct fifoinfo *fip = vp->v_fifoinfo;
+
+	VI_LOCK(vp);
+	if (fip != NULL && vp->v_usecount == 0) {
+		vp->v_fifoinfo = NULL;
+		(void) soclose(fip->fi_readsock);
+		(void) soclose(fip->fi_writesock);
+		FREE(fip, M_VNODE);
+	}
+	VOP_UNLOCK(vp, LK_INTERLOCK, ap->a_td);
+	return (0);
 }