Messages in this thread | | | Date | Sat, 2 Jan 2021 08:41:09 -0600 | From | Corey Minyard <> | Subject | Re: [PATCH 2/2] drivers:tty:pty: Fix a race causing data loss on close |
| |
On Mon, Nov 23, 2020 at 06:49:02PM -0600, minyard@acm.org wrote: > From: Corey Minyard <cminyard@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 <cminyard@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 >
| |