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

[Kostik Belousov]

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.
> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-bugs/attachments/20081021/f3ffa3e5/attachment.pgp