svn commit: r259441 - head/sys/kern

[Jilles Tjoelker]

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