From f1aeb0c09b74900e9e96f24334b27c83cc383f9b Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Tue, 3 Jul 2007 17:22:14 +0000 Subject: [PATCH] Fix a number of races in the controlling terminal open/close code. 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 added the comment: Dragonfly-bug: --- sys/kern/kern_exit.c | 24 +++++---------- sys/kern/tty.c | 53 ++++++++++++++++++++++++++------ sys/kern/tty_tty.c | 73 ++++++++++++++++++++++++++++---------------- 3 files changed, 97 insertions(+), 53 deletions(-) diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c index 8f1183cc91..647bf64773 100644 --- a/sys/kern/kern_exit.c +++ b/sys/kern/kern_exit.c @@ -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. diff --git a/sys/kern/tty.c b/sys/kern/tty.c index 864cc60feb..f300fbfd36 100644 --- a/sys/kern/tty.c +++ b/sys/kern/tty.c @@ -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); \ diff --git a/sys/kern/tty_tty.c b/sys/kern/tty_tty.c index 9f64ea57f1..ce27467538 100644 --- a/sys/kern/tty_tty.c +++ b/sys/kern/tty_tty.c @@ -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; } -- 2.41.0