Date: Sun, 11 Sep 2005 20:57:56 GMT From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 83418 for review Message-ID: <200509112057.j8BKvubT077793@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=83418 Change 83418 by rwatson@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) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200509112057.j8BKvubT077793>