Fix a race with the clearing of p->p_session->s_ttyvp. NULL the pointer
authorMatthew Dillon <dillon@dragonflybsd.org>
Tue, 15 Jun 2004 00:30:55 +0000 (00:30 +0000)
committerMatthew Dillon <dillon@dragonflybsd.org>
Tue, 15 Jun 2004 00:30:55 +0000 (00:30 +0000)
out before calling vrele() rather then after.

Fix a bug with v_opencount accounting on revoke().  The underlying device
was being closed properly but v_opencount was being decremented which causes
it to go negative when close() is called on the descriptor later on.  To
fix the bug we zero out v_opencount() when the underlying vnode's device
is disassociated and spec_close() now only decrements it when the device is
associated.

Reported-by: GeekGod
Testing-by: GeekGod, Hiten, David Rhodus.
sys/kern/kern_exit.c
sys/kern/vfs_subr.c
sys/kern/vfs_vnops.c
sys/vfs/specfs/spec_vnops.c

index e27b067..c69eb80 100644 (file)
@@ -37,7 +37,7 @@
  *
  *     @(#)kern_exit.c 8.7 (Berkeley) 2/12/94
  * $FreeBSD: src/sys/kern/kern_exit.c,v 1.92.2.11 2003/01/13 22:51:16 dillon Exp $
- * $DragonFly: src/sys/kern/kern_exit.c,v 1.35 2004/06/04 20:35:36 dillon Exp $
+ * $DragonFly: src/sys/kern/kern_exit.c,v 1.36 2004/06/15 00:30:53 dillon Exp $
  */
 
 #include "opt_compat.h"
@@ -186,6 +186,7 @@ exit1(int rv)
         * This may block!
         */
        fdfree(p);
+       p->p_fd = NULL;
 
        if(p->p_leader->p_peers) {
                q = p->p_leader;
@@ -237,13 +238,18 @@ exit1(int rv)
 
        if (SESS_LEADER(p)) {
                struct session *sp = p->p_session;
+               struct vnode *vp;
 
                if (sp->s_ttyvp) {
                        /*
-                        * Controlling process.
-                        * Signal foreground pgrp,
-                        * drain controlling terminal
-                        * and revoke access to controlling terminal.
+                        * We are the controlling process.  Signal the 
+                        * foreground process group, drain the controlling
+                        * terminal, and revoke access to the controlling
+                        * terminal.
+                        *
+                        * NOTE: while waiting for the process group to exit
+                        * it is possible that one of the processes in the
+                        * group will revoke the tty, so we have to recheck.
                         */
                        if (sp->s_ttyp && (sp->s_ttyp->t_session == sp)) {
                                if (sp->s_ttyp->t_pgrp)
@@ -253,12 +259,16 @@ exit1(int rv)
                                 * The tty could have been revoked
                                 * if we blocked.
                                 */
-                               if (sp->s_ttyvp)
-                                       VOP_REVOKE(sp->s_ttyvp, REVOKEALL);
+                               if ((vp = sp->s_ttyvp) != NULL) {
+                                       sp->s_ttyvp = NULL;
+                                       VOP_REVOKE(vp, REVOKEALL);
+                                       vrele(vp);
+                               }
                        }
-                       if (sp->s_ttyvp)
+                       if ((vp = sp->s_ttyvp) != NULL) {
+                               sp->s_ttyvp = NULL;
                                vrele(sp->s_ttyvp);
-                       sp->s_ttyvp = NULL;
+                       }
                        /*
                         * s_ttyp is not zero'd; we use this to indicate
                         * that the session once had a controlling terminal.
index dfd5be2..bbaa844 100644 (file)
@@ -37,7 +37,7 @@
  *
  *     @(#)vfs_subr.c  8.31 (Berkeley) 5/26/95
  * $FreeBSD: src/sys/kern/vfs_subr.c,v 1.249.2.30 2003/04/04 20:35:57 tegge Exp $
- * $DragonFly: src/sys/kern/vfs_subr.c,v 1.32 2004/05/26 01:29:58 dillon Exp $
+ * $DragonFly: src/sys/kern/vfs_subr.c,v 1.33 2004/06/15 00:30:53 dillon Exp $
  */
 
 /*
@@ -1556,9 +1556,13 @@ v_release_rdev(struct vnode *vp)
        if ((dev = vp->v_rdev) != NULL) {
                lwkt_gettoken(&ilock, &spechash_token);
                SLIST_REMOVE(&dev->si_hlist, vp, vnode, v_specnext);
-               if (dev_ref_debug)
-                       printf("Y2");
+               if (dev_ref_debug && vp->v_opencount != 0) {
+                       printf("releasing rdev with non-0 "
+                               "v_opencount(%d) (revoked?)\n",
+                               vp->v_opencount);
+               }
                vp->v_rdev = NULL;
+               vp->v_opencount = 0;
                release_dev(dev);
                lwkt_reltoken(&ilock);
        }
@@ -2215,7 +2219,9 @@ vgonel(struct vnode *vp, lwkt_tokref_t vlock, struct thread *td)
 
        /*
         * If special device, remove it from special device alias list
-        * if it is on one.
+        * if it is on one.  This should normally only occur if a vnode is
+        * being revoked as the device should otherwise have been released
+        * naturally.
         */
        if ((vp->v_type == VBLK || vp->v_type == VCHR) && vp->v_rdev != NULL) {
                v_release_rdev(vp);
index 1eef42f..f18d035 100644 (file)
@@ -37,7 +37,7 @@
  *
  *     @(#)vfs_vnops.c 8.2 (Berkeley) 1/21/94
  * $FreeBSD: src/sys/kern/vfs_vnops.c,v 1.87.2.13 2002/12/29 18:19:53 dillon Exp $
- * $DragonFly: src/sys/kern/vfs_vnops.c,v 1.21 2004/05/21 15:41:23 drhodus Exp $
+ * $DragonFly: src/sys/kern/vfs_vnops.c,v 1.22 2004/06/15 00:30:53 dillon Exp $
  */
 
 #include <sys/param.h>
@@ -581,6 +581,7 @@ static int
 vn_ioctl(struct file *fp, u_long com, caddr_t data, struct thread *td)
 {
        struct vnode *vp = ((struct vnode *)fp->f_data);
+       struct vnode *ovp;
        struct ucred *ucred;
        struct vattr vattr;
        int error;
@@ -623,11 +624,11 @@ vn_ioctl(struct file *fp, u_long com, caddr_t data, struct thread *td)
                                return (0);
 
                        /* Get rid of reference to old control tty */
-                       if (sess->s_ttyvp)
-                               vrele(sess->s_ttyvp);
-
-                       sess->s_ttyvp = vp;
+                       ovp = sess->s_ttyvp;
                        vref(vp);
+                       sess->s_ttyvp = vp;
+                       if (ovp)
+                               vrele(ovp);
                }
                return (error);
        }
index 2cb17a2..c8267fc 100644 (file)
@@ -32,7 +32,7 @@
  *
  *     @(#)spec_vnops.c        8.14 (Berkeley) 5/21/95
  * $FreeBSD: src/sys/miscfs/specfs/spec_vnops.c,v 1.131.2.4 2001/02/26 04:23:20 jlemon Exp $
- * $DragonFly: src/sys/vfs/specfs/spec_vnops.c,v 1.17 2004/06/06 18:47:51 dillon Exp $
+ * $DragonFly: src/sys/vfs/specfs/spec_vnops.c,v 1.18 2004/06/15 00:30:55 dillon Exp $
  */
 
 #include <sys/param.h>
@@ -151,21 +151,26 @@ spec_open(struct vop_open_args *ap)
        /*
         * Resolve the device.  If the vnode is already open v_rdev may
         * already be resolved.  However, if the device changes out from
-        * under us we report it (and, for now, we allow it).
+        * under us we report it (and, for now, we allow it).  Since
+        * v_release_rdev() zero's v_opencount, we have to save and restore
+        * it when replacing the rdev reference.
         */
        if (vp->v_rdev != NULL) {
                dev = udev2dev(vp->v_udev, isblk);
                if (dev != vp->v_rdev) {
+                       int oc = vp->v_opencount;
                        printf(
                            "Warning: spec_open: dev %s was lost",
                            vp->v_rdev->si_name);
                        v_release_rdev(vp);
                        error = v_associate_rdev(vp, 
                                        udev2dev(vp->v_udev, isblk));
-                       if (error)
+                       if (error) {
                                printf(", reacquisition failed\n");
-                       else
+                       } else {
+                               vp->v_opencount = oc;
                                printf(", reacquisition successful\n");
+                       }
                } else {
                        error = 0;
                }
@@ -590,13 +595,16 @@ spec_close(struct vop_close_args *ap)
         * process' controlling terminal.  In that case,
         * if the reference count is 2 (this last descriptor
         * plus the session), release the reference from the session.
+        *
+        * It is possible for v_opencount to be 0 or 1 in this case, 0
+        * because the tty might have been revoked.
         */
        if (dev)
                reference_dev(dev);
-       if (vcount(vp) == 2 && vp->v_opencount == 1 && 
+       if (vcount(vp) == 2 && vp->v_opencount <= 1 && 
            p && (vp->v_flag & VXLOCK) == 0 && vp == p->p_session->s_ttyvp) {
-               vrele(vp);
                p->p_session->s_ttyvp = NULL;
+               vrele(vp);
        }
 
        /*
@@ -618,22 +626,20 @@ spec_close(struct vop_close_args *ap)
 
        /*
         * Track the actual opens and closes on the vnode.  The last close
-        * disassociates the rdev.
+        * disassociates the rdev.  If the rdev is already disassociated 
+        * the vnode might have been revoked and no further opencount
+        * tracking occurs.
         */
-       KKASSERT(vp->v_opencount > 0);
-       if (dev_ref_debug) {
-               if (dev) {
+       if (dev) {
+               KKASSERT(vp->v_opencount > 0);
+               if (dev_ref_debug) {
                        printf("spec_close: %s %d\n",
                                dev->si_name, vp->v_opencount - 1);
-               } else {
-                       printf("spec_close: closed revoked device %08x\n",
-                               vp->v_udev);
                }
-       }
-       if (--vp->v_opencount == 0)
-               v_release_rdev(vp);
-       if (dev)
+               if (--vp->v_opencount == 0)
+                       v_release_rdev(vp);
                release_dev(dev);
+       }
        return(error);
 }