Hi Brian, On 02/28/2016 07:56 PM, Brian Bloniarz wrote: > (Take 3, fix compile error in n_hdlc.c) > > Hi Peter, I saw Marc Aurele La France's proposed patch to n_tty to fix > OpenSSH, and your feedback. Patch below is an attempt to address that > feedback. Please let me know if this is the change you envisioned; > (see Marc's excellent original writeup for details on the issue). > > [PATCH] n_tty: wait for buffer work in read() and poll(). > > Undoes the following four changes: > > 1) f95499c3030fe1bfad57745f2db1959c5b43dca8 > n_tty: Don't wait for buffer work in read() loop > > 2) f8747d4a466ab2cafe56112c51b3379f9fdb7a12 > tty: Fix pty master read() after slave closes > > 3) 52bce7f8d4fc633c9a9d0646eef58ba6ae9a3b73 > pty, n_tty: Simplify input processing on final close > > 4) 1a48632ffed61352a7810ce089dc5a8bcd505a60 > pty: Fix input race when closing > > These changes caused a regression in OpenSSH, as it assumes that the > first read() to return EAGAIN after a SIGCHLD means that all the child's > output has been returned.
This is not descriptive enough of the requirements of OpenSSH. For example, it fails to note that the attempted read() is non-blocking for which EAGAIN is an expected result that doesn't not mean the slave-side has been closed. Something like: Fix OpenSSH pty regression on close OpenSSH expects the (non-blocking) read() of pty master to return EAGAIN only if it has received all of the slave-side output after it has received SIGCHLD. This used to work on pre-3.12 kernels. This fix effectively forces non-blocking read() and poll() to block for parallel i/o to complete for all ttys. ... > Inspired by analysis and patch from Marc Aurele La France <t...@tuyoix.net> > > Reported-by: Volth <open...@volth.com> > Reported-by: Marc Aurele La France <t...@tuyoix.net> > BugLink: https://bugzilla.mindrot.org/show_bug.cgi?id=52 > BugLink: https://bugzilla.mindrot.org/show_bug.cgi?id=2492 > Signed-off-by: Brian Bloniarz <brian.bloni...@gmail.com> > --- > Documentation/serial/tty.txt | 3 --- > drivers/tty/n_hdlc.c | 4 ++-- > drivers/tty/n_tty.c | 34 +++++++++++++++------------------- > drivers/tty/pty.c | 4 +--- > drivers/tty/tty_buffer.c | 29 +---------------------------- > include/linux/tty.h | 1 - > 6 files changed, 19 insertions(+), 56 deletions(-) > > diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt > index bc3842d..e2dea3d 100644 > --- a/Documentation/serial/tty.txt > +++ b/Documentation/serial/tty.txt > @@ -213,9 +213,6 @@ TTY_IO_ERROR If set, causes all subsequent > userspace read/write > > TTY_OTHER_CLOSED Device is a pty and the other side has closed. > > -TTY_OTHER_DONE Device is a pty and the other side has closed > and > - all pending input processing has been completed. > - > TTY_NO_WRITE_SPLIT Prevent driver from splitting up writes into > smaller chunks. > > diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c > index bbc4ce6..644ddb8 100644 > --- a/drivers/tty/n_hdlc.c > +++ b/drivers/tty/n_hdlc.c > @@ -600,7 +600,7 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, > struct file *file, > add_wait_queue(&tty->read_wait, &wait); > > for (;;) { > - if (test_bit(TTY_OTHER_DONE, &tty->flags)) { > + if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) { > ret = -EIO; > break; > } > @@ -828,7 +828,7 @@ static unsigned int n_hdlc_tty_poll(struct tty_struct > *tty, struct file *filp, > /* set bits for operations that won't block */ > if (n_hdlc->rx_buf_list.head) > mask |= POLLIN | POLLRDNORM; /* readable */ > - if (test_bit(TTY_OTHER_DONE, &tty->flags)) > + if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) > mask |= POLLHUP; > if (tty_hung_up_p(filp)) > mask |= POLLHUP; > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index b280abaa..fc04011 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -1952,10 +1952,20 @@ err: > return -ENOMEM; > } > > +/** > + * Synchronously pushes the terminal flip buffers to the line discipline > + * and checks for available data. No, see below. > + * > + * Must not be called from IRQ context. > + */ > static inline int input_available_p(struct tty_struct *tty, int poll) > { > struct n_tty_data *ldata = tty->disc_data; > - int amt = poll && !TIME_CHAR(tty) && MIN_CHAR(tty) ? MIN_CHAR(tty) : 1; > + int amt; > + > + flush_work(&tty->port->buf.work); This is not necessary and can deadlock. The wait for buffer work to finish is only necessary input_available_p() returns false. Then in n_tty_read() the termios lock needs to be dropped before waiting for buffer work and rechecking input_available_p(). Please test your patches with lockdep enabled. Also, abstract the naked flush_work(), similar to tty_buffer_cancel_work(). > + > + amt = poll && !TIME_CHAR(tty) && MIN_CHAR(tty) ? MIN_CHAR(tty) : 1; > > if (ldata->icanon && !L_EXTPROC(tty)) > return ldata->canon_head != ldata->read_tail; > @@ -1963,18 +1973,6 @@ static inline int input_available_p(struct tty_struct > *tty, int poll) > return ldata->commit_head - ldata->read_tail >= amt; > } > > -static inline int check_other_done(struct tty_struct *tty) > -{ > - int done = test_bit(TTY_OTHER_DONE, &tty->flags); > - if (done) { > - /* paired with cmpxchg() in check_other_closed(); ensures > - * read buffer head index is not stale > - */ > - smp_mb__after_atomic(); > - } > - return done; > -} > - > /** > * copy_from_read_buf - copy read data directly > * @tty: terminal device > @@ -2170,7 +2168,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, > struct file *file, > struct n_tty_data *ldata = tty->disc_data; > unsigned char __user *b = buf; > DEFINE_WAIT_FUNC(wait, woken_wake_function); > - int c, done; > + int c; > int minimum, time; > ssize_t retval = 0; > long timeout; > @@ -2238,10 +2236,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, > struct file *file, > ((minimum - (b - buf)) >= 1)) > ldata->minimum_to_wake = (minimum - (b - buf)); > > - done = check_other_done(tty); > - > if (!input_available_p(tty, 0)) { > - if (done) { > + if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) { > retval = -EIO; > break; > } > @@ -2445,12 +2441,12 @@ static unsigned int n_tty_poll(struct tty_struct > *tty, struct file *file, > > poll_wait(file, &tty->read_wait, wait); > poll_wait(file, &tty->write_wait, wait); > - if (check_other_done(tty)) > - mask |= POLLHUP; > if (input_available_p(tty, 1)) > mask |= POLLIN | POLLRDNORM; > if (tty->packet && tty->link->ctrl_status) > mask |= POLLPRI | POLLIN | POLLRDNORM; > + if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) > + mask |= POLLHUP; > if (tty_hung_up_p(file)) > mask |= POLLHUP; > if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) { > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > index 2348fa6..6427a39 100644 > --- a/drivers/tty/pty.c > +++ b/drivers/tty/pty.c > @@ -59,7 +59,7 @@ static void pty_close(struct tty_struct *tty, struct file > *filp) > if (!tty->link) > return; > set_bit(TTY_OTHER_CLOSED, &tty->link->flags); > - tty_flip_buffer_push(tty->link->port); > + 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); > @@ -247,9 +247,7 @@ static int pty_open(struct tty_struct *tty, struct file > *filp) > goto out; > > clear_bit(TTY_IO_ERROR, &tty->flags); > - /* TTY_OTHER_CLOSED must be cleared before TTY_OTHER_DONE */ > clear_bit(TTY_OTHER_CLOSED, &tty->link->flags); > - clear_bit(TTY_OTHER_DONE, &tty->link->flags); > set_bit(TTY_THROTTLED, &tty->flags); > return 0; > > diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c > index 3cd31e0..3c0e914 100644 > --- a/drivers/tty/tty_buffer.c > +++ b/drivers/tty/tty_buffer.c > @@ -37,29 +37,6 @@ > > #define TTY_BUFFER_PAGE (((PAGE_SIZE - sizeof(struct tty_buffer)) / 2) > & ~0xFF) > > -/* > - * If all tty flip buffers have been processed by flush_to_ldisc() or > - * dropped by tty_buffer_flush(), check if the linked pty has been closed. > - * If so, wake the reader/poll to process > - */ > -static inline void check_other_closed(struct tty_struct *tty) > -{ > - unsigned long flags, old; > - > - /* transition from TTY_OTHER_CLOSED => TTY_OTHER_DONE must be atomic */ > - for (flags = ACCESS_ONCE(tty->flags); > - test_bit(TTY_OTHER_CLOSED, &flags); > - ) { > - old = flags; > - __set_bit(TTY_OTHER_DONE, &flags); > - flags = cmpxchg(&tty->flags, old, flags); > - if (old == flags) { > - wake_up_interruptible(&tty->read_wait); > - break; > - } > - } > -} > - > /** > * tty_buffer_lock_exclusive - gain exclusive access to buffer > * tty_buffer_unlock_exclusive - release exclusive access > @@ -254,8 +231,6 @@ void tty_buffer_flush(struct tty_struct *tty, struct > tty_ldisc *ld) > if (ld && ld->ops->flush_buffer) > ld->ops->flush_buffer(tty); > > - check_other_closed(tty); > - > atomic_dec(&buf->priority); > mutex_unlock(&buf->lock); > } > @@ -505,10 +480,8 @@ static void flush_to_ldisc(struct work_struct *work) > */ > count = smp_load_acquire(&head->commit) - head->read; > if (!count) { > - if (next == NULL) { > - check_other_closed(tty); > + if (next == NULL) > break; > - } > buf->head = next; > tty_buffer_free(port, head); > continue; > diff --git a/include/linux/tty.h b/include/linux/tty.h > index d9fb4b0..af18365 100644 > --- a/include/linux/tty.h > +++ b/include/linux/tty.h > @@ -338,7 +338,6 @@ struct tty_file_private { > #define TTY_EXCLUSIVE 3 /* Exclusive open mode */ > #define TTY_DEBUG 4 /* Debugging */ > #define TTY_DO_WRITE_WAKEUP 5 /* Call write_wakeup after queuing new > */ > -#define TTY_OTHER_DONE 6 /* Closed pty has completed > input processing */ > #define TTY_LDISC_OPEN 11 /* Line discipline is open */ > #define TTY_PTY_LOCK 16 /* pty private */ > #define TTY_NO_WRITE_SPLIT 17 /* Preserve write boundaries to driver > */ >
