summaryrefslogtreecommitdiff
path: root/drivers/tty
diff options
context:
space:
mode:
authorCorey Minyard <cminyard@mvista.com>2020-11-24 03:49:02 +0300
committerGreg Kroah-Hartman <gregkh@linuxfoundation.org>2021-01-07 18:30:55 +0300
commit33d4ae98859873ddd49e22e4ca724387548b3d89 (patch)
tree5e54534503026b25dacdd199e686d33e74a8f680 /drivers/tty
parentf446776ebffb9d3fb145b8d84f7f358f64cb54d6 (diff)
downloadlinux-33d4ae98859873ddd49e22e4ca724387548b3d89.tar.xz
drivers:tty:pty: Fix a race causing data loss on close
Remove the tty_vhangup() from the pty code and just release the redirect. The tty_vhangup() results in data loss and data out of order issues. If you write to a pty master an immediately close the pty master, the receiver might get a chunk of data dropped, but then receive some later data. That's obviously something rather unexpected for a user. It certainly confused my test program. It turns out that tty_vhangup() on the slave pty gets called from pty_close(), and that causes the data on the slave side to be flushed, but due to races more data can be copied into the slave side's buffer after that. Consider the following sequence: thread1 thread2 thread3 ------- ------- ------- | |-write data into buffer, | | n_tty buffer is filled | | along with other buffers | |-pty_close(master) | |--tty_vhangup(slave) | |---tty_ldisc_hangup() | |----n_tty_flush_buffer() | |-----reset_buffer_flags() |-n_tty_read() | |--up_read(&tty->termios_rwsem); | |------down_read(&tty->termios_rwsem) | |------clear n_tty buffer contents | |------up_read(&tty->termios_rwsem) |--tty_buffer_flush_work() | |--schedules work calling | | flush_to_ldisc() | | |-flush_to_ldisc() | |--receive_buf() | |---tty_port_default_receive_buf() | |----tty_ldisc_receive_buf() | |-----n_tty_receive_buf2() | |------n_tty_receive_buf_common() | |-------down_read(&tty->termios_rwsem) | |-------__receive_buf() | | copies data into n_tty buffer | |-------up_read(&tty->termios_rwsem) |--down_read(&tty->termios_rwsem) |--copy buffer data to user >From this sequence, you can see that thread2 writes to the buffer then only clears the part of the buffer in n_tty. The n_tty receive buffer code then copies more data into the n_tty buffer. But part of the vhangup, releasing the redirect, is still required to avoid issues with consoles running on pty slaves. So do that. As far as I can tell, that is all that should be required. Signed-off-by: Corey Minyard <cminyard@mvista.com> Link: https://lore.kernel.org/r/20201124004902.1398477-3-minyard@acm.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Diffstat (limited to 'drivers/tty')
-rw-r--r--drivers/tty/pty.c15
-rw-r--r--drivers/tty/tty_io.c5
2 files changed, 16 insertions, 4 deletions
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 5e2374580e27..8b2797b6ee44 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -66,7 +66,8 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
wake_up_interruptible(&tty->link->read_wait);
wake_up_interruptible(&tty->link->write_wait);
if (tty->driver->subtype == PTY_TYPE_MASTER) {
- set_bit(TTY_OTHER_CLOSED, &tty->flags);
+ struct file *f;
+
#ifdef CONFIG_UNIX98_PTYS
if (tty->driver == ptm_driver) {
mutex_lock(&devpts_mutex);
@@ -75,7 +76,17 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
mutex_unlock(&devpts_mutex);
}
#endif
- tty_vhangup(tty->link);
+
+ /*
+ * This hack is required because a program can open a
+ * pty and redirect a console to it, but if the pty is
+ * closed and the console is not released, then the
+ * slave side will never close. So release the
+ * redirect when the master closes.
+ */
+ f = tty_release_redirect(tty->link);
+ if (f)
+ fput(f);
}
}
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 225f4933fbde..f19a34a93fe5 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -545,7 +545,9 @@ EXPORT_SYMBOL_GPL(tty_wakeup);
* @tty: tty device
*
* This is available to the pty code so if the master closes, if the
- * slave is a redirect it can release the redirect.
+ * slave is a redirect it can release the redirect. It returns the
+ * filp for the redirect, which must be fput when the operations on
+ * the tty are completed.
*/
struct file *tty_release_redirect(struct tty_struct *tty)
{
@@ -560,7 +562,6 @@ struct file *tty_release_redirect(struct tty_struct *tty)
return f;
}
-EXPORT_SYMBOL_GPL(tty_release_redirect);
/**
* __tty_hangup - actual handler for hangup events