Date: Tue, 21 Oct 2008 21:29:12 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: re@freebsd.org, freebsd-bugs@freebsd.org Subject: Re: kern/89538: [tty] [panic] triggered by "sysctl -a" Message-ID: <20081021182912.GS94031@deviant.kiev.zoral.com.ua> In-Reply-To: <200810211341.40856.jhb@freebsd.org> References: <20081021092132.GA1594@tmn.ru> <200810211341.40856.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--lCKkSrrNUaKEhaIQ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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 u=
ntil
> > there is not too late for 6.4!!!
> >=20
> > Sergey N. Voronkov,
>=20
> There are actually a few changes to merge. Looks like I didn't merge to =
6.x=20
> 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.
>=20
> jhb 2008-01-08 04:53:29 UTC
>=20
> FreeBSD src repository
>=20
> Modified files:
> sys/kern tty.c=20
> 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 valu=
e,
> 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 handl=
er.
> =20
> MFC after: 3 days
> Tested by: avatar
> =20
> Revision Changes Path
> 1.274 +20 -3 src/sys/kern/tty.c
>=20
> kib 2008-05-23 16:47:55 UTC
>=20
> FreeBSD src repository
>=20
> Modified files:
> sys/kern tty.c=20
> 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.
> =20
> 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.
> =20
> Reported, debugging help and tested by: pho
> DIscussed with and reviewed by: jhb
> MFC after: 1 week
> =20
> Revision Changes Path
> 1.280 +5 -2 src/sys/kern/tty.c
>=20
>=20
> Property changes on: .
> ___________________________________________________________________
> Modified: svn:mergeinfo
> Merged /head/sys:r175152,179251
>=20
> Index: kern/tty.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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.
> */
> -=20
> void
> ttyfree(struct tty *tp)
> {
> + struct cdev *dev;
> u_int unit;
> =20
> mtx_assert(&Giant, MA_OWNED);
> ttygone(tp);
> unit =3D tp->t_devunit;
> - destroy_dev(tp->t_mdev);
> + dev =3D tp->t_mdev;
> + dev->si_tty =3D NULL;
> + tp->t_dev =3D NULL;
> + destroy_dev(dev);
> + ttyrel(tp);
> free_unr(tty_unit, unit);
> }
> =20
> @@ -3049,8 +3053,9 @@
> tp =3D TAILQ_FIRST(&tty_list);
> if (tp !=3D NULL)
> ttyref(tp);
> - mtx_unlock(&tty_list_mutex);
> while (tp !=3D NULL) {
> + if (tp->t_state & TS_GONE)
> + goto nexttp;
> bzero(&xt, sizeof xt);
> xt.xt_size =3D sizeof xt;
> #define XT_COPY(field) xt.xt_##field =3D tp->t_##field
> @@ -3058,6 +3063,18 @@
> xt.xt_cancc =3D tp->t_canq.c_cc;
> xt.xt_outcc =3D 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 !=3D NULL)
> xt.xt_dev =3D 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 =3D SYSCTL_OUT(req, &xt, sizeof xt);
> if (error !=3D 0) {
> ttyrel(tp);
> return (error);
> }
> mtx_lock(&tty_list_mutex);
> - tp2 =3D TAILQ_NEXT(tp, t_list);
> +nexttp: tp2 =3D TAILQ_NEXT(tp, t_list);
> if (tp2 !=3D NULL)
> ttyref(tp2);
> mtx_unlock(&tty_list_mutex);
> ttyrel(tp);
> tp =3D tp2;
> + mtx_lock(&tty_list_mutex);
> }
> + mtx_unlock(&tty_list_mutex);
> return (0);
> }
> =20
>=20
> --=20
> John Baldwin
--lCKkSrrNUaKEhaIQ
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (FreeBSD)
iEYEARECAAYFAkj+H3gACgkQC3+MBN1Mb4hQcgCfUESgqvZdzuE23dALIUG+eFdv
owAAnjckjGgxhgTbtaIjGE81AdZ16rMJ
=zU62
-----END PGP SIGNATURE-----
--lCKkSrrNUaKEhaIQ--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081021182912.GS94031>