Date: Mon, 26 Jun 2006 13:06:17 +0200 (CEST) From: Martin Blapp <mb@imp.ch> To: Max Laier <max@love2party.net> Cc: freebsd-stable@freebsd.org, "Wojciech A. Koszek" <wkoszek@freebsd.org>, csjp@freebsd.org, Robert Watson <rwatson@freebsd.org>, Patrick Guelat <patg@imp.ch> Subject: Re: Crash with FreeBSD 6.1 STABLE of today Message-ID: <20060626115110.O14714@godot.imp.ch> In-Reply-To: <200606231928.58063.max@love2party.net> References: <20060621202508.S17514@godot.imp.ch> <20060623133915.S14714@godot.imp.ch> <1151078632.62769.30.camel@buffy.york.ac.uk> <200606231928.58063.max@love2party.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Max, Robert and Gavin, > Note, I am not sure if the patch still applies or is correct at all, but from > looking at it (and the name of the file) I seem to remember that there was a > problem with t_pgrp and t_session being accessed unlocked in some places. > Maybe it helps, maybe it doesn't. > > [1] http://people.freebsd.org/~mlaier/tty.t_pgrp.diff Some things are now different. The part at 2551,2567 has now additional PGRP_LOCKs. I've also added the proctree locking at line 1654 as I did in the first patch. This part is unlocked too ... , don't you think ? That's the place there panic appeared. @@ -1654,8 +1668,8 @@ !ISSET(tp->t_cflag, CLOCAL)) { SET(tp->t_state, TS_ZOMBIE); CLR(tp->t_state, TS_CONNECTED); + sx_slock(&proctree_lock); if (tp->t_session) { - sx_slock(&proctree_lock); if (tp->t_session->s_leader) { struct proc *p; @@ -1664,8 +1678,8 @@ psignal(p, SIGHUP); PROC_UNLOCK(p); } - sx_sunlock(&proctree_lock); } + sx_sunlock(&proctree_lock); ttyflush(tp, FREAD | FWRITE); return (0); } Also this place was unlocked: @@ -942,8 +952,11 @@ * Policy -- Don't allow FIOSETOWN on someone else's * controlling tty */ - if (tp->t_session != NULL && !isctty(p, tp)) + sx_snlock(&proctree_lock); + if (tp->t_session != NULL && !isctty(p, tp)) { + sx_sunlock(&proctree_lock); return (ENOTTY); + } error = fsetown(*(int *)data, &tp->t_sigio); if (error) Here is the complete patch. What do you thing about it ? It's a FreeBSD 6 STABLE patch. I'll test this patch now and tell you any problems with it. Please do the same if you have time ... Also available at: http://mx.imp.ch/patch-tty.t_pgrp.diff Martin --- sys/kern/tty.c.orig Thu Dec 15 18:13:40 2005 +++ sys/kern/tty.c Mon Jun 26 12:53:33 2006 @@ -329,8 +329,10 @@ tp->t_gen++; tp->t_line = TTYDISC; tp->t_hotchar = 0; + sx_xlock(&proctree_lock); tp->t_pgrp = NULL; tp->t_session = NULL; + sx_xunlock(&proctree_lock); tp->t_state = 0; knlist_clear(&tp->t_rsel.si_note, 0); knlist_clear(&tp->t_wsel.si_note, 0); @@ -401,11 +403,13 @@ return (0); if (ISSET(iflag, BRKINT)) { ttyflush(tp, FREAD | FWRITE); + sx_slock(&proctree_lock); if (tp->t_pgrp != NULL) { PGRP_LOCK(tp->t_pgrp); pgsignal(tp->t_pgrp, SIGINT, 1); PGRP_UNLOCK(tp->t_pgrp); } + sx_sunlock(&proctree_lock); goto endcase; } if (ISSET(iflag, PARMRK)) @@ -483,23 +487,27 @@ if (!ISSET(lflag, NOFLSH)) ttyflush(tp, FREAD | FWRITE); ttyecho(c, tp); + sx_slock(&proctree_lock); if (tp->t_pgrp != NULL) { PGRP_LOCK(tp->t_pgrp); pgsignal(tp->t_pgrp, CCEQ(cc[VINTR], c) ? SIGINT : SIGQUIT, 1); PGRP_UNLOCK(tp->t_pgrp); } + sx_sunlock(&proctree_lock); goto endcase; } if (CCEQ(cc[VSUSP], c)) { if (!ISSET(lflag, NOFLSH)) ttyflush(tp, FREAD); ttyecho(c, tp); + sx_slock(&proctree_lock); if (tp->t_pgrp != NULL) { PGRP_LOCK(tp->t_pgrp); pgsignal(tp->t_pgrp, SIGTSTP, 1); PGRP_UNLOCK(tp->t_pgrp); } + sx_sunlock(&proctree_lock); goto endcase; } } @@ -617,11 +625,13 @@ * ^T - kernel info and generate SIGINFO */ if (CCEQ(cc[VSTATUS], c) && ISSET(lflag, IEXTEN)) { + sx_slock(&proctree_lock); if (ISSET(lflag, ISIG) && tp->t_pgrp != NULL) { PGRP_LOCK(tp->t_pgrp); pgsignal(tp->t_pgrp, SIGINFO, 1); PGRP_UNLOCK(tp->t_pgrp); } + sx_sunlock(&proctree_lock); if (!ISSET(lflag, NOKERNINFO)) ttyinfo(tp); goto endcase; @@ -942,8 +952,11 @@ * Policy -- Don't allow FIOSETOWN on someone else's * controlling tty */ - if (tp->t_session != NULL && !isctty(p, tp)) + sx_snlock(&proctree_lock); + if (tp->t_session != NULL && !isctty(p, tp)) { + sx_sunlock(&proctree_lock); return (ENOTTY); + } error = fsetown(*(int *)data, &tp->t_sigio); if (error) @@ -1013,7 +1026,9 @@ case TIOCGPGRP: /* get pgrp of tty */ if (!isctty(p, tp)) return (ENOTTY); + sx_slock(&proctree_lock); *(int *)data = tp->t_pgrp ? tp->t_pgrp->pg_id : NO_PID; + sx_sunlock(&proctree_lock); break; #ifdef TIOCHPCL case TIOCHPCL: /* hang up on last close */ @@ -1193,11 +1208,11 @@ break; case TIOCSCTTY: /* become controlling tty */ /* Session ctty vnode pointer set in vnode layer. */ - sx_slock(&proctree_lock); + sx_xlock(&proctree_lock); /* XXX: protect t_pgrp */ if (!SESS_LEADER(p) || ((p->p_session->s_ttyvp || tp->t_session) && (tp->t_session != p->p_session))) { - sx_sunlock(&proctree_lock); + sx_xunlock(&proctree_lock); return (EPERM); } tp->t_session = p->p_session; @@ -1209,28 +1224,28 @@ PROC_LOCK(p); p->p_flag |= P_CONTROLT; PROC_UNLOCK(p); - sx_sunlock(&proctree_lock); + sx_xunlock(&proctree_lock); break; case TIOCSPGRP: { /* set pgrp of tty */ - sx_slock(&proctree_lock); + sx_xlock(&proctree_lock); /* XXX: protect t_pgrp */ pgrp = pgfind(*(int *)data); if (!isctty(p, tp)) { if (pgrp != NULL) PGRP_UNLOCK(pgrp); - sx_sunlock(&proctree_lock); + sx_xunlock(&proctree_lock); return (ENOTTY); } if (pgrp == NULL) { - sx_sunlock(&proctree_lock); + sx_xunlock(&proctree_lock); return (EPERM); } PGRP_UNLOCK(pgrp); if (pgrp->pg_session != p->p_session) { - sx_sunlock(&proctree_lock); + sx_xunlock(&proctree_lock); return (EPERM); } - sx_sunlock(&proctree_lock); tp->t_pgrp = pgrp; + sx_xunlock(&proctree_lock); break; } case TIOCSTAT: /* simulate control-T */ @@ -1242,11 +1257,13 @@ if (bcmp((caddr_t)&tp->t_winsize, data, sizeof (struct winsize))) { tp->t_winsize = *(struct winsize *)data; + sx_slock(&proctree_lock); if (tp->t_pgrp != NULL) { PGRP_LOCK(tp->t_pgrp); pgsignal(tp->t_pgrp, SIGWINCH, 1); PGRP_UNLOCK(tp->t_pgrp); } + sx_sunlock(&proctree_lock); } break; case TIOCSDRAINWAIT: @@ -1654,8 +1671,8 @@ !ISSET(tp->t_cflag, CLOCAL)) { SET(tp->t_state, TS_ZOMBIE); CLR(tp->t_state, TS_CONNECTED); + sx_slock(&proctree_lock); if (tp->t_session) { - sx_slock(&proctree_lock); if (tp->t_session->s_leader) { struct proc *p; @@ -1664,8 +1681,8 @@ psignal(p, SIGHUP); PROC_UNLOCK(p); } - sx_sunlock(&proctree_lock); } + sx_sunlock(&proctree_lock); ttyflush(tp, FREAD | FWRITE); return (0); } @@ -1937,11 +1954,13 @@ */ if (CCEQ(cc[VDSUSP], c) && ISSET(lflag, IEXTEN | ISIG) == (IEXTEN | ISIG)) { + sx_slock(&proctree_lock); if (tp->t_pgrp != NULL) { PGRP_LOCK(tp->t_pgrp); pgsignal(tp->t_pgrp, SIGTSTP, 1); PGRP_UNLOCK(tp->t_pgrp); } + sx_sunlock(&proctree_lock); if (first) { error = ttysleep(tp, &lbolt, TTIPRI | PCATCH, "ttybg3", 0); @@ -2553,12 +2572,15 @@ * On return following a ttyprintf(), we set tp->t_rocount to 0 so * that pending input will be retyped on BS. */ + sx_slock(&proctree_lock); if (tp->t_session == NULL) { + sx_sunlock(&proctree_lock); ttyprintf(tp, "not a controlling terminal\n"); tp->t_rocount = 0; return; } if (tp->t_pgrp == NULL) { + sx_sunlock(&proctree_lock); ttyprintf(tp, "no foreground process group\n"); tp->t_rocount = 0; return; @@ -2566,10 +2588,12 @@ PGRP_LOCK(tp->t_pgrp); if (LIST_EMPTY(&tp->t_pgrp->pg_members)) { PGRP_UNLOCK(tp->t_pgrp); + sx_sunlock(&proctree_lock); ttyprintf(tp, "empty foreground process group\n"); tp->t_rocount = 0; return; } + sx_sunlock(&proctree_lock); /* * Pick the most interesting process and copy some of its @@ -3046,10 +3070,12 @@ XT_COPY(state); XT_COPY(flags); XT_COPY(timeout); + sx_slock(&proctree_lock); if (tp->t_pgrp != NULL) xt.xt_pgid = tp->t_pgrp->pg_id; if (tp->t_session != NULL) xt.xt_sid = tp->t_session->s_sid; + sx_sunlock(&proctree_lock); XT_COPY(termios); XT_COPY(winsize); XT_COPY(column);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060626115110.O14714>