From owner-p4-projects@FreeBSD.ORG Sun Sep 11 20:57:59 2005 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 0418416A421; Sun, 11 Sep 2005 20:57:59 +0000 (GMT) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id A17F816A41F for ; Sun, 11 Sep 2005 20:57:58 +0000 (GMT) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from repoman.freebsd.org (repoman.freebsd.org [216.136.204.115]) by mx1.FreeBSD.org (Postfix) with ESMTP id 6435F43D46 for ; Sun, 11 Sep 2005 20:57:58 +0000 (GMT) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.13.1/8.13.1) with ESMTP id j8BKvwoH077796 for ; Sun, 11 Sep 2005 20:57:58 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.13.1/8.13.1/Submit) id j8BKvubT077793 for perforce@freebsd.org; Sun, 11 Sep 2005 20:57:56 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Date: Sun, 11 Sep 2005 20:57:56 GMT Message-Id: <200509112057.j8BKvubT077793@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to bb+lists.freebsd.perforce@cyrus.watson.org using -f From: Robert Watson To: Perforce Change Reviews Cc: Subject: PERFORCE change 83418 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 11 Sep 2005 20:57:59 -0000 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) {