From owner-svn-src-head@FreeBSD.ORG Sun Feb 16 23:03:10 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id ADB1498; Sun, 16 Feb 2014 23:03:10 +0000 (UTC) Received: from mx1.stack.nl (relay04.stack.nl [IPv6:2001:610:1108:5010::107]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 4113E1749; Sun, 16 Feb 2014 23:03:10 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id 6530CB806F; Mon, 17 Feb 2014 00:03:07 +0100 (CET) Received: by snail.stack.nl (Postfix, from userid 1677) id 4FCD728497; Mon, 17 Feb 2014 00:03:07 +0100 (CET) Date: Mon, 17 Feb 2014 00:03:07 +0100 From: Jilles Tjoelker To: Marcel Moolenaar , ed@FreeBSD.org Subject: Re: svn commit: r259441 - head/sys/kern Message-ID: <20140216230306.GA33995@stack.nl> References: <201312160050.rBG0oFjY071411@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201312160050.rBG0oFjY071411@svn.freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 16 Feb 2014 23:03:10 -0000 On Mon, Dec 16, 2013 at 12:50:15AM +0000, Marcel Moolenaar wrote: > Author: marcel > Date: Mon Dec 16 00:50:14 2013 > New Revision: 259441 > URL: http://svnweb.freebsd.org/changeset/base/259441 > Log: > Properly drain the TTY when both revoke(2) and close(2) end up closing > the TTY. In such a case, ttydev_close() is called multiple times and > each time, t_revokecnt is incremented and cv_broadcast() is called for > both the t_outwait and t_inwait condition variables. > Let's say revoke(2) comes in first and gets to call tty_drain() from > ttydev_leave(). Let's say that the revoke comes from init(8) as the > result of running "shutdown -r now". Since shutdown prints various > messages to the console before announing that the machine will reboot > immediately, let's also say that the output queue is not empty and > that tty_drain() has something to do. Let's assume this all happens > on a 9600 baud serial console, so it takes a time to drain. > The shutdown command will exit(2) and as such will end up closing > stdout. Let's say this close will come in second, bump t_revokecnt > and call tty_wakeup(). This has tty_wait() return prematurely and > the next thing that will happen is that the thread doing revoke(2) > will flush the TTY. Since the drain wasn't complete, the flush will > effectively drop whatever is left in t_outq. > This change takes into account that tty_drain() will return ERESTART > due to the fact that t_revokecnt was bumped and in that case simply > call tty_drain() again. The thread in question is already performing > the close so it can safely finish draining the TTY before destroying > the TTY structure. > Now all messages from shutdown will be printed on the serial console. > Obtained from: Juniper Networks, Inc. > Modified: > head/sys/kern/tty.c > Modified: head/sys/kern/tty.c > ============================================================================== > --- head/sys/kern/tty.c Sun Dec 15 23:49:42 2013 (r259440) > +++ head/sys/kern/tty.c Mon Dec 16 00:50:14 2013 (r259441) > @@ -191,8 +191,10 @@ ttydev_leave(struct tty *tp) > > /* Drain any output. */ > MPASS((tp->t_flags & TF_STOPPED) == 0); > - if (!tty_gone(tp)) > - tty_drain(tp); > + if (!tty_gone(tp)) { > + while (tty_drain(tp) == ERESTART) > + ; > + } > > ttydisc_close(tp); > Unfortunately, this is only correct if the [ERESTART] is because of the (tp->t_revokecnt != revokecnt) check. If cv_wait_sig() returned ERESTART, it will return ERESTART immediately the next time it is called, until the signal has been handled. Therefore, if a process performing the last close of a TTY via close(2) receives a signal with SA_RESTART set, it will spin until the data has drained. Formerly, the data would be discarded when the signal was received (without reflecting this in close(2)'s return value, since ttydev_leave() returns nothing and ttydev_close() always returns 0). One way to fix this would be to replicate the revokecnt check into ttydev_leave() so that tty_drain() is only retried if t_revokecnt changed. Discarding data because of a signal is not ideal, but I think leaving a process unkillable because of a drain that will never complete is worse. A process that cares about data in TTY buffers can use tcdrain(3) and handle signals and errors normally. -- Jilles Tjoelker