[PATCH 0/3] Fix buffer flush in signal char handling

[PATCH 0/3] Fix buffer flush in signal char handling

[Peter Hurley]

Greg,

This series fixes a long-standing problem with Ctrl-C interruption
of pty output. A while back I tried to fix this in a really ugly
way that Alan ixnay'd.

Quick background: the FIXME in pty_flush_buffer() relates to the
potential for deadlock if tty_buffer_flush() is done in parallel
by both ptys. The two potential sources for deadlock were the
tty->ldisc_sem and the tty_buffer lock.

The first source of deadlock, the tty->ldisc_sem, can/is avoided
by not waiting for the ldisc ref; ie., the buffer flush is simply
not performed if the ldisc ref is not acquired. In this context,
not acquiring the ldisc_sem is only possible when changing the
line discipline. Although this approach is not perfect, I feel
it's a > 99% solution.

The second source of deadlock stems from the fact that the
input worker is holding the tty buffer lock while processing
input. While this prevents a concurrent buffer flush of its
own tty buffer, this also threatens a lock order inversion:

 CPU 0                                 | CPU 1
 flush_to_ldisc()                      | flush_to_ldisc()
   claim slave buffer lock             |   claim master buffer lock
   - process input -                   |   - process input -
   n_tty_receive_signal_char           |   n_tty_receive_signal_char
     tty_driver_flush_buffer           |     tty_driver_flush_buffer
       pty_flush_buffer                |       pty_flush_buffer
         tty_buffer_flush(other)       |         tty_buffer_flush(other)
           try to claim other buf lock |           try to claim other buf lock

                                 * DEADLOCK *

_Except_ the CPU1 call chain is not possible because the
pty master can never have termios settings of either BRKINT or
ISIG, so the lock order inversion is not possible.

So patch 2 re-enables the tty_buffer_flush() in pty_flush_buffer() and
adds lockdep annotation for the tty buffer lock; normal ttys and pty
masters use the subclass 0 lock and pty slaves use the subclass 1 lock.

This will still warn of any lock order inversions that are actually
a problem.

Patch 1 just generalizes the existing lockdep annotation used for the
tty_lock(), so that patch 2 and 3 can use it.

Patch 3 fixes the very obvious problems that shows up when pty slaves start
flushing data; all kinds of i/o ordering issues show up. For example, if you
interrupt a 'cat large-text-file', the echoed '^C' appears out-of-order, or a
chunk of output is missing before the trailing out (because that is now
flushed by pty_flush_buffer() but the writer hasn't processed the signal yet).
FWIW, these same problems exist with regular ttys, but the speed of ptys
makes it way more obvious.

Regards,

Peter Hurley (3):
  tty: Make lock subclasses available for other tty locks
  pty: Fix buffer flush deadlock
  n_tty: Fix signal handling flushes

 drivers/tty/n_tty.c      | 45 ++++++++++++++++++++++++++++++---------------
 drivers/tty/pty.c        | 11 ++++++++++-
 drivers/tty/tty_buffer.c |  6 ++++++
 drivers/tty/tty_mutex.c  | 12 +-----------
 include/linux/tty.h      | 24 ++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 27 deletions(-)

-- 
2.2.2

[PATCH 1/3] tty: Make lock subclasses available for other tty locks

[Peter Hurley]

Besides nested legacy_mutex locking which is required on pty pair
teardown, other nested pty operations require lock subclassing.

Move lock subclass definition to tty interface header, include/linux/tty.h,
and document its use.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_mutex.c | 12 +-----------
 include/linux/tty.h     | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index a872389..0efcf71 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -4,18 +4,8 @@
 #include <linux/semaphore.h>
 #include <linux/sched.h>
 
-/*
- * Nested tty locks are necessary for releasing pty pairs.
- * The stable lock order is master pty first, then slave pty.
- */
-
 /* Legacy tty mutex glue */
 
-enum {
-	TTY_MUTEX_NORMAL,
-	TTY_MUTEX_SLAVE,
-};
-
 /*
  * Getting the big tty mutex.
  */
@@ -58,5 +48,5 @@ void __lockfunc tty_unlock_slave(struct tty_struct *tty)
 
 void tty_set_lock_subclass(struct tty_struct *tty)
 {
-	lockdep_set_subclass(&tty->legacy_mutex, TTY_MUTEX_SLAVE);
+	lockdep_set_subclass(&tty->legacy_mutex, TTY_LOCK_SLAVE);
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 09da425..4c1c453 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -14,6 +14,23 @@
 #include <linux/llist.h>
 
 
+/*
+ * Lock subclasses for tty locks
+ *
+ * TTY_LOCK_NORMAL is for normal ttys and master ptys.
+ * TTY_LOCK_SLAVE is for slave ptys only.
+ *
+ * Lock subclasses are necessary for handling nested locking with pty pairs.
+ * tty locks which use nested locking:
+ *
+ * legacy_mutex - Nested tty locks are necessary for releasing pty pairs.
+ *		  The stable lock order is master pty first, then slave pty.
+ */
+
+enum {
+	TTY_LOCK_NORMAL = 0,
+	TTY_LOCK_SLAVE,
+};
 
 /*
  * (Note: the *_driver.minor_start values 1, 64, 128, 192 are
-- 
2.2.2

[PATCH 2/3] pty: Fix buffer flush deadlock

[Peter Hurley]

The pty driver does not clear its write buffer when commanded.
This is to avoid an apparent deadlock between parallel flushes from
both pty ends; specifically when handling either BRK or INTR input.
However, parallel flushes from this source is not possible since
the pty master can never be set to BRKINT or ISIG. Parallel flushes
from other sources are possible but these do not threaten deadlocks.

Annotate the tty buffer mutex for lockdep to represent the nested
tty_buffer locking which occurs when the pty slave is processing input
(its buffer mutex held) and receives INTR or BRK and acquires the
linked tty buffer mutex via tty_buffer_flush().

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c        | 10 +++++++++-
 drivers/tty/tty_buffer.c |  6 ++++++
 include/linux/tty.h      |  4 ++++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 2cf2567..e0007d1 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -209,10 +209,16 @@ static int pty_signal(struct tty_struct *tty, int sig)
 static void pty_flush_buffer(struct tty_struct *tty)
 {
 	struct tty_struct *to = tty->link;
+	struct tty_ldisc *ld;
 
 	if (!to)
 		return;
-	/* tty_buffer_flush(to); FIXME */
+
+	ld = tty_ldisc_ref(to);
+	tty_buffer_flush(to, ld);
+	if (ld)
+		tty_ldisc_deref(ld);
+
 	if (to->packet) {
 		spin_lock_irq(&tty->ctrl_lock);
 		tty->ctrl_status |= TIOCPKT_FLUSHWRITE;
@@ -422,6 +428,8 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	tty->port = ports[1];
 	o_tty->port->itty = o_tty;
 
+	tty_buffer_set_lock_subclass(o_tty->port);
+
 	tty_driver_kref_get(driver);
 	tty->count++;
 	o_tty->count++;
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 3605103..7566164 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -557,3 +557,9 @@ int tty_buffer_set_limit(struct tty_port *port, int limit)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tty_buffer_set_limit);
+
+/* slave ptys can claim nested buffer lock when handling BRK and INTR */
+void tty_buffer_set_lock_subclass(struct tty_port *port)
+{
+	lockdep_set_subclass(&port->buf.lock, TTY_LOCK_SLAVE);
+}
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 4c1c453..c60efad 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -25,6 +25,9 @@
  *
  * legacy_mutex - Nested tty locks are necessary for releasing pty pairs.
  *		  The stable lock order is master pty first, then slave pty.
+ * tty_buffer lock - slave ptys can claim nested buffer lock when handling
+ *		     signal chars. The stable lock order is slave pty, then
+ *		     master.
  */
 
 enum {
@@ -460,6 +463,7 @@ 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);
+extern void tty_buffer_set_lock_subclass(struct tty_port *port);
 extern speed_t tty_termios_baud_rate(struct ktermios *termios);
 extern speed_t tty_termios_input_baud_rate(struct ktermios *termios);
 extern void tty_termios_encode_baud_rate(struct ktermios *termios,
-- 
2.2.2

[PATCH 3/3] n_tty: Fix signal handling flushes

[Peter Hurley]

BRKINT and ISIG requires input and output flush when a signal char
is received. However, the order of operations is significant since
parallel i/o may be ongoing.

Merge the signal handling for BRKINT with ISIG handling.

Process the signal first. This ensures any ongoing i/o is aborted;
without this, a waiting writer may continue writing after the flush
occurs and after the signal char has been echoed.

Write lock the termios_rwsem, which excludes parallel writers from
pushing new i/o until after the output buffers are flushed; claiming
the write lock is necessary anyway to exclude parallel readers while
the read buffer is flushed.

Subclass the termios_rwsem for ptys since the slave pty performing
the flush may appear to reorder the termios_rwsem->tty buffer lock
lock order; adding annotation clarifies that
  slave tty_buffer lock-> slave termios_rwsem -> master tty_buffer lock
is a valid lock order.

Flush the echo buffer. In this context, the echo buffer is 'output'.
Otherwise, the output will appear discontinuous because the output buffer
was cleared which contains older output than the echo buffer.

Open-code the read buffer flush since the input worker does not need
kicking (this is the input worker).

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 45 ++++++++++++++++++++++++++++++---------------
 drivers/tty/pty.c   |  1 +
 include/linux/tty.h |  3 +++
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 031a36b..ea26ea6 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1088,16 +1088,45 @@ static void eraser(unsigned char c, struct tty_struct *tty)
  *	Called when a signal is being sent due to terminal input.
  *	Called from the driver receive_buf path so serialized.
  *
+ *	Performs input and output flush if !NOFLSH. In this context, the echo
+ *	buffer is 'output'. The signal is processed first to alert any current
+ *	readers or writers to discontinue and exit their i/o loops.
+ *
  *	Locking: ctrl_lock
  */
 
 static void isig(int sig, struct tty_struct *tty)
 {
+	struct n_tty_data *ldata = tty->disc_data;
 	struct pid *tty_pgrp = tty_get_pgrp(tty);
 	if (tty_pgrp) {
 		kill_pgrp(tty_pgrp, sig, 1);
 		put_pid(tty_pgrp);
 	}
+
+	if (!L_NOFLSH(tty)) {
+		up_read(&tty->termios_rwsem);
+		down_write(&tty->termios_rwsem);
+
+		/* clear echo buffer */
+		mutex_lock(&ldata->output_lock);
+		ldata->echo_head = ldata->echo_tail = 0;
+		ldata->echo_mark = ldata->echo_commit = 0;
+		mutex_unlock(&ldata->output_lock);
+
+		/* clear output buffer */
+		tty_driver_flush_buffer(tty);
+
+		/* clear input buffer */
+		reset_buffer_flags(tty->disc_data);
+
+		/* notify pty master of flush */
+		if (tty->link)
+			n_tty_packet_mode_flush(tty);
+
+		up_write(&tty->termios_rwsem);
+		down_read(&tty->termios_rwsem);
+	}
 }
 
 /**
@@ -1121,13 +1150,6 @@ static void n_tty_receive_break(struct tty_struct *tty)
 		return;
 	if (I_BRKINT(tty)) {
 		isig(SIGINT, tty);
-		if (!L_NOFLSH(tty)) {
-			/* flushing needs exclusive termios_rwsem */
-			up_read(&tty->termios_rwsem);
-			n_tty_flush_buffer(tty);
-			tty_driver_flush_buffer(tty);
-			down_read(&tty->termios_rwsem);
-		}
 		return;
 	}
 	if (I_PARMRK(tty)) {
@@ -1197,13 +1219,7 @@ static void n_tty_receive_parity_error(struct tty_struct *tty, unsigned char c)
 static void
 n_tty_receive_signal_char(struct tty_struct *tty, int signal, unsigned char c)
 {
-	if (!L_NOFLSH(tty)) {
-		/* flushing needs exclusive termios_rwsem */
-		up_read(&tty->termios_rwsem);
-		n_tty_flush_buffer(tty);
-		tty_driver_flush_buffer(tty);
-		down_read(&tty->termios_rwsem);
-	}
+	isig(signal, tty);
 	if (I_IXON(tty))
 		start_tty(tty);
 	if (L_ECHO(tty)) {
@@ -1211,7 +1227,6 @@ n_tty_receive_signal_char(struct tty_struct *tty, int signal, unsigned char c)
 		commit_echoes(tty);
 	} else
 		process_echoes(tty);
-	isig(signal, tty);
 	return;
 }
 
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index e0007d1..ee06b77 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -392,6 +392,7 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 		goto err_put_module;
 
 	tty_set_lock_subclass(o_tty);
+	lockdep_set_subclass(&o_tty->termios_rwsem, TTY_LOCK_SLAVE);
 
 	if (legacy) {
 		/* We always use new tty termios data so we can do this
diff --git a/include/linux/tty.h b/include/linux/tty.h
index c60efad..358a337 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -25,6 +25,9 @@
  *
  * legacy_mutex - Nested tty locks are necessary for releasing pty pairs.
  *		  The stable lock order is master pty first, then slave pty.
+ * termios_rwsem - The stable lock order is tty_buffer lock->termios_rwsem.
+ *		   Subclassing this lock enables the slave pty to hold its
+ *		   termios_rwsem when claiming the master tty_buffer lock.
  * tty_buffer lock - slave ptys can claim nested buffer lock when handling
  *		     signal chars. The stable lock order is slave pty, then
  *		     master.
-- 
2.2.2