Re: [PATCH] [69/99] TTY: ldisc, do not close until there are readers

[Jiri Slaby <jslaby@suse.cz>]

On 07/28/2011 01:49 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 92f6fa09bd453ffe3351fa1f1377a1b7cfa911e6 upstream.
>
> We restored tty_ldisc_wait_idle in 100eeae2c5c (TTY: restore
> tty_ldisc_wait_idle). We used it in the ldisc changing path to fix the
> case where there are tasks in n_tty_read waiting for data and somebody
> tries to change ldisc.
>
> Similar to the case above, there may be also tasks waiting in
> n_tty_read while hangup is performed. As 65b770468e98 (tty-ldisc: turn
> ldisc user count into a proper refcount) removed the wait-until-idle
> from all paths, hangup path won't wait for them to disappear either
> now. So add it back even to the hangup path.
>
> There is a difference, we need uninterruptible sleep as there is
> obviously HUP signal pending. So tty_ldisc_wait_idle now sleeps
> without possibility to be interrupted. This is what original
> tty_ldisc_wait_idle did. After the wait idle reintroduction
> (100eeae2c5c), we have had interruptible sleeps for the ldisc changing
> path. But as there is a 5s timeout anyway, we don't allow it to be
> interrupted from now on. It's not worth the added complexity of
> deciding what kind of sleep we want.
>
> Before 65b770468e98 tty_ldisc_release was called also from
> tty_ldisc_release. It is called from tty_release, so I don't think we
> need to restore that one.
>
> This is nicely reproducible after constifying the timing when
> drivers/tty/n_tty.c is patched as follows ("TTY: ntty, add one more
> sanity check" patch is needed to actually see it explode):
> %% -1548,6 +1549,7 @@ static int n_tty_open(struct tty_struct *tty)
>
>          /* These are ugly. Currently a malloc failure here can panic */
>          if (!tty->read_buf) {
> +               msleep(100);
>                  tty->read_buf = kzalloc(N_TTY_BUF_SIZE, GFP_KERNEL);
>                  if (!tty->read_buf)
>                          return -ENOMEM;
> %% -1785,6 +1788,7 @@ do_it_again:
>                                  break;
>                          }
>                          timeout = schedule_timeout(timeout);
> +                       msleep(20);
>                          continue;
>                  }
>                  __set_current_state(TASK_RUNNING);
> ===== With a process: =====
>      while (1) {
>          int fd = open(argv[1], O_RDWR);
>          read(fd, buf, sizeof(buf));
>          close(fd);
>      }
> ===== and its child: =====
>          setsid();
>          while (1) {
>                  int fd = open(tty, O_RDWR|O_NOCTTY);
>                  ioctl(fd, TIOCSCTTY, 1);
>                  vhangup();
>                  close(fd);
>                  usleep(100 * (10 + random() % 1000));
>          }
> ===== EOF =====
>
> References: https://bugzilla.novell.com/show_bug.cgi?id=693374
> References: https://bugzilla.novell.com/show_bug.cgi?id=694509
> Signed-off-by: Jiri Slaby<jslaby@suse.cz>
> Cc: Alan Cox<alan@lxorguk.ukuu.org.uk>
> Cc: Linus Torvalds<torvalds@linux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman<gregkh@suse.de>
> Signed-off-by: Andi Kleen<ak@linux.intel.com>
>
> ---
>   drivers/char/tty_ldisc.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.35.y/drivers/char/tty_ldisc.c
> ===================================================================
> --- linux-2.6.35.y.orig/drivers/char/tty_ldisc.c
> +++ linux-2.6.35.y/drivers/char/tty_ldisc.c
> @@ -543,7 +543,7 @@ static int tty_ldisc_halt(struct tty_str
>   static int tty_ldisc_wait_idle(struct tty_struct *tty)
>   {
>   	int ret;
> -	ret = wait_event_interruptible_timeout(tty_ldisc_idle,
> +	ret = wait_event_timeout(tty_ldisc_idle,
>   			atomic_read(&tty->ldisc->users) == 1, 5 * HZ);
>   	if (ret<  0)
>   		return ret;
> @@ -750,6 +750,8 @@ static int tty_ldisc_reinit(struct tty_s
>   	if (IS_ERR(ld))
>   		return -1;
>
> +	WARN_ON_ONCE(tty_ldisc_wait_idle(tty));

Please hold on for backporting this into stable, the WARN_ON is reported 
to trigger in some cases.

> +
>   	tty_ldisc_close(tty, tty->ldisc);
>   	tty_ldisc_put(tty->ldisc);
>   	tty->ldisc = NULL;

regards,
-- 
js
suse labs