Crash with FreeBSD 6.1 STABLE of today
Martin Blapp
mb at imp.ch
Mon Jun 26 11:06:26 UTC 2006
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);
More information about the freebsd-stable
mailing list