Date: Tue, 21 Oct 2008 21:46:32 +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: <20081021184632.GU94031@deviant.kiev.zoral.com.ua> In-Reply-To: <200810211437.18048.jhb@freebsd.org> References: <20081021092132.GA1594@tmn.ru> <200810211341.40856.jhb@freebsd.org> <20081021182912.GS94031@deviant.kiev.zoral.com.ua> <200810211437.18048.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--NQqDiQVflaif2hna
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Tue, Oct 21, 2008 at 02:37:17PM -0400, John Baldwin wrote:
> 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=20
> until
> > > > 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=20
> 6.x=20
> > > because of the 6.3 freeze and forgot to do it after the freeze was li=
fted.
> > I guess this is a request for approval. It is approved then.
> >=20
> > Note that in RELENG_6 destroy_dev() does not guarantee that all threads
> > leaved cdevsw methods before device is destoyed.
>=20
> Would you rather remove the ttyrel() from ttyfree() then and just leak th=
e=20
> structures?
I do not know what is worse. Leak looks more tolerable.
>=20
> > >=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 panic=
s in
> > > dev2udev() when a tty was being detached concurrently with the sysc=
tl
> > > handler:
> > > - Hold the 'tty_list_mutex' lock while we read all the fields out o=
f 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 whe=
n the
> > > sysctl handler read the value of t_dev prior to it being set to N=
ULL
> > > 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=20
> ttyrel()
> > > in that case and not destroy the cdev until after we've copied al=
l of=20
> our
> > > data. We may see a NULL cdev pointer or we may see the previous=
=20
> value,
> > > but the previous value will no longer point to a destroyed cdev i=
f we
> > > see it.
> > > - Fix the ttyfree() routine used by tty device drivers in their det=
ach
> > > methods to use ttyrel() on the tty so we don't leak them. Also, =
fix=20
> 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=
=20
> handler.
> > > =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
> >=20
>=20
>=20
>=20
> --=20
> John Baldwin
--NQqDiQVflaif2hna
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (FreeBSD)
iEYEARECAAYFAkj+I4gACgkQC3+MBN1Mb4j9VgCfZVAFTHhlbnODTqhmOvaZy5eh
kVMAoPN6U1RdpPHQYoFgpWkAcos4hQku
=iAlL
-----END PGP SIGNATURE-----
--NQqDiQVflaif2hna--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081021184632.GU94031>