From owner-freebsd-bugs@FreeBSD.ORG Tue Oct 21 17:42:34 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 604851065771; Tue, 21 Oct 2008 17:42:34 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id D285B8FC16; Tue, 21 Oct 2008 17:42:33 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from localhost.corp.yahoo.com (john@localhost [IPv6:::1]) (authenticated bits=0) by server.baldwin.cx (8.14.2/8.14.2) with ESMTP id m9LHgQKm006777; Tue, 21 Oct 2008 13:42:27 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: "Sergey N. Voronkov" Date: Tue, 21 Oct 2008 13:41:40 -0400 User-Agent: KMail/1.9.7 References: <20081021092132.GA1594@tmn.ru> In-Reply-To: <20081021092132.GA1594@tmn.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200810211341.40856.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [IPv6:::1]); Tue, 21 Oct 2008 13:42:27 -0400 (EDT) X-Virus-Scanned: ClamAV 0.93.1/8461/Tue Oct 21 10:16:58 2008 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.6 required=4.2 tests=BAYES_00,NO_RELAYS autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: freebsd-bugs@freebsd.org, re@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 17:42:34 -0000 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. 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