On Mon, Nov 23, 2020 at 06:49:02PM -0600, miny...@acm.org wrote:
> From: Corey Minyard <cminy...@mvista.com>
>
> 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.
It's been a while, so ping on this. I'm pretty sure this is the right
fix, the more I've thought about it.
Thankks,
-corey
>
> 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 <cminy...@mvista.com>
> ---
> drivers/tty/pty.c | 15 +++++++++++++--
> drivers/tty/tty_io.c | 5 +++--
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index 23368cec7ee8..29be6b985e76 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -67,7 +67,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);
> @@ -76,7 +77,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 571b1d7d4d5a..91c33a0df3c4 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -547,7 +547,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)
> {
> @@ -562,7 +564,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
> --
> 2.25.1
>
