A read() from a pty master may mistakenly indicate EOF (errno == -EIO) after the pty slave has closed, even though input data remains to be read. For example,

       pty slave       |        input worker        |    pty master
                       |                            |
                       |                            |   n_tty_read()
pty_write()            |                            |     input avail? no
  add data             |                            |     sleep
  schedule worker  --->|                            |     .
                       |---> flush_to_ldisc()       |     .
pty_close()            |       fill read buffer     |     .
  wait for worker      |       wakeup reader    --->|     .
                       |       read buffer full?    |---> input avail ? yes
                       |<---   yes - exit worker    |     copy 4096 bytes to user
  TTY_OTHER_CLOSED <---|                            |<--- kick worker
                       |                            |

**** New read() before worker starts ****

Several conditions are required to trigger this race:
1. the ldisc read buffer must become full so the input worker exits
2. the read() count parameter must be >= 4096 so the ldisc read buffer is empty
3. the subsequent read() occurs before the kicked worker has processed more input

However, the underlying cause of the race is that data is pipelined, while tty state is not; ie., data already written by the pty slave end is not yet visible to the pty master end, but state changes by the pty slave end are visible to the pty master end immediately.

Pipeline the TTY_OTHER_CLOSED state through input worker to the reader.
1. Introduce TTY_OTHER_DONE which is set by the input worker when TTY_OTHER_CLOSED is set and either the input buffers are flushed or input processing has completed. Readers/polls are woken when TTY_OTHER_DONE is set.
2. Reader/poll checks TTY_OTHER_DONE instead of TTY_OTHER_CLOSED.
3. A new input worker is started from pty_close() after setting TTY_OTHER_CLOSED, which ensures the TTY_OTHER_DONE state will be set if the last input worker is already finished (or just about to exit).

Remove tty_flush_to_ldisc(); no in-tree callers.

Fixes 52bce7f8d4fc ("pty, n_tty: Simplify input processing on final close")

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=96311

BugLink http://bugs.launchpad.net/bugs/1429756

Reported-by Andy Whitcroft <[email protected]>

Reported-by H.J. Lu <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 Documentation/serial/tty.txt |    3 +++
 drivers/tty/n_hdlc.c         |    4 ++--
 drivers/tty/n_tty.c          |   22 ++++++++++++++++++----
 drivers/tty/pty.c            |    5 +++--
 drivers/tty/tty_buffer.c     |   41 +++++++++++++++++++++++++++--------------
 include/linux/tty.h          |    2 +-
 6 files changed, 54 insertions(+), 23 deletions(-)

Documentation/serial/tty.txt +3 -0

@@ -198,6 +198,9 @@ TTY_IO_ERROR		If set, causes all subsequ
 
 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.

drivers/tty/n_hdlc.c +2 -2

@@ -600,7 +600,7 @@ static ssize_t n_hdlc_tty_read(struct tt
 	add_wait_queue(&tty->read_wait, &wait);
 
 	for (;;) {
-		if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
+		if (test_bit(TTY_OTHER_DONE, &tty->flags)) {
 			ret = -EIO;
 			break;
 		}

@@ -828,7 +828,7 @@ static unsigned int n_hdlc_tty_poll(stru
 		/* set bits for operations that won't block */
 		if (n_hdlc->rx_buf_list.head)
 			mask |= POLLIN | POLLRDNORM;	/* readable */
-		if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
+		if (test_bit(TTY_OTHER_DONE, &tty->flags))
 			mask |= POLLHUP;
 		if (tty_hung_up_p(filp))
 			mask |= POLLHUP;

drivers/tty/n_tty.c +18 -4

@@ -1949,6 +1949,18 @@ static inline int input_available_p(stru
 		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

@@ -2167,7 +2179,7 @@ static ssize_t n_tty_read(struct tty_str
 	struct n_tty_data *ldata = tty->disc_data;
 	unsigned char __user *b = buf;
 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
-	int c;
+	int c, done;
 	int minimum, time;
 	ssize_t retval = 0;
 	long timeout;

@@ -2235,8 +2247,10 @@ static ssize_t n_tty_read(struct tty_str
 		    ((minimum - (b - buf)) >= 1))
 			ldata->minimum_to_wake = (minimum - (b - buf));
 
+		done = check_other_done(tty);
+
 		if (!input_available_p(tty, 0)) {
-			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
+			if (done) {
 				retval = -EIO;
 				break;
 			}

@@ -2443,12 +2457,12 @@ static unsigned int n_tty_poll(struct tt
 
 	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))) {

drivers/tty/pty.c +3 -2

@@ -53,9 +53,8 @@ static void pty_close(struct tty_struct
 	/* Review - krefs on tty_link ?? */
 	if (!tty->link)
 		return;
-	tty_flush_to_ldisc(tty->link);
 	set_bit(TTY_OTHER_CLOSED, &tty->link->flags);
-	wake_up_interruptible(&tty->link->read_wait);
+	tty_flip_buffer_push(tty->link->port);
 	wake_up_interruptible(&tty->link->write_wait);
 	if (tty->driver->subtype == PTY_TYPE_MASTER) {
 		set_bit(TTY_OTHER_CLOSED, &tty->flags);

@@ -243,7 +242,9 @@ static int pty_open(struct tty_struct *t
 		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;

drivers/tty/tty_buffer.c +27 ~~-14~~

66 lines not on this page · raw message

include/linux/tty.h +1 -1

@@ -339,6 +339,7 @@ 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 */

@@ -462,7 +463,6 @@ extern int tty_hung_up_p(struct file *fi
 extern void do_SAK(struct tty_struct *tty);
 extern void __do_SAK(struct tty_struct *tty);
 extern void no_tty(void);
-extern void tty_flush_to_ldisc(struct tty_struct *tty);
 extern void tty_buffer_free_all(struct tty_port *port);
 extern void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld);
 extern void tty_buffer_init(struct tty_port *port);

Patches currently in stable-queue which might be from [email protected] are

queue-4.0/pty-fix-input-race-when-closing.patch