From owner-freebsd-current@FreeBSD.ORG Fri May 16 05:43:26 2003 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id B10C137B401 for ; Fri, 16 May 2003 05:43:26 -0700 (PDT) Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id EFDD343F75 for ; Fri, 16 May 2003 05:43:25 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.12.9/8.12.9) with ESMTP id h4GChJM7058171 for ; Fri, 16 May 2003 05:43:23 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <200305161243.h4GChJM7058171@gw.catspoiler.org> Date: Fri, 16 May 2003 05:43:19 -0700 (PDT) From: Don Lewis To: current@FreeBSD.org MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii Subject: CFR: fifo_open()/fifo_close() patch X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 May 2003 12:43:27 -0000 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); }