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

[John Baldwin]

On Tuesday 21 October 2008 02:29:12 pm Kostik Belousov wrote:
> On Tue, Oct 21, 2008 at 01:41:40PM -0400, John Baldwin wrote:
> > 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.
> I guess this is a request for approval. It is approved then.
> 
> Note that in RELENG_6 destroy_dev() does not guarantee that all threads
> leaved cdevsw methods before device is destoyed.

Would you rather remove the ttyrel() from ttyfree() then and just leak the 
structures?

> > 
> > 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
> 



-- 
John Baldwin