[BUG] tty: Userland can create hung tasks

From: Tejun Heo
Date: Tue Oct 31 2017 - 22:07:05 EST

Next message: Baolin Wang: "[PATCH v2 1/2] regmap: Add hardware spinlock support"
Previous message: Shankara Pailoor: "Re: KASAN: use-after-free in move_expired_inodes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Baolin Wang: "[PATCH v2 1/2] regmap: Add hardware spinlock support"
Previous message: Shankara Pailoor: "Re: KASAN: use-after-free in move_expired_inodes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]