[BUG] tty: Userland can create hung tasks

[BUG] tty: Userland can create hung tasks

[Tejun Heo]

Hello,

tty hangup code doesn't mark the console as being HUPed for, e.g.,
/dev/console and that can put the session leader trying to
disassociate from the controlling terminal in an indefinite D sleep.

Looking at the code, I have no idea why some tty devices are never
marked being hung up.  It *looks* intentional and dates back to the
git origin but I couldn't find any clue.  The following patch is a
workaround which fixes the observed problem but definitely isn't the
proper fix.

For details, please read the patch description.  If you scroll down,
there's a reproducer too.

Thanks.

------ 8< ------
Subject: [PATCH] tty: make n_tty_read() always abort if hangup is in progress

__tty_hangup() sets file->f_op to hung_up_tty_fops iff the write
operation is tty_write(), which means that, for example, hanging up
/dev/console doesn't clear its f_op as its write op is
redirected_tty_write().

tty_hung_up_p() tests f_op for hung_up_tty_fops to determine whether
the terminal is (being) hung up.  In turn, n_tty_read() uses this test
to decide whether readers should abort due to hangup.

Combined, this means that n_tty_read() can't tell whether /dev/console
is being hung up or not.  This can lead to the following scenario.

 1. A session contains two processes.  The leader and its child.  The
    child ignores SIGHUP.

 2. The leader exits and starts disassociating from the controlling
    terminal (/dev/console).

 3. __tty_hangup() skips setting f_op to hung_up_tty_fops.

 4. SIGHUP is delivered and ignored.

 5. tty_ldisc_hangup() is invoked.  It wakes up the waits which should
    clear the read lockers of tty->ldisc_sem.

 6. The reader wakes up but because tty_hung_up_p() is false, it
    doesn't abort and goes back to sleep while read-holding
    tty->ldisc_sem.

 7. The leader progresses to tty_ldisc_lock() in tty_ldisc_hangup()
    and is now stuck in D sleep indefinitely waiting for
    tty->ldisc_sem.

This leads to hung task warnings like the following.

  [  492.713289] INFO: task agetty:2662 blocked for more than 120 seconds.
  [  492.726170]       Not tainted 4.11.3-dbg-tty-lockup-02478-gfd6c7ee-dirty #28
  [  492.740264] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  [  492.755919]     0  2662      1 0x00000086
  [  492.763940] Call Trace:
  [  492.768834]  __schedule+0x267/0x890
  [  492.775816]  schedule+0x36/0x80
  [  492.782094]  schedule_timeout+0x23c/0x2e0
  [  492.790120]  ldsem_down_write+0xce/0x1f6
  [  492.797974]  tty_ldisc_lock+0x16/0x30
  [  492.805300]  tty_ldisc_hangup+0xb3/0x1b0
  [  492.813143]  __tty_hangup+0x300/0x410
  [  492.820470]  disassociate_ctty+0x6c/0x290
  [  492.828486]  do_exit+0x7ef/0xb00
  [  492.834946]  do_group_exit+0x3f/0xa0
  [  492.842092]  get_signal+0x1b3/0x5d0
  [  492.849077]  do_signal+0x28/0x660
  [  492.855720]  ? __fput+0x174/0x1e0
  [  492.862353]  ? __audit_syscall_exit+0x1f3/0x280
  [  492.871402]  exit_to_usermode_loop+0x46/0x86
  [  492.879926]  do_syscall_64+0x9c/0xb0
  [  492.887073]  entry_SYSCALL64_slow_path+0x25/0x25
  [  492.896295] RIP: 0033:0x7f69b3e7f783
  [  492.903438] RSP: 002b:00007ffdcb249ca8 EFLAGS: 00000246
  [  492.913868]  ORIG_RAX: 0000000000000017
  [  492.921536] RAX: fffffffffffffdfe RBX: 00007ffdcb249cd0 RCX: 00007f69b3e7f783
  [  492.935786] RDX: 0000000000000000 RSI: 00007ffdcb249da0 RDI: 0000000000000005
  [  492.950034] RBP: 00007ffdcb249e20 R08: 0000000000000000 R09: 00007ffdcb249c60
  [  492.964284] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
  [  492.964285] R13: 0000000000000004 R14: 0000000000000100 R15: 00007ffdcb24b750

This patch works around the issue by marking that hangup is in
progress in tty->flags regardless of the tty type and make
n_tty_read() abort accordingly.  This isn't a proper fix but does work
around the observed problem.

The following is the repro.  Run "$PROG /dev/console".  The parent
process hangs in D state.

  #include <sys/types.h>
  #include <sys/stat.h>
  #include <sys/wait.h>
  #include <sys/ioctl.h>
  #include <fcntl.h>
  #include <unistd.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <errno.h>
  #include <signal.h>
  #include <time.h>
  #include <termios.h>

  int main(int argc, char **argv)
  {
	  struct sigaction sact = { .sa_handler = SIG_IGN };
	  struct timespec ts1s = { .tv_sec = 1 };
	  pid_t pid;
	  int fd;

	  if (argc < 2) {
		  fprintf(stderr, "test-hung-tty /dev/$TTY\n");
		  return 1;
	  }

	  /* fork a child to ensure that it isn't already the session leader */
	  pid = fork();
	  if (pid < 0) {
		  perror("fork");
		  return 1;
	  }

	  if (pid > 0) {
		  /* top parent, wait for everyone */
		  while (waitpid(-1, NULL, 0) >= 0)
			  ;
		  if (errno != ECHILD)
			  perror("waitpid");
		  return 0;
	  }

	  /* new session, start a new session and set the controlling tty */
	  if (setsid() < 0) {
		  perror("setsid");
		  return 1;
	  }

	  fd = open(argv[1], O_RDWR);
	  if (fd < 0) {
		  perror("open");
		  return 1;
	  }

	  if (ioctl(fd, TIOCSCTTY, 1) < 0) {
		  perror("ioctl");
		  return 1;
	  }

	  /* fork a child, sleep a bit and exit */
	  pid = fork();
	  if (pid < 0) {
		  perror("fork");
		  return 1;
	  }

	  if (pid > 0) {
		  nanosleep(&ts1s, NULL);
		  printf("Session leader exiting\n");
		  exit(0);
	  }

	  /*
	   * The child ignores SIGHUP and keeps reading from the controlling
	   * tty.  Because SIGHUP is ignored, the child doesn't get killed on
	   * parent exit and the bug in n_tty makes the read(2) block the
	   * parent's control terminal hangup attempt.  The parent ends up in
	   * D sleep until the child is explicitly killed.
	   */
	  sigaction(SIGHUP, &sact, NULL);
	  printf("Child reading tty\n");
	  while (1) {
		  char buf[1024];

		  if (read(fd, buf, sizeof(buf)) < 0) {
			  perror("read");
			  return 1;
		  }
	  }

	  return 0;
  }

WORKAROUND_NOT_SIGNED_OFF
---
 drivers/tty/n_tty.c  | 3 ++-
 drivers/tty/tty_io.c | 3 +++
 include/linux/tty.h  | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index bdf0e6e..cb1e356 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2180,7 +2180,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 					retval = -EIO;
 					break;
 				}
-				if (tty_hung_up_p(file))
+				if (tty_hung_up_p(file) ||
+				    test_bit(TTY_HUPPING, &tty->flags))
 					break;
 				if (!timeout)
 					break;
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e6d1a65..012ac8a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -710,6 +710,8 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 		return;
 	}
 
+	set_bit(TTY_HUPPING, &tty->flags);
+
 	/* inuse_filps is protected by the single tty lock,
 	   this really needs to change if we want to flush the
 	   workqueue with the lock held */
@@ -764,6 +766,7 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 	 * from the ldisc side, which is now guaranteed.
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
+	clear_bit(TTY_HUPPING, &tty->flags);
 	tty_unlock(tty);
 
 	if (f)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904..bce2765 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -362,6 +362,7 @@ struct tty_file_private {
 #define TTY_PTY_LOCK 		16	/* pty private */
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
 #define TTY_HUPPED 		18	/* Post driver->hangup() */
+#define TTY_HUPPING		19	/* Hangup in progress */
 #define TTY_LDISC_HALTED	22	/* Line discipline is halted */
 
 /* Values for tty->flow_change */
-- 
2.9.5

Re: [BUG] tty: Userland can create hung tasks

[Alan Cox]

On Tue, 31 Oct 2017 19:06:51 -0700
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> tty hangup code doesn't mark the console as being HUPed for, e.g.,
> /dev/console and that can put the session leader trying to
> disassociate from the controlling terminal in an indefinite D sleep.
> 
> Looking at the code, I have no idea why some tty devices are never
> marked being hung up.  It *looks* intentional and dates back to the
> git origin but I couldn't find any clue.  The following patch is a
> workaround which fixes the observed problem but definitely isn't the
> proper fix.

Two reasons

1. It broke the serial consoles because they would hang up and close down
the hardware. With tty_port that *should* be fixable properly for any
cases remaining.

2. The console layer was (and still is) completely broken and doens't
refcount properly. So if you turn on console hangups it breaks (as indeed
does freeing consoles and half a dozen other things).

Sadly it seems that nobody cares about the console.

Alan

Re: [BUG] tty: Userland can create hung tasks

[Tejun Heo]

Hello, Alan.

On Wed, Nov 01, 2017 at 05:09:08PM +0000, Alan Cox wrote:
> On Tue, 31 Oct 2017 19:06:51 -0700
> Tejun Heo <tj@kernel.org> wrote:
> 
> > Hello,
> > 
> > tty hangup code doesn't mark the console as being HUPed for, e.g.,
> > /dev/console and that can put the session leader trying to
> > disassociate from the controlling terminal in an indefinite D sleep.
> > 
> > Looking at the code, I have no idea why some tty devices are never
> > marked being hung up.  It *looks* intentional and dates back to the
> > git origin but I couldn't find any clue.  The following patch is a
> > workaround which fixes the observed problem but definitely isn't the
> > proper fix.
> 
> Two reasons
> 
> 1. It broke the serial consoles because they would hang up and close down
> the hardware. With tty_port that *should* be fixable properly for any
> cases remaining.
> 
> 2. The console layer was (and still is) completely broken and doens't
> refcount properly. So if you turn on console hangups it breaks (as indeed
> does freeing consoles and half a dozen other things).
> 
> Sadly it seems that nobody cares about the console.

Hmm... so, does something like the posted workaround look reasonable
to you?  It doesn't fix the actual hangup issue but gets rid of
processes getting stuck in D sleep indefinitely.

Thanks.

-- 
tejun