Fix a number of races in the controlling terminal open/close code.
authorMatthew Dillon <dillon@dragonflybsd.org>
Tue, 3 Jul 2007 17:22:14 +0000 (17:22 +0000)
committerMatthew Dillon <dillon@dragonflybsd.org>
Tue, 3 Jul 2007 17:22:14 +0000 (17:22 +0000)
This does not completely fix issue 715 but it should help reduce the
problem set.

* Integrate the revoke into ttyclosesession() so it can be issued as part
  of the same locking op.
* Check for races after acquiring the vnode lock and retry if necessary.
* Use vhold/vdrop to prevent destruction of the vnode during a possible
  race while we are attempting to obtain the lock.

Reported-by: Thomas Nikolajsen <thomas.nikolajsen@mail.dk> added the comment:
Dragonfly-bug: <http://bugs.dragonflybsd.org/issue715>

sys/kern/kern_exit.c
sys/kern/tty.c
sys/kern/tty_tty.c

index 8f1183c..647bf64 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.82 2007/07/01 01:11:35 dillon Exp $
+ * $DragonFly: src/sys/kern/kern_exit.c,v 1.83 2007/07/03 17:22:14 dillon Exp $
  */
 
 #include "opt_compat.h"
@@ -364,7 +364,6 @@ exit1(int rv)
 
        if (SESS_LEADER(p)) {
                struct session *sp = p->p_session;
-               struct vnode *vp;
 
                if (sp->s_ttyvp) {
                        /*
@@ -375,31 +374,22 @@ exit1(int rv)
                         *
                         * 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.
+                        * group will revoke the tty, so the ttyclosesession()
+                        * function will re-check sp->s_ttyvp.
                         */
                        if (sp->s_ttyp && (sp->s_ttyp->t_session == sp)) {
                                if (sp->s_ttyp->t_pgrp)
                                        pgsignal(sp->s_ttyp->t_pgrp, SIGHUP, 1);
-                               (void) ttywait(sp->s_ttyp);
-                               /*
-                                * The tty could have been revoked
-                                * if we blocked.
-                                */
-                               if ((vp = sp->s_ttyvp) != NULL) {
-                                       ttyclosesession(sp, 0);
-                                       vx_lock(vp);
-                                       VOP_REVOKE(vp, REVOKEALL);
-                                       vx_unlock(vp);
-                                       vrele(vp);      /* s_ttyvp ref */
-                               }
+                               ttywait(sp->s_ttyp);
+                               ttyclosesession(sp, 1); /* also revoke */
                        }
                        /*
                         * Release the tty.  If someone has it open via
                         * /dev/tty then close it (since they no longer can
                         * once we've NULL'd it out).
                         */
-                       if (sp->s_ttyvp)
-                               ttyclosesession(sp, 1);
+                       ttyclosesession(sp, 0);
+
                        /*
                         * s_ttyp is not zero'd; we use this to indicate
                         * that the session once had a controlling terminal.
index 864cc60..f300fbf 100644 (file)
@@ -37,7 +37,7 @@
  *
  *     @(#)tty.c       8.8 (Berkeley) 1/21/94
  * $FreeBSD: src/sys/kern/tty.c,v 1.129.2.5 2002/03/11 01:32:31 dd Exp $
- * $DragonFly: src/sys/kern/tty.c,v 1.41 2007/02/25 23:17:12 corecode Exp $
+ * $DragonFly: src/sys/kern/tty.c,v 1.42 2007/07/03 17:22:14 dillon Exp $
  */
 
 /*-
@@ -292,31 +292,64 @@ ttyclearsession(struct tty *tp)
 }
 
 /*
- * Terminate the tty vnode association for a session.  This is the 
+ * Release the tty vnode association for a session.  This is the 
  * 'other half' of the close.  Because multiple opens of /dev/tty
  * only generate a single open to the actual tty, the file modes
  * are locked to FREAD|FWRITE.
+ *
+ * If dorevoke is non-zero, the session is also revoked.  We have to
+ * close the vnode if VCTTYISOPEN is set.
  */
 void
-ttyclosesession(struct session *sp, int dorele)
+ttyclosesession(struct session *sp, int dorevoke)
 {
        struct vnode *vp;
 
+retry:
+       /*
+        * There may not be a controlling terminal or it may have been closed
+        * out from under us.
+        */
        if ((vp = sp->s_ttyvp) == NULL)
                return;
-       sp->s_ttyvp = NULL;
-       if (vp->v_flag & VCTTYISOPEN) {
-               if (vn_lock(vp, LK_EXCLUSIVE|LK_RETRY) == 0) {
+
+       /*
+        * We need a lock if we have to close or revoke.
+        */
+       if ((vp->v_flag & VCTTYISOPEN) || dorevoke) {
+               vhold(vp);
+               if (vn_lock(vp, LK_EXCLUSIVE|LK_RETRY)) {
+                       vdrop(vp);
+                       goto retry;
+               }
+
+               /*
+                * Retry if the vnode was ripped out from under us
+                */
+               if (vp != sp->s_ttyvp) {
+                       vn_unlock(vp);
+                       vdrop(vp);
+                       goto retry;
+               }
+
+               /*
+                * Close and revoke as needed
+                */
+               sp->s_ttyvp = NULL;
+               if (vp->v_flag & VCTTYISOPEN) {
                        vclrflags(vp, VCTTYISOPEN);
                        VOP_CLOSE(vp, FREAD|FWRITE);
-                       vn_unlock(vp);
                }
+               if (dorevoke)
+                       VOP_REVOKE(vp, REVOKEALL);
+               vn_unlock(vp);
+               vdrop(vp);
+       } else {
+               sp->s_ttyvp = NULL;
        }
-       if (dorele)
-               vrele(vp);
+       vrele(vp);
 }
 
-
 #define        FLUSHQ(q) {                                                     \
        if ((q)->c_cc)                                                  \
                ndflush(q, (q)->c_cc);                                  \
index 9f64ea5..ce27467 100644 (file)
@@ -32,7 +32,7 @@
  *
  *     @(#)tty_tty.c   8.2 (Berkeley) 9/23/93
  * $FreeBSD: src/sys/kern/tty_tty.c,v 1.30 1999/09/25 18:24:24 phk Exp $
- * $DragonFly: src/sys/kern/tty_tty.c,v 1.18 2006/09/10 01:26:39 dillon Exp $
+ * $DragonFly: src/sys/kern/tty_tty.c,v 1.19 2007/07/03 17:22:14 dillon Exp $
  */
 
 /*
@@ -84,22 +84,31 @@ cttyopen(struct dev_open_args *ap)
        int error;
 
        KKASSERT(p);
-       ttyvp = cttyvp(p);
-       if (ttyvp) {
-               if (ttyvp->v_flag & VCTTYISOPEN) {
-                       error = 0;
-               } else {
-                       vsetflags(ttyvp, VCTTYISOPEN);
-                       vn_lock(ttyvp, LK_EXCLUSIVE | LK_RETRY);
-                       error = VOP_OPEN(ttyvp, FREAD|FWRITE, ap->a_cred, NULL);
-                       if (error)
-                               vclrflags(ttyvp, VCTTYISOPEN);
-                       vn_unlock(ttyvp);
-               }
-       } else {
-               error = ENXIO;
+retry:
+       if ((ttyvp = cttyvp(p)) == NULL)
+               return (ENXIO);
+       if (ttyvp->v_flag & VCTTYISOPEN)
+               return (0);
+
+       /*
+        * Messy interlock, don't let the vnode go away while we try to
+        * lock it and check for race after we might have blocked.
+        */
+       vhold(ttyvp);
+       vn_lock(ttyvp, LK_EXCLUSIVE | LK_RETRY);
+       if (ttyvp != cttyvp(p) || (ttyvp->v_flag & VCTTYISOPEN)) {
+               kprintf("Warning: cttyopen: race avoided\n");
+               vn_unlock(ttyvp);
+               vdrop(ttyvp);
+               goto retry;
        }
-       return (error);
+       vsetflags(ttyvp, VCTTYISOPEN);
+       error = VOP_OPEN(ttyvp, FREAD|FWRITE, ap->a_cred, NULL);
+       if (error)
+               vclrflags(ttyvp, VCTTYISOPEN);
+       vn_unlock(ttyvp);
+       vdrop(ttyvp);
+       return(error);
 }
 
 /*
@@ -115,21 +124,33 @@ cttyclose(struct dev_close_args *ap)
        int error;
 
        KKASSERT(p);
-       ttyvp = cttyvp(p);
-       if (ttyvp == NULL) {
+retry:
+       /*
+        * The tty may have been TIOCNOTTY'd, don't return an
+        * error on close.  We just have nothing to do.
+        */
+       if ((ttyvp = cttyvp(p)) == NULL)
+               return(0);
+       if (ttyvp->v_flag & VCTTYISOPEN) {
                /*
-                * The tty may have been TIOCNOTTY'd, don't return an
-                * error on close.  We just have nothing to do.
+                * Avoid a nasty race if we block while getting the lock.
                 */
-               /* error = EIO; */
-               error = 0;
-       } else if (ttyvp->v_flag & VCTTYISOPEN) {
-               vclrflags(ttyvp, VCTTYISOPEN);
+               vhold(ttyvp);
                error = vn_lock(ttyvp, LK_EXCLUSIVE | LK_RETRY);
-               if (error == 0) {
-                       error = VOP_CLOSE(ttyvp, FREAD|FWRITE);
+               if (error) {
+                       vdrop(ttyvp);
+                       goto retry;
+               }
+               if (ttyvp != cttyvp(p) || (ttyvp->v_flag & VCTTYISOPEN) == 0) {
+                       kprintf("Warning: cttyclose: race avoided\n");
                        vn_unlock(ttyvp);
+                       vdrop(ttyvp);
+                       goto retry;
                }
+               vclrflags(ttyvp, VCTTYISOPEN);
+               error = VOP_CLOSE(ttyvp, FREAD|FWRITE);
+               vn_unlock(ttyvp);
+               vdrop(ttyvp);
        } else {
                error = 0;
        }