From owner-freebsd-bugs@FreeBSD.ORG Tue Oct 21 18:46:37 2008 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4A4DC1065699; Tue, 21 Oct 2008 18:46:37 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.terabit.net.ua (mail.terabit.net.ua [195.137.202.147]) by mx1.freebsd.org (Postfix) with ESMTP id CF5508FC19; Tue, 21 Oct 2008 18:46:36 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from skuns.zoral.com.ua ([91.193.166.194] helo=mail.zoral.com.ua) by mail.terabit.net.ua with esmtps (TLSv1:AES256-SHA:256) (Exim 4.63 (FreeBSD)) (envelope-from ) id 1KsMFX-000IAc-Uc; Tue, 21 Oct 2008 21:46:35 +0300 Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id m9LIkWP0034856 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 21 Oct 2008 21:46:33 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.3/8.14.3) with ESMTP id m9LIkWt3057534; Tue, 21 Oct 2008 21:46:32 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.3/8.14.3/Submit) id m9LIkWvK057532; Tue, 21 Oct 2008 21:46:32 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 21 Oct 2008 21:46:32 +0300 From: Kostik Belousov To: John Baldwin Message-ID: <20081021184632.GU94031@deviant.kiev.zoral.com.ua> References: <20081021092132.GA1594@tmn.ru> <200810211341.40856.jhb@freebsd.org> <20081021182912.GS94031@deviant.kiev.zoral.com.ua> <200810211437.18048.jhb@freebsd.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="NQqDiQVflaif2hna" Content-Disposition: inline In-Reply-To: <200810211437.18048.jhb@freebsd.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: ClamAV version 0.93.3, clamav-milter version 0.93.3 on skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua X-Virus-Scanned: mail.terabit.net.ua 1KsMFX-000IAc-Uc 776c8f08c975625e3ed66be9d69be34b X-Terabit: YES Cc: re@freebsd.org, freebsd-bugs@freebsd.org Subject: Re: kern/89538: [tty] [panic] triggered by "sysctl -a" X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Oct 2008 18:46:37 -0000 --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--