kern/89538: [tty] [panic] triggered by "sysctl -a"

[John Baldwin]

On Tuesday 21 October 2008 05:21:32 am Sergey N. Voronkov wrote:
> Please, reopen this PR. Fix need to be MFC'd to RELENG_6 & RELENG_6_4 until
> there is not too late for 6.4!!!
> 
> Sergey N. Voronkov,

There are actually a few changes to merge.  Looks like I didn't merge to 6.x 
because of the 6.3 freeze and forgot to do it after the freeze was lifted.

jhb         2008-01-08 04:53:29 UTC

  FreeBSD src repository

  Modified files:
    sys/kern             tty.c 
  Log:
  Close a race in the kern.ttys sysctl handler that resulted in panics in
  dev2udev() when a tty was being detached concurrently with the sysctl
  handler:
  - Hold the 'tty_list_mutex' lock while we read all the fields out of the
    struct tty for copying out later.  Previously the pty(4) and pts(4)
    destroy routines could set t_dev to NULL, drop their reference on the
    tty and destroy the cdev while the sysctl handler was attempting to
    invoke dev2udev() on the cdev being destroyed.  This happened when the
    sysctl handler read the value of t_dev prior to it being set to NULL
    either due to it being stale or due to timing races.  By holding the
    list lock we guarantee that the destroy routines will block in ttyrel()
    in that case and not destroy the cdev until after we've copied all of our
    data.  We may see a NULL cdev pointer or we may see the previous value,
    but the previous value will no longer point to a destroyed cdev if we
    see it.
  - Fix the ttyfree() routine used by tty device drivers in their detach
    methods to use ttyrel() on the tty so we don't leak them.  Also, fix it
    to use the same order of operations as pty/pts destruction (set t_dev
    NULL, ttyrel(), destroy_dev()) so it cooperates with the sysctl handler.
  
  MFC after:      3 days
  Tested by:      avatar
  
  Revision  Changes    Path
  1.274     +20 -3     src/sys/kern/tty.c

kib         2008-05-23 16:47:55 UTC

  FreeBSD src repository

  Modified files:
    sys/kern             tty.c 
  Log:
  Rev. 1.274 put the ttyrel() call before the destroy_dev() in the
  ttyfree(), freeing the tty. Since destroy_dev() may call d_purge()
  cdevsw method, that is the ttypurge() for the tty, the code ends up
  accessing freed tty structure.
  
  Put the ttyrel() after destroy_dev() in the ttyfree. To prevent the
  panic the rev. 1.274 provided fix for, check the TS_GONE in sysctl
  handler and refuse to provide information on such tty.
  
  Reported, debugging help and tested by: pho
  DIscussed with and reviewed by: jhb
  MFC after:      1 week
  
  Revision  Changes    Path
  1.280     +5 -2      src/sys/kern/tty.c


Property changes on: .
___________________________________________________________________
Modified: svn:mergeinfo
   Merged /head/sys:r175152,179251

Index: kern/tty.c
===================================================================
--- kern/tty.c	(revision 184128)
+++ kern/tty.c	(working copy)
@@ -3024,16 +3024,20 @@
  *
  * XXX: This shall sleep until all threads have left the driver.
  */
- 
 void
 ttyfree(struct tty *tp)
 {
+	struct cdev *dev;
 	u_int unit;
  
 	mtx_assert(&Giant, MA_OWNED);
 	ttygone(tp);
 	unit = tp->t_devunit;
-	destroy_dev(tp->t_mdev);
+	dev = tp->t_mdev;
+	dev->si_tty = NULL;
+	tp->t_dev = NULL;
+	destroy_dev(dev);
+	ttyrel(tp);
 	free_unr(tty_unit, unit);
 }
 
@@ -3049,8 +3053,9 @@
 	tp = TAILQ_FIRST(&tty_list);
 	if (tp != NULL)
 		ttyref(tp);
-	mtx_unlock(&tty_list_mutex);
 	while (tp != NULL) {
+		if (tp->t_state & TS_GONE)
+			goto nexttp;
 		bzero(&xt, sizeof xt);
 		xt.xt_size = sizeof xt;
 #define XT_COPY(field) xt.xt_##field = tp->t_##field
@@ -3058,6 +3063,18 @@
 		xt.xt_cancc = tp->t_canq.c_cc;
 		xt.xt_outcc = tp->t_outq.c_cc;
 		XT_COPY(line);
+
+		/*
+		 * XXX: We hold the tty list lock while doing this to
+		 * work around a race with pty/pts tty destruction.
+		 * They set t_dev to NULL and then call ttyrel() to
+		 * free the structure which will block on the list
+		 * lock before they call destroy_dev() on the cdev
+		 * backing t_dev.
+		 *
+		 * XXX: ttyfree() now does the same since it has been
+		 * fixed to not leak ttys.
+		 */
 		if (tp->t_dev != NULL)
 			xt.xt_dev = dev2udev(tp->t_dev);
 		XT_COPY(state);
@@ -3080,19 +3097,22 @@
 		XT_COPY(olowat);
 		XT_COPY(ospeedwat);
 #undef XT_COPY
+		mtx_unlock(&tty_list_mutex);
 		error = SYSCTL_OUT(req, &xt, sizeof xt);
 		if (error != 0) {
 			ttyrel(tp);
 			return (error);
 		}
 		mtx_lock(&tty_list_mutex);
-		tp2 = TAILQ_NEXT(tp, t_list);
+nexttp:		tp2 = TAILQ_NEXT(tp, t_list);
 		if (tp2 != NULL)
 			ttyref(tp2);
 		mtx_unlock(&tty_list_mutex);
 		ttyrel(tp);
 		tp = tp2;
+		mtx_lock(&tty_list_mutex);
 	}
+	mtx_unlock(&tty_list_mutex);
 	return (0);
 }
 

-- 
John Baldwin