[PATCH 1/1] pty, n_tty: continue processing input until the tty_buffer chain is flushed

[PATCH 1/1] pty, n_tty: continue processing input until the tty_buffer chain is flushed

[Andy Whitcroft]

In commit 52bce7f8d4fc ("pty, n_tty: Simplify input processing on
final close") the tty_flush_to_ldisc() data flush was moved from the
n_tty_input() path to the pty_close() path because the new locking ensured
that the data had already been copied:

However, this only guarantees that it will see _some_ of the pending
data, we cannot guarantee that all of the queued data will fit into the
output buffer.  When the output buffer fills the flush worker triggered
in pty_close will complete leaving the remaining data queued in the
tty_buffer chain.

When this occurs the reader will see the initial tranch of data correctly
and consume it.  As we return this to the caller we will spot the
additional pending data and trigger another flush worker.  So far so good.

However, we now have a race, if the consumer gets back into pty_read
before the flush worker is actually able to actually progress the queue,
it will see the see the tty input buffer empty, the other end marked
TTY_OTHER_CLOSED and return EIO to the caller even though data is en-route.

Fix this by ignoring TTY_OTHER_CLOSED while there remain unflushed buffers.

Fixes: 52bce7f8d4fc ("pty, n_tty: Simplify input processing on final close")
BugLink: http://bugs.launchpad.net/bugs/1429756
Cc: <stable@vger.kernel.org> # 3.19+
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/tty/n_tty.c      |  4 +++-
 drivers/tty/tty_buffer.c | 19 +++++++++++++++++++
 include/linux/tty_flip.h |  2 ++
 3 files changed, 24 insertions(+), 1 deletion(-)

	This was found during self-tests for the upstart init system which
	tests log handling by pumping large chunks of /dev/zero through
	to various logs in parallel.  This was failing with short files
	at random, which was traced back to this race.

	Build tested against v4.0-rc4, heavily run tested against v3.19.1.

	-apw

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index cf6e0f2..76b38f0 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -38,6 +38,7 @@
 #include <linux/sched.h>
 #include <linux/interrupt.h>
 #include <linux/tty.h>
+#include <linux/tty_flip.h>
 #include <linux/timer.h>
 #include <linux/ctype.h>
 #include <linux/mm.h>
@@ -2236,7 +2237,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 			ldata->minimum_to_wake = (minimum - (b - buf));
 
 		if (!input_available_p(tty, 0)) {
-			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
+			if (test_bit(TTY_OTHER_CLOSED, &tty->flags) &&
+			    !tty_data_pending_to_ldisc(tty)) {
 				retval = -EIO;
 				break;
 			}
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 7566164..04cfabb 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -424,6 +424,25 @@ receive_buf(struct tty_struct *tty, struct tty_buffer *head, int count)
 	return count;
 }
 
+int tty_data_pending_to_ldisc(struct tty_struct *tty)
+{
+	struct tty_bufhead *buf = &tty->port->buf;
+	struct tty_buffer *head = buf->head;
+
+	struct tty_buffer *next;
+	int count;
+
+	next = head->next;
+	/* paired w/ barrier in __tty_buffer_request_room();
+	 * ensures commit value read is not stale if the head
+	 * is advancing to the next buffer
+	 */
+	smp_rmb();
+	count = head->commit - head->read;
+
+	return (count || next);
+}
+
 /**
  *	flush_to_ldisc
  *	@work: tty structure passed from work queue.
diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
index c28dd52..a896b94 100644
--- a/include/linux/tty_flip.h
+++ b/include/linux/tty_flip.h
@@ -38,4 +38,6 @@ static inline int tty_insert_flip_string(struct tty_port *port,
 extern void tty_buffer_lock_exclusive(struct tty_port *port);
 extern void tty_buffer_unlock_exclusive(struct tty_port *port);
 
+extern int tty_data_pending_to_ldisc(struct tty_struct *tty);
+
 #endif /* _LINUX_TTY_FLIP_H */
-- 
1.9.1

Re: [PATCH 1/1] pty, n_tty: continue processing input until the tty_buffer chain is flushed

[Peter Hurley]

Hi Andy,

On 03/16/2015 10:30 AM, Andy Whitcroft wrote:
> In commit 52bce7f8d4fc ("pty, n_tty: Simplify input processing on
> final close") the tty_flush_to_ldisc() data flush was moved from the
> n_tty_input() path to the pty_close() path because the new locking ensured
> that the data had already been copied:
> 
> However, this only guarantees that it will see _some_ of the pending
> data, we cannot guarantee that all of the queued data will fit into the
> output buffer.  When the output buffer fills the flush worker triggered
> in pty_close will complete leaving the remaining data queued in the
> tty_buffer chain.
> 
> When this occurs the reader will see the initial tranch of data correctly
> and consume it.  As we return this to the caller we will spot the
> additional pending data and trigger another flush worker.  So far so good.
> 
> However, we now have a race, if the consumer gets back into pty_read
> before the flush worker is actually able to actually progress the queue,
> it will see the see the tty input buffer empty, the other end marked
> TTY_OTHER_CLOSED and return EIO to the caller even though data is en-route.
> 
> Fix this by ignoring TTY_OTHER_CLOSED while there remain unflushed buffers.
> 
> Fixes: 52bce7f8d4fc ("pty, n_tty: Simplify input processing on final close")
> BugLink: http://bugs.launchpad.net/bugs/1429756
> Cc: <stable@vger.kernel.org> # 3.19+
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/tty/n_tty.c      |  4 +++-
>  drivers/tty/tty_buffer.c | 19 +++++++++++++++++++
>  include/linux/tty_flip.h |  2 ++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> 	This was found during self-tests for the upstart init system which
> 	tests log handling by pumping large chunks of /dev/zero through
> 	to various logs in parallel.  This was failing with short files
> 	at random, which was traced back to this race.
> 
> 	Build tested against v4.0-rc4, heavily run tested against v3.19.1.
> 
> 	-apw

Thanks for discovering this bug.

I just managed to reproduce this problem with a test jig;
can you confirm that your self-tests also use >= 4096-byte read buffer
(which I think is necessary to trigger the worker race)?

Regards,
Peter Hurley

> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index cf6e0f2..76b38f0 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -38,6 +38,7 @@
>  #include <linux/sched.h>
>  #include <linux/interrupt.h>
>  #include <linux/tty.h>
> +#include <linux/tty_flip.h>
>  #include <linux/timer.h>
>  #include <linux/ctype.h>
>  #include <linux/mm.h>
> @@ -2236,7 +2237,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
>  			ldata->minimum_to_wake = (minimum - (b - buf));
>  
>  		if (!input_available_p(tty, 0)) {
> -			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
> +			if (test_bit(TTY_OTHER_CLOSED, &tty->flags) &&
> +			    !tty_data_pending_to_ldisc(tty)) {
>  				retval = -EIO;
>  				break;
>  			}
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 7566164..04cfabb 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -424,6 +424,25 @@ receive_buf(struct tty_struct *tty, struct tty_buffer *head, int count)
>  	return count;
>  }
>  
> +int tty_data_pending_to_ldisc(struct tty_struct *tty)
> +{
> +	struct tty_bufhead *buf = &tty->port->buf;
> +	struct tty_buffer *head = buf->head;
> +
> +	struct tty_buffer *next;
> +	int count;
> +
> +	next = head->next;
> +	/* paired w/ barrier in __tty_buffer_request_room();
> +	 * ensures commit value read is not stale if the head
> +	 * is advancing to the next buffer
> +	 */
> +	smp_rmb();
> +	count = head->commit - head->read;
> +
> +	return (count || next);
> +}
> +
>  /**
>   *	flush_to_ldisc
>   *	@work: tty structure passed from work queue.
> diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
> index c28dd52..a896b94 100644
> --- a/include/linux/tty_flip.h
> +++ b/include/linux/tty_flip.h
> @@ -38,4 +38,6 @@ static inline int tty_insert_flip_string(struct tty_port *port,
>  extern void tty_buffer_lock_exclusive(struct tty_port *port);
>  extern void tty_buffer_unlock_exclusive(struct tty_port *port);
>  
> +extern int tty_data_pending_to_ldisc(struct tty_struct *tty);
> +
>  #endif /* _LINUX_TTY_FLIP_H */
> 

Re: [PATCH 1/1] pty, n_tty: continue processing input until the tty_buffer chain is flushed

[Andy Whitcroft]

On Mon, Mar 16, 2015 at 02:03:23PM -0400, Peter Hurley wrote:

> I just managed to reproduce this problem with a test jig;
> can you confirm that your self-tests also use >= 4096-byte read buffer
> (which I think is necessary to trigger the worker race)?

The read which triggers the EIO is indeed using a 4096 byte buffer:

    1396 read(17, 0x7f78214c3b30, 4096) = -1 EIO (Input/output error)

We found a simpler reproduce by than the upstart selftest which is included
in the bug linked in the commit text.  Specifically in comment 13 [1].

Thanks for looking at this.

-apw

[1] https://bugs.launchpad.net/ubuntu/+source/upstart/+bug/1429756/comments/13

Re: [PATCH 1/1] pty, n_tty: continue processing input until the tty_buffer chain is flushed

[Peter Hurley]

Hi Andy,

On 03/16/2015 10:30 AM, Andy Whitcroft wrote:
> In commit 52bce7f8d4fc ("pty, n_tty: Simplify input processing on
> final close") the tty_flush_to_ldisc() data flush was moved from the
> n_tty_input() path to the pty_close() path because the new locking ensured
> that the data had already been copied:
> 
> However, this only guarantees that it will see _some_ of the pending
> data, we cannot guarantee that all of the queued data will fit into the
> output buffer.  When the output buffer fills the flush worker triggered
> in pty_close will complete leaving the remaining data queued in the
> tty_buffer chain.
> 
> When this occurs the reader will see the initial tranch of data correctly
> and consume it.  As we return this to the caller we will spot the
> additional pending data and trigger another flush worker.  So far so good.
> 
> However, we now have a race, if the consumer gets back into pty_read
> before the flush worker is actually able to actually progress the queue,
> it will see the see the tty input buffer empty, the other end marked
> TTY_OTHER_CLOSED and return EIO to the caller even though data is en-route.
> 
> Fix this by ignoring TTY_OTHER_CLOSED while there remain unflushed buffers.
> 
> Fixes: 52bce7f8d4fc ("pty, n_tty: Simplify input processing on final close")
> BugLink: http://bugs.launchpad.net/bugs/1429756
> Cc: <stable@vger.kernel.org> # 3.19+
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/tty/n_tty.c      |  4 +++-
>  drivers/tty/tty_buffer.c | 19 +++++++++++++++++++
>  include/linux/tty_flip.h |  2 ++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> 	This was found during self-tests for the upstart init system which
> 	tests log handling by pumping large chunks of /dev/zero through
> 	to various logs in parallel.  This was failing with short files
> 	at random, which was traced back to this race.
> 
> 	Build tested against v4.0-rc4, heavily run tested against v3.19.1.
> 
> 	-apw
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index cf6e0f2..76b38f0 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -38,6 +38,7 @@
>  #include <linux/sched.h>
>  #include <linux/interrupt.h>
>  #include <linux/tty.h>
> +#include <linux/tty_flip.h>
>  #include <linux/timer.h>
>  #include <linux/ctype.h>
>  #include <linux/mm.h>
> @@ -2236,7 +2237,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
>  			ldata->minimum_to_wake = (minimum - (b - buf));
>  
>  		if (!input_available_p(tty, 0)) {
> -			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
> +			if (test_bit(TTY_OTHER_CLOSED, &tty->flags) &&
> +			    !tty_data_pending_to_ldisc(tty)) {
>  				retval = -EIO;
>  				break;
>  			}
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 7566164..04cfabb 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -424,6 +424,25 @@ receive_buf(struct tty_struct *tty, struct tty_buffer *head, int count)
>  	return count;
>  }
>  
> +int tty_data_pending_to_ldisc(struct tty_struct *tty)
> +{
> +	struct tty_bufhead *buf = &tty->port->buf;
> +	struct tty_buffer *head = buf->head;
> +
> +	struct tty_buffer *next;
> +	int count;
> +
> +	next = head->next;
> +	/* paired w/ barrier in __tty_buffer_request_room();
> +	 * ensures commit value read is not stale if the head
> +	 * is advancing to the next buffer
> +	 */
> +	smp_rmb();
> +	count = head->commit - head->read;
> +
> +	return (count || next);

Thanks for trying to fix this.
Unfortunately, there's 2 problems with this approach:

1. Data in the tty buffers does not guarantee data will be generated
   in the read buffer (and thus cause this reader to wake up to handle
   new data). For example, if the remaining data is all non-output
   Ctrl values or the data has eraser sequences that results in no output,
   then the reader will hang.

2. flush_to_ldisc() does not update head->read in a manner that allows
   an observer to determine the flush_to_ldisc() worker state. So
   tty_data_pending_to_ldisc() could succeed when it should not have
   and again the reader will hang. For example, the flush_to_ldisc()
   input worker could have been preempted after waking the reader but
   before updating head->read (or even ldata->no_room), so if the read()
   completes and is re-issued, then it will observe no input_available(),
   TTY_OTHER_CLOSED and head->commit - head->read != 0.

I think the right way to fix this is to view the input worker as a pipeline
stage and to pipeline the signals with the data; I'm studying that solution
right now.

Regards,
Peter Hurley


> +}
> +
>  /**
>   *	flush_to_ldisc
>   *	@work: tty structure passed from work queue.
> diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
> index c28dd52..a896b94 100644
> --- a/include/linux/tty_flip.h
> +++ b/include/linux/tty_flip.h
> @@ -38,4 +38,6 @@ static inline int tty_insert_flip_string(struct tty_port *port,
>  extern void tty_buffer_lock_exclusive(struct tty_port *port);
>  extern void tty_buffer_unlock_exclusive(struct tty_port *port);
>  
> +extern int tty_data_pending_to_ldisc(struct tty_struct *tty);
> +
>  #endif /* _LINUX_TTY_FLIP_H */
> 

[PATCH] pty: Fix input race when closing

[Peter Hurley]

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 ****

                       |                            |   n_tty_read()
                       |                            |     input avail? no
                       |                            |     TTY_OTHER_CLOSED? yes
                       |                            |     return -EIO

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
Cc: <stable@vger.kernel.org> # 3.19+
Reported-by: Andy Whitcroft <apw@canonical.com>
Reported-by: H.J. Lu <hjl.tools@gmail.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 Documentation/serial/tty.txt |  3 +++
 drivers/tty/n_hdlc.c         |  4 ++--
 drivers/tty/n_tty.c          |  4 ++--
 drivers/tty/pty.c            |  3 +--
 drivers/tty/tty_buffer.c     | 25 +++++++++++--------------
 include/linux/tty.h          |  2 +-
 6 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt
index 1e52d67..dbe6623 100644
--- a/Documentation/serial/tty.txt
+++ b/Documentation/serial/tty.txt
@@ -198,6 +198,9 @@ 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 644ddb8..bbc4ce6 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_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(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_CLOSED, &tty->flags))
+		if (test_bit(TTY_OTHER_DONE, &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 54da8f4..522de6d 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2233,7 +2233,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 			ldata->minimum_to_wake = (minimum - (b - buf));
 
 		if (!input_available_p(tty, 0)) {
-			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
+			if (test_bit(TTY_OTHER_DONE, &tty->flags)) {
 				retval = -EIO;
 				break;
 			}
@@ -2444,7 +2444,7 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 		mask |= POLLIN | POLLRDNORM;
 	if (tty->packet && tty->link->ctrl_status)
 		mask |= POLLPRI | POLLIN | POLLRDNORM;
-	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
+	if (test_bit(TTY_OTHER_DONE, &tty->flags))
 		mask |= POLLHUP;
 	if (tty_hung_up_p(file))
 		mask |= POLLHUP;
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 6fffb53..ebcff90 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -59,9 +59,8 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 	/* 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);
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 7566164..642dcd0 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -229,6 +229,11 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
 	if (ld && ld->ops->flush_buffer)
 		ld->ops->flush_buffer(tty);
 
+	if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
+		set_bit(TTY_OTHER_DONE, &tty->flags);
+		wake_up_interruptible(&tty->read_wait);
+	}
+
 	atomic_dec(&buf->priority);
 	mutex_unlock(&buf->lock);
 }
@@ -471,8 +476,13 @@ static void flush_to_ldisc(struct work_struct *work)
 		smp_rmb();
 		count = head->commit - head->read;
 		if (!count) {
-			if (next == NULL)
+			if (next == NULL) {
+				if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
+					set_bit(TTY_OTHER_DONE, &tty->flags);
+					wake_up_interruptible(&tty->read_wait);
+				}
 				break;
+			}
 			buf->head = next;
 			tty_buffer_free(port, head);
 			continue;
@@ -489,19 +499,6 @@ static void flush_to_ldisc(struct work_struct *work)
 }
 
 /**
- *	tty_flush_to_ldisc
- *	@tty: tty to push
- *
- *	Push the terminal flip buffers to the line discipline.
- *
- *	Must not be called from IRQ context.
- */
-void tty_flush_to_ldisc(struct tty_struct *tty)
-{
-	flush_work(&tty->port->buf.work);
-}
-
-/**
  *	tty_flip_buffer_push	-	terminal
  *	@port: tty port to push
  *
diff --git a/include/linux/tty.h b/include/linux/tty.h
index f9fbdf1..0f29f31 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -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 *filp);
 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);
-- 
2.3.5

Re: [PATCH] pty: Fix input race when closing

[H.J. Lu]

On Thu, Apr 9, 2015 at 7:54 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> 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 ****
>
>                        |                            |   n_tty_read()
>                        |                            |     input avail? no
>                        |                            |     TTY_OTHER_CLOSED? yes
>                        |                            |     return -EIO
>
> 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
> Cc: <stable@vger.kernel.org> # 3.19+
> Reported-by: Andy Whitcroft <apw@canonical.com>
> Reported-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  Documentation/serial/tty.txt |  3 +++
>  drivers/tty/n_hdlc.c         |  4 ++--
>  drivers/tty/n_tty.c          |  4 ++--
>  drivers/tty/pty.c            |  3 +--
>  drivers/tty/tty_buffer.c     | 25 +++++++++++--------------
>  include/linux/tty.h          |  2 +-
>  6 files changed, 20 insertions(+), 21 deletions(-)
>

I tried it on 3.19.3 and it doesn't work with the testcase in

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


-- 
H.J.

Re: [PATCH] pty: Fix input race when closing

[Peter Hurley]

On 04/09/2015 01:43 PM, H.J. Lu wrote:
> On Thu, Apr 9, 2015 at 7:54 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> 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 ****
>>
>>                        |                            |   n_tty_read()
>>                        |                            |     input avail? no
>>                        |                            |     TTY_OTHER_CLOSED? yes
>>                        |                            |     return -EIO
>>
>> 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
>> Cc: <stable@vger.kernel.org> # 3.19+
>> Reported-by: Andy Whitcroft <apw@canonical.com>
>> Reported-by: H.J. Lu <hjl.tools@gmail.com>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>  Documentation/serial/tty.txt |  3 +++
>>  drivers/tty/n_hdlc.c         |  4 ++--
>>  drivers/tty/n_tty.c          |  4 ++--
>>  drivers/tty/pty.c            |  3 +--
>>  drivers/tty/tty_buffer.c     | 25 +++++++++++--------------
>>  include/linux/tty.h          |  2 +-
>>  6 files changed, 20 insertions(+), 21 deletions(-)
>>
> 
> I tried it on 3.19.3 and it doesn't work with the testcase in
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=96311

Can you test this patch on top of mainline? There's a couple of
fixes in 4.0-rc that specifically address weakly-ordered CPUs.

Regards,
Peter Hurley

Re: [PATCH] pty: Fix input race when closing

[H.J. Lu]

On Thu, Apr 9, 2015 at 10:53 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 04/09/2015 01:43 PM, H.J. Lu wrote:
>> On Thu, Apr 9, 2015 at 7:54 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> 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 ****
>>>
>>>                        |                            |   n_tty_read()
>>>                        |                            |     input avail? no
>>>                        |                            |     TTY_OTHER_CLOSED? yes
>>>                        |                            |     return -EIO
>>>
>>> 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
>>> Cc: <stable@vger.kernel.org> # 3.19+
>>> Reported-by: Andy Whitcroft <apw@canonical.com>
>>> Reported-by: H.J. Lu <hjl.tools@gmail.com>
>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>>> ---
>>>  Documentation/serial/tty.txt |  3 +++
>>>  drivers/tty/n_hdlc.c         |  4 ++--
>>>  drivers/tty/n_tty.c          |  4 ++--
>>>  drivers/tty/pty.c            |  3 +--
>>>  drivers/tty/tty_buffer.c     | 25 +++++++++++--------------
>>>  include/linux/tty.h          |  2 +-
>>>  6 files changed, 20 insertions(+), 21 deletions(-)
>>>
>>
>> I tried it on 3.19.3 and it doesn't work with the testcase in
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=96311
>
> Can you test this patch on top of mainline? There's a couple of
> fixes in 4.0-rc that specifically address weakly-ordered CPUs.
>

I will give it a try.


-- 
H.J.

Re: [PATCH] pty: Fix input race when closing

[H.J. Lu]

On Thu, Apr 9, 2015 at 10:53 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 04/09/2015 01:43 PM, H.J. Lu wrote:
>> On Thu, Apr 9, 2015 at 7:54 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> 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 ****
>>>
>>>                        |                            |   n_tty_read()
>>>                        |                            |     input avail? no
>>>                        |                            |     TTY_OTHER_CLOSED? yes
>>>                        |                            |     return -EIO
>>>
>>> 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
>>> Cc: <stable@vger.kernel.org> # 3.19+
>>> Reported-by: Andy Whitcroft <apw@canonical.com>
>>> Reported-by: H.J. Lu <hjl.tools@gmail.com>
>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>>> ---
>>>  Documentation/serial/tty.txt |  3 +++
>>>  drivers/tty/n_hdlc.c         |  4 ++--
>>>  drivers/tty/n_tty.c          |  4 ++--
>>>  drivers/tty/pty.c            |  3 +--
>>>  drivers/tty/tty_buffer.c     | 25 +++++++++++--------------
>>>  include/linux/tty.h          |  2 +-
>>>  6 files changed, 20 insertions(+), 21 deletions(-)
>>>
>>
>> I tried it on 3.19.3 and it doesn't work with the testcase in
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=96311
>
> Can you test this patch on top of mainline? There's a couple of
> fixes in 4.0-rc that specifically address weakly-ordered CPUs.

I tried it on 4.0.0-rc7 and it doesn't work.

-- 
H.J.