Date: Fri, 16 May 2003 05:43:19 -0700 (PDT) From: Don Lewis <truckman@FreeBSD.org> To: current@FreeBSD.org Subject: CFR: fifo_open()/fifo_close() patch Message-ID: <200305161243.h4GChJM7058171@gw.catspoiler.org>
next in thread | raw e-mail | index | archive | help
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
@@ -70,6 +70,7 @@
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 *);
@@ -97,6 +98,7 @@
{ &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 },
@@ -171,7 +173,7 @@
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);
@@ -205,6 +207,10 @@
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) {
@@ -227,12 +233,14 @@
}
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
@@ -243,17 +251,22 @@
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)
@@ -262,9 +275,8 @@
}
}
}
- 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);
}
@@ -525,10 +537,11 @@
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)
@@ -539,15 +552,33 @@
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);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200305161243.h4GChJM7058171>