Skip site navigation (1)Skip section navigation (2)
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>