Re: [PATCH] [98/223] TTY: open/hangup race fixup

[Jiri Slaby <jslaby@suse.cz>]

On 12/13/2010 12:46 AM, Andi Kleen wrote:
> 2.6.35-longterm review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> From: Jiri Slaby <jslaby@suse.cz>
> 
> commit acfa747baf73922021a047f2d87a2d866f5dbab5 upstream.
> 
> Like in the "TTY: don't allow reopen when ldisc is changing" patch,
> this one fixes a TTY WARNING as described in the option 1) there:
> 1) __tty_hangup from tty_ldisc_hangup to tty_ldisc_enable. During this
> section tty_lock is held. However tty_lock is temporarily dropped in
> the middle of the function by tty_ldisc_hangup.
> 
> The fix is to introduce a new flag which we set during the unlocked
> window and check it in tty_reopen too. The flag is TTY_HUPPING and is
> cleared after TTY_HUPPED is set.
> 
> While at it, remove duplicate TTY_HUPPED set_bit. The one after
> calling ops->hangup seems to be more correct. But anyway, we hold
> tty_lock, so there should be no difference.
> 
> Also document the function it does that kind of crap.
> 
> Nicely reproducible with two forked children:
> static void do_work(const char *tty)
> {
> 	if (signal(SIGHUP, SIG_IGN) == SIG_ERR) exit(1);
> 	setsid();
> 	while (1) {
> 		int fd = open(tty, O_RDWR|O_NOCTTY);
> 		if (fd < 0) continue;
> 		if (ioctl(fd, TIOCSCTTY)) continue;
> 		if (vhangup()) continue;
> 		close(fd);
> 	}
> 	exit(0);
> }
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Reported-by: <Valdis.Kletnieks@vt.edu>
> Reported-by: Kyle McMartin <kyle@mcmartin.ca>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
>  drivers/char/tty_io.c |   10 +++++++++-
>  include/linux/tty.h   |    1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> Index: linux/drivers/char/tty_io.c
> ===================================================================
> --- linux.orig/drivers/char/tty_io.c
> +++ linux/drivers/char/tty_io.c
> @@ -514,7 +514,10 @@ static void do_tty_hangup(struct work_st
>  	spin_unlock(&redirect_lock);
>  
>  	/* inuse_filps is protected by the single kernel lock */
> +
>  	lock_kernel();
> +	/* some functions below drop BTM, so we need this bit */
> +	set_bit(TTY_HUPPING, &tty->flags);

This and 97/223 should not be needed in 2.6.35 -- there is BKL
protecting the race. IOW BKL removal in 2.6.36 introduced the regression
these 2 patches fix.

regards,
-- 
js
suse labs