[PATCH -next 0/9] tty: Fix buffer work access-after-free

[PATCH -next 0/9] tty: Fix buffer work access-after-free

[Peter Hurley]

This patch series addresses the causes of flush_to_ldisc accessing
the tty after freeing.

The most common cause stems from the n_tty_close() path spuriously
scheduling buffer work, when the ldisc has already been halted.
This is fixed in 'tty: Don't reschedule buffer work while closing'

The other causes have a central theme: incorrect order-of-operations
when halting a line discipline. In general, to prevent buffer work
from being scheduled requires:
  1. Disallowing further ldisc references
  2. Waiting for all existing ldisc references to be released
  3. Cancelling existing buffer work
If the wait takes place _after_ cancellation, then new work
can be scheduled by existing holder(s) of ldisc reference(s). That's
bad.

Halting the line discipline is performed when,
 - hanging up the tty (tty_ldisc_hangup())
 - TIOCSETD ioctl (tty_set_ldisc())
 - finally closing the tty (pair) (tty_ldisc_release())

Concurrent halts are governed by the following requirements:
1. tty_ldisc_release is not concurrent with the other two and so
   does not need lock or state protection during the ldiscs halt.
2. Accesses through tty->ldisc must be protected by the ldisc_mutex.
   The wait operation checks the user count (ldisc references)
   in tty->ldisc->users.
3. tty_set_ldisc() reschedules buffer work that was pending when
   the ldiscs were halted. That must be an atomic operation in
   conjunction with re-enabling the ldisc -- which necessitates
   locking concurrent halts (tty_ldisc_release is exempt here)
4. The legacy mutex cannot be held while waiting for ldisc
   reference(s) release -or- for cancelling buffer work.
5. Because of #4, the legacy mutex must be dropped prior to or
   during the halt. Which means reacquiring after the halt. But
   to preserve lock order the ldisc_mutex must be dropped and
   reacquired after reacquiring the legacy mutex.
6. Because of #5, the ldisc state may have changed while the
   ldisc mutex was dropped.

Note: this series does not include the lock correction initially
reported on by Sebastian Andrzej Siewior <bigeasy@linutronix.de> here
https://lkml.org/lkml/2012/11/21/267 . I commented on the latest
version here https://lkml.org/lkml/2012/12/3/362

This series also does not include Jiri's debug patch here
https://lkml.org/lkml/2012/11/2/278 for identifying which itty cleanup
path is used. I think it may be helpful to include this in -next.

Peter Hurley (9):
  tty: WARN if buffer work racing with tty free
  tty: Add diagnostic for halted line discipline
  tty: Don't reschedule buffer work while closing
  tty: Refactor wait for ldisc refs out of tty_ldisc_hangup()
  tty: Remove unnecessary re-test of ldisc ref count
  tty: Fix ldisc halt sequence on hangup
  tty: Strengthen no-subsequent-use guarantee of tty_ldisc_halt()
  tty: Remove unnecessary buffer work flush
  tty: Halt both ldiscs concurrently

 drivers/tty/n_tty.c      |   8 ++-
 drivers/tty/tty_buffer.c |   3 +
 drivers/tty/tty_io.c     |   2 +
 drivers/tty/tty_ldisc.c  | 171 +++++++++++++++++++++++++++++------------------
 include/linux/tty.h      |   1 +
 5 files changed, 119 insertions(+), 66 deletions(-)

-- 
1.8.0

[PATCH -next 1/9] tty: WARN if buffer work racing with tty free

[Peter Hurley]

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 78c3000..3d2b6d7 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1511,6 +1511,8 @@ static void queue_release_one_tty(struct kref *kref)
 {
 	struct tty_struct *tty = container_of(kref, struct tty_struct, kref);
 
+	WARN_ON(work_pending(&tty->port->buf.work));
+
 	/* The hangup queue is now free so we can reuse it rather than
 	   waste a chunk of memory for each port */
 	INIT_WORK(&tty->hangup_work, release_one_tty);
-- 
1.8.0

[PATCH -next 2/9] tty: Add diagnostic for halted line discipline

[Peter Hurley]

Flip buffer work must not be scheduled after the line discipline
has been halted; issue warning.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c      |  6 ++++++
 drivers/tty/tty_buffer.c |  3 +++
 drivers/tty/tty_ldisc.c  | 45 +++++++++++++++++++++++++--------------------
 include/linux/tty.h      |  1 +
 4 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 19083ef..3f704a9 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -152,6 +152,12 @@ static void n_tty_set_room(struct tty_struct *tty)
 	if (left && !old_left) {
 		WARN_RATELIMIT(tty->port->itty == NULL,
 				"scheduling with invalid itty\n");
+		/* see if ldisc has been killed - if so, this means that
+		 * even though the ldisc has been halted and ->buf.work
+		 * cancelled, ->buf.work is about to be rescheduled
+		 */
+		WARN_RATELIMIT(test_bit(TTY_LDISC_HALTED, &tty->flags),
+			       "scheduling buffer work for halted ldisc\n");
 		schedule_work(&tty->port->buf.work);
 	}
 }
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index cc9cbcf..d8d6f77 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -520,6 +520,9 @@ void tty_flip_buffer_push(struct tty_struct *tty)
 	struct tty_bufhead *buf = &tty->port->buf;
 	unsigned long flags;
 
+	WARN_RATELIMIT(test_bit(TTY_LDISC_HALTED, &tty->flags),
+		       "scheduling buffer work for halted ldisc\n");
+
 	spin_lock_irqsave(&buf->lock, flags);
 	if (buf->tail != NULL)
 		buf->tail->commit = buf->tail->used;
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index c578229..f986bb0 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -373,6 +373,7 @@ static inline void tty_ldisc_put(struct tty_ldisc *ld)
 
 void tty_ldisc_enable(struct tty_struct *tty)
 {
+	clear_bit(TTY_LDISC_HALTED, &tty->flags);
 	set_bit(TTY_LDISC, &tty->flags);
 	clear_bit(TTY_LDISC_CHANGING, &tty->flags);
 	wake_up(&tty_ldisc_wait);
@@ -496,26 +497,6 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
 }
 
 /**
- *	tty_ldisc_halt		-	shut down the line discipline
- *	@tty: tty device
- *
- *	Shut down the line discipline and work queue for this tty device.
- *	The TTY_LDISC flag being cleared ensures no further references can
- *	be obtained while the delayed work queue halt ensures that no more
- *	data is fed to the ldisc.
- *
- *	You need to do a 'flush_scheduled_work()' (outside the ldisc_mutex)
- *	in order to make sure any currently executing ldisc work is also
- *	flushed.
- */
-
-static int tty_ldisc_halt(struct tty_struct *tty)
-{
-	clear_bit(TTY_LDISC, &tty->flags);
-	return cancel_work_sync(&tty->port->buf.work);
-}
-
-/**
  *	tty_ldisc_flush_works	-	flush all works of a tty
  *	@tty: tty device to flush works for
  *
@@ -545,6 +526,29 @@ static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
 }
 
 /**
+ *	tty_ldisc_halt		-	shut down the line discipline
+ *	@tty: tty device
+ *
+ *	Shut down the line discipline and work queue for this tty device.
+ *	The TTY_LDISC flag being cleared ensures no further references can
+ *	be obtained while the delayed work queue halt ensures that no more
+ *	data is fed to the ldisc.
+ *
+ *	You need to do a 'flush_scheduled_work()' (outside the ldisc_mutex)
+ *	in order to make sure any currently executing ldisc work is also
+ *	flushed.
+ */
+
+static int tty_ldisc_halt(struct tty_struct *tty)
+{
+	int scheduled;
+	clear_bit(TTY_LDISC, &tty->flags);
+	scheduled = cancel_work_sync(&tty->port->buf.work);
+	set_bit(TTY_LDISC_HALTED, &tty->flags);
+	return scheduled;
+}
+
+/**
  *	tty_set_ldisc		-	set line discipline
  *	@tty: the terminal to set
  *	@ldisc: the line discipline
@@ -818,6 +822,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	clear_bit(TTY_LDISC, &tty->flags);
 	tty_unlock(tty);
 	cancel_work_sync(&tty->port->buf.work);
+	set_bit(TTY_LDISC_HALTED, &tty->flags);
 	mutex_unlock(&tty->ldisc_mutex);
 retry:
 	tty_lock(tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 8db1b56..e6b968d 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -314,6 +314,7 @@ struct tty_file_private {
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
 #define TTY_HUPPED 		18	/* Post driver->hangup() */
 #define TTY_HUPPING 		21	/* ->hangup() in progress */
+#define TTY_LDISC_HALTED	22	/* Line discipline is halted */
 
 #define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
 
-- 
1.8.0

[PATCH -next 3/9] tty: Don't reschedule buffer work while closing

[Peter Hurley]

Prevent buffer work scheduling when called from n_tty_close(). Since
the ldisc has been halted and the tty soon-to-be-destructed, pending
work would be accessing an invalid tty and ldisc state. Fixes this:

[   38.051111] ------------[ cut here ]------------
[   38.052113] WARNING: at /home/peter/src/kernels/next/drivers/tty/n_tty.c:160 n_tty_set_room.part.6+0x8b/0xa0()
[   38.053916] Hardware name: Bochs
[   38.054819] Modules linked in: netconsole configfs bnep rfcomm bluetooth parport_pc ppdev snd_hda_intel snd_hda_codec
snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq psmouse snd_timer serio_raw mac_hid snd_seq_device
snd microcode lp parport virtio_balloon soundcore i2c_piix4 snd_page_alloc floppy 8139too 8139cp
[   38.059704] Pid: 1564, comm: pty_kill Tainted: G        W    3.7.0-next-20121130+ttydebug-xeon #20121130+ttydebug
[   38.061578] Call Trace:
[   38.062491]  [<ffffffff81058b4f>] warn_slowpath_common+0x7f/0xc0
[   38.063448]  [<ffffffff81058baa>] warn_slowpath_null+0x1a/0x20
[   38.064439]  [<ffffffff8142dc2b>] n_tty_set_room.part.6+0x8b/0xa0
[   38.065381]  [<ffffffff8142dc82>] n_tty_set_room+0x42/0x80
[   38.066323]  [<ffffffff8142e6f2>] reset_buffer_flags+0x102/0x160
[   38.077508]  [<ffffffff8142e76d>] n_tty_flush_buffer+0x1d/0x90
[   38.078782]  [<ffffffff81046569>] ? default_spin_lock_flags+0x9/0x10
[   38.079734]  [<ffffffff8142e804>] n_tty_close+0x24/0x60
[   38.080730]  [<ffffffff81431b61>] tty_ldisc_close.isra.2+0x41/0x60
[   38.081680]  [<ffffffff81431bbb>] tty_ldisc_kill+0x3b/0x80
[   38.082618]  [<ffffffff81432a07>] tty_ldisc_release+0x77/0xe0
[   38.083549]  [<ffffffff8142b781>] tty_release+0x451/0x4d0
[   38.084525]  [<ffffffff811950be>] __fput+0xae/0x230
[   38.085472]  [<ffffffff8119524e>] ____fput+0xe/0x10
[   38.086401]  [<ffffffff8107aa88>] task_work_run+0xc8/0xf0
[   38.087334]  [<ffffffff8105ea56>] do_exit+0x196/0x4b0
[   38.088304]  [<ffffffff8106c77b>] ? __dequeue_signal+0x6b/0xb0
[   38.089240]  [<ffffffff8105ef34>] do_group_exit+0x44/0xa0
[   38.090182]  [<ffffffff8106f43d>] get_signal_to_deliver+0x20d/0x4e0
[   38.091125]  [<ffffffff81016979>] do_signal+0x29/0x130
[   38.092096]  [<ffffffff81431a9e>] ? tty_ldisc_deref+0xe/0x10
[   38.093030]  [<ffffffff8142a317>] ? tty_write+0xb7/0xf0
[   38.093976]  [<ffffffff81193f53>] ? vfs_write+0xb3/0x180
[   38.094904]  [<ffffffff81016b20>] do_notify_resume+0x80/0xc0
[   38.095830]  [<ffffffff81700492>] int_signal+0x12/0x17
[   38.096788] ---[ end trace 5f6f7a9651cd999b ]---

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 3f704a9..574d099 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -149,7 +149,7 @@ static void n_tty_set_room(struct tty_struct *tty)
 	tty->receive_room = left;
 
 	/* Did this open up the receive buffer? We may need to flip */
-	if (left && !old_left) {
+	if (left && !old_left && !test_bit(TTY_CLOSING, &tty->flags)) {
 		WARN_RATELIMIT(tty->port->itty == NULL,
 				"scheduling with invalid itty\n");
 		/* see if ldisc has been killed - if so, this means that
-- 
1.8.0

[PATCH -next 4/9] tty: Refactor wait for ldisc refs out of tty_ldisc_hangup()

[Peter Hurley]

Refactor tty_ldisc_hangup() to extract standalone function,
tty_ldisc_hangup_wait_idle(), to wait for ldisc references
to be released.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 54 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index f986bb0..6c1d8aa 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -549,6 +549,41 @@ static int tty_ldisc_halt(struct tty_struct *tty)
 }
 
 /**
+ *	tty_ldisc_hangup_wait_idle - wait for the ldisc to become idle
+ *	@tty: tty to wait for
+ *
+ *	Wait for the line discipline to become idle. The discipline must
+ *	have been halted for this to guarantee it remains idle.
+ *
+ *	Caller must hold legacy and ->ldisc_mutex.
+ */
+static bool tty_ldisc_hangup_wait_idle(struct tty_struct *tty)
+{
+	while (tty->ldisc) {	/* Not yet closed */
+		if (atomic_read(&tty->ldisc->users) != 1) {
+			char cur_n[TASK_COMM_LEN], tty_n[64];
+			long timeout = 3 * HZ;
+			tty_unlock(tty);
+
+			while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
+				timeout = MAX_SCHEDULE_TIMEOUT;
+				printk_ratelimited(KERN_WARNING
+					"%s: waiting (%s) for %s took too long, but we keep waiting...\n",
+					__func__, get_task_comm(cur_n, current),
+					tty_name(tty, tty_n));
+			}
+			/* must reacquire both locks and preserve lock order */
+			mutex_unlock(&tty->ldisc_mutex);
+			tty_lock(tty);
+			mutex_lock(&tty->ldisc_mutex);
+			continue;
+		}
+		break;
+	}
+	return !!(tty->ldisc);
+}
+
+/**
  *	tty_set_ldisc		-	set line discipline
  *	@tty: the terminal to set
  *	@ldisc: the line discipline
@@ -824,7 +859,6 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	cancel_work_sync(&tty->port->buf.work);
 	set_bit(TTY_LDISC_HALTED, &tty->flags);
 	mutex_unlock(&tty->ldisc_mutex);
-retry:
 	tty_lock(tty);
 	mutex_lock(&tty->ldisc_mutex);
 
@@ -832,23 +866,7 @@ retry:
 	   reopen it. We could defer this to the next open but
 	   it means auditing a lot of other paths so this is
 	   a FIXME */
-	if (tty->ldisc) {	/* Not yet closed */
-		if (atomic_read(&tty->ldisc->users) != 1) {
-			char cur_n[TASK_COMM_LEN], tty_n[64];
-			long timeout = 3 * HZ;
-			tty_unlock(tty);
-
-			while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
-				timeout = MAX_SCHEDULE_TIMEOUT;
-				printk_ratelimited(KERN_WARNING
-					"%s: waiting (%s) for %s took too long, but we keep waiting...\n",
-					__func__, get_task_comm(cur_n, current),
-					tty_name(tty, tty_n));
-			}
-			mutex_unlock(&tty->ldisc_mutex);
-			goto retry;
-		}
-
+	if (tty_ldisc_hangup_wait_idle(tty)) {
 		if (reset == 0) {
 
 			if (!tty_ldisc_reinit(tty, tty->termios.c_line))
-- 
1.8.0

[PATCH -next 5/9] tty: Remove unnecessary re-test of ldisc ref count

[Peter Hurley]

Since the tty->ldisc is prevented from being changed by tty_set_ldisc()
when a tty is being hung up, re-testing the ldisc user count is
unnecessary -- ie, it cannot be a different ldisc and the user count
cannot have increased (assuming the caller meets the precondition that
TTY_LDISC flag is cleared)

Removal of the 'early-out' locking optimization is necessary for
the subsequent patch 'tty: Fix ldisc halt sequence on hangup'.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 6c1d8aa..0d18d1e 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -556,29 +556,29 @@ static int tty_ldisc_halt(struct tty_struct *tty)
  *	have been halted for this to guarantee it remains idle.
  *
  *	Caller must hold legacy and ->ldisc_mutex.
+ *
+ *	NB: tty_set_ldisc() is prevented from changing the ldisc concurrently
+ *	with this function when it checks the TTY_HUPPING state.
  */
 static bool tty_ldisc_hangup_wait_idle(struct tty_struct *tty)
 {
-	while (tty->ldisc) {	/* Not yet closed */
-		if (atomic_read(&tty->ldisc->users) != 1) {
-			char cur_n[TASK_COMM_LEN], tty_n[64];
-			long timeout = 3 * HZ;
-			tty_unlock(tty);
-
-			while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
-				timeout = MAX_SCHEDULE_TIMEOUT;
-				printk_ratelimited(KERN_WARNING
-					"%s: waiting (%s) for %s took too long, but we keep waiting...\n",
-					__func__, get_task_comm(cur_n, current),
-					tty_name(tty, tty_n));
-			}
-			/* must reacquire both locks and preserve lock order */
-			mutex_unlock(&tty->ldisc_mutex);
-			tty_lock(tty);
-			mutex_lock(&tty->ldisc_mutex);
-			continue;
+	char cur_n[TASK_COMM_LEN], tty_n[64];
+	long timeout = 3 * HZ;
+
+	if (tty->ldisc) {	/* Not yet closed */
+		tty_unlock(tty);
+
+		while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
+			timeout = MAX_SCHEDULE_TIMEOUT;
+			printk_ratelimited(KERN_WARNING
+				"%s: waiting (%s) for %s took too long, but we keep waiting...\n",
+				__func__, get_task_comm(cur_n, current),
+				tty_name(tty, tty_n));
 		}
-		break;
+		/* must reacquire both locks and preserve lock order */
+		mutex_unlock(&tty->ldisc_mutex);
+		tty_lock(tty);
+		mutex_lock(&tty->ldisc_mutex);
 	}
 	return !!(tty->ldisc);
 }
-- 
1.8.0

[PATCH -next 6/9] tty: Fix ldisc halt sequence on hangup

[Peter Hurley]

Flip buffer work cannot be cancelled until all outstanding ldisc
references have been released. Convert the ldisc ref wait into
a full ldisc halt with buffer work cancellation.

Note that the legacy mutex is not held while cancelling.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 0d18d1e..c3dec37 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -549,22 +549,31 @@ static int tty_ldisc_halt(struct tty_struct *tty)
 }
 
 /**
- *	tty_ldisc_hangup_wait_idle - wait for the ldisc to become idle
- *	@tty: tty to wait for
- *
- *	Wait for the line discipline to become idle. The discipline must
- *	have been halted for this to guarantee it remains idle.
+ *	tty_ldisc_hangup_halt - halt the line discipline for hangup
+ *	@tty: tty being hung up
  *
+ *	Shut down the line discipline and work queue for the tty device
+ *	being hungup. Clear the TTY_LDISC flag to ensure no further
+ *	references can be obtained, wait for remaining references to be
+ *	released, and cancel pending buffer work to ensure no more
+ *	data is fed to the ldisc.
  *	Caller must hold legacy and ->ldisc_mutex.
  *
  *	NB: tty_set_ldisc() is prevented from changing the ldisc concurrently
  *	with this function when it checks the TTY_HUPPING state.
+ *
+ *	NB: if tty->ldisc is NULL then buffer work does not need to be
+ *	cancelled because it must already have done as a precondition
+ *	of setting tty->ldisc to NULL
+ *
  */
-static bool tty_ldisc_hangup_wait_idle(struct tty_struct *tty)
+static bool tty_ldisc_hangup_halt(struct tty_struct *tty)
 {
 	char cur_n[TASK_COMM_LEN], tty_n[64];
 	long timeout = 3 * HZ;
 
+	clear_bit(TTY_LDISC, &tty->flags);
+
 	if (tty->ldisc) {	/* Not yet closed */
 		tty_unlock(tty);
 
@@ -575,6 +584,10 @@ static bool tty_ldisc_hangup_wait_idle(struct tty_struct *tty)
 				__func__, get_task_comm(cur_n, current),
 				tty_name(tty, tty_n));
 		}
+
+		cancel_work_sync(&tty->port->buf.work);
+		set_bit(TTY_LDISC_HALTED, &tty->flags);
+
 		/* must reacquire both locks and preserve lock order */
 		mutex_unlock(&tty->ldisc_mutex);
 		tty_lock(tty);
@@ -849,24 +862,11 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	 */
 	mutex_lock(&tty->ldisc_mutex);
 
-	/*
-	 * this is like tty_ldisc_halt, but we need to give up
-	 * the BTM before calling cancel_work_sync, which may
-	 * need to wait for another function taking the BTM
-	 */
-	clear_bit(TTY_LDISC, &tty->flags);
-	tty_unlock(tty);
-	cancel_work_sync(&tty->port->buf.work);
-	set_bit(TTY_LDISC_HALTED, &tty->flags);
-	mutex_unlock(&tty->ldisc_mutex);
-	tty_lock(tty);
-	mutex_lock(&tty->ldisc_mutex);
-
 	/* At this point we have a closed ldisc and we want to
 	   reopen it. We could defer this to the next open but
 	   it means auditing a lot of other paths so this is
 	   a FIXME */
-	if (tty_ldisc_hangup_wait_idle(tty)) {
+	if (tty_ldisc_hangup_halt(tty)) {
 		if (reset == 0) {
 
 			if (!tty_ldisc_reinit(tty, tty->termios.c_line))
-- 
1.8.0

[PATCH -next 7/9] tty: Strengthen no-subsequent-use guarantee of tty_ldisc_halt()

[Peter Hurley]

In preparation for destructing and freeing the tty, the line discipline
must first be brought to an inactive state before it can be destructed.
This line discipline shutdown must:
 - disallow new users of the ldisc
 - wait for existing ldisc users to finish
 - only then, cancel/flush their pending/running work

Factor tty_ldisc_wait_idle() from tty_set_ldisc() and tty_ldisc_kill()
to ensure this shutdown order.

Failure to provide this guarantee can result in scheduled work
running after the tty has already been freed, as indicated in the
following log message:

[   88.331234] WARNING: at /home/peter/src/kernels/next/drivers/tty/tty_buffer.c:435 flush_to_ldisc+0x194/0x1d0()
[   88.334505] Hardware name: Bochs
[   88.335618] tty is bad=-1
[   88.335703] Modules linked in: netconsole configfs bnep rfcomm bluetooth ......
[   88.345272] Pid: 39, comm: kworker/1:1 Tainted: G        W    3.7.0-next-20121129+ttydebug-xeon #20121129+ttydebug
[   88.347736] Call Trace:
[   88.349024]  [<ffffffff81058aff>] warn_slowpath_common+0x7f/0xc0
[   88.350383]  [<ffffffff81058bf6>] warn_slowpath_fmt+0x46/0x50
[   88.351745]  [<ffffffff81432bd4>] flush_to_ldisc+0x194/0x1d0
[   88.353047]  [<ffffffff816f7fe1>] ? _raw_spin_unlock_irq+0x21/0x50
[   88.354190]  [<ffffffff8108a809>] ? finish_task_switch+0x49/0xe0
[   88.355436]  [<ffffffff81077ad1>] process_one_work+0x121/0x490
[   88.357674]  [<ffffffff81432a40>] ? __tty_buffer_flush+0x90/0x90
[   88.358954]  [<ffffffff81078c84>] worker_thread+0x164/0x3e0
[   88.360247]  [<ffffffff81078b20>] ? manage_workers+0x120/0x120
[   88.361282]  [<ffffffff8107e230>] kthread+0xc0/0xd0
[   88.362284]  [<ffffffff816f0000>] ? cmos_do_probe+0x2eb/0x3bf
[   88.363391]  [<ffffffff8107e170>] ? flush_kthread_worker+0xb0/0xb0
[   88.364797]  [<ffffffff816fff6c>] ret_from_fork+0x7c/0xb0
[   88.366087]  [<ffffffff8107e170>] ? flush_kthread_worker+0xb0/0xb0
[   88.367266] ---[ end trace 453a7c9f38fbfec0 ]---

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index c3dec37..9f4c7b0 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -528,24 +528,38 @@ static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
 /**
  *	tty_ldisc_halt		-	shut down the line discipline
  *	@tty: tty device
+ *	@pending: returns true if work was scheduled when cancelled
+ *		  (can be set to NULL)
+ *	@timeout: # of jiffies to wait for ldisc refs to be released
  *
  *	Shut down the line discipline and work queue for this tty device.
  *	The TTY_LDISC flag being cleared ensures no further references can
  *	be obtained while the delayed work queue halt ensures that no more
  *	data is fed to the ldisc.
  *
+ *	Furthermore, guarantee that existing ldisc references have been
+ *	released, which in turn, guarantees that no future buffer work
+ *	can be rescheduled.
+ *
  *	You need to do a 'flush_scheduled_work()' (outside the ldisc_mutex)
  *	in order to make sure any currently executing ldisc work is also
  *	flushed.
  */
 
-static int tty_ldisc_halt(struct tty_struct *tty)
+static int tty_ldisc_halt(struct tty_struct *tty, int *pending, long timeout)
 {
-	int scheduled;
+	int scheduled, retval;
+
 	clear_bit(TTY_LDISC, &tty->flags);
+	retval = tty_ldisc_wait_idle(tty, timeout);
+	if (retval)
+		return retval;
+
 	scheduled = cancel_work_sync(&tty->port->buf.work);
 	set_bit(TTY_LDISC_HALTED, &tty->flags);
-	return scheduled;
+	if (pending)
+		*pending = scheduled;
+	return 0;
 }
 
 /**
@@ -687,9 +701,9 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	 *	parallel to the change and re-referencing the tty.
 	 */
 
-	work = tty_ldisc_halt(tty);
+	retval = tty_ldisc_halt(tty, &work, 5 * HZ);
 	if (o_tty)
-		o_work = tty_ldisc_halt(o_tty);
+		tty_ldisc_halt(o_tty, &o_work, 0);
 
 	/*
 	 * Wait for ->hangup_work and ->buf.work handlers to terminate.
@@ -700,8 +714,6 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	tty_ldisc_flush_works(tty);
 
-	retval = tty_ldisc_wait_idle(tty, 5 * HZ);
-
 	tty_lock(tty);
 	mutex_lock(&tty->ldisc_mutex);
 
@@ -920,11 +932,6 @@ int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty)
 
 static void tty_ldisc_kill(struct tty_struct *tty)
 {
-	/* There cannot be users from userspace now. But there still might be
-	 * drivers holding a reference via tty_ldisc_ref. Do not steal them the
-	 * ldisc until they are done. */
-	tty_ldisc_wait_idle(tty, MAX_SCHEDULE_TIMEOUT);
-
 	mutex_lock(&tty->ldisc_mutex);
 	/*
 	 * Now kill off the ldisc
@@ -958,10 +965,10 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
 	 */
 
 	tty_lock_pair(tty, o_tty);
-	tty_ldisc_halt(tty);
+	tty_ldisc_halt(tty, NULL, MAX_SCHEDULE_TIMEOUT);
 	tty_ldisc_flush_works(tty);
 	if (o_tty) {
-		tty_ldisc_halt(o_tty);
+		tty_ldisc_halt(o_tty, NULL, MAX_SCHEDULE_TIMEOUT);
 		tty_ldisc_flush_works(o_tty);
 	}
 
-- 
1.8.0

[PATCH -next 8/9] tty: Remove unnecessary buffer work flush

[Peter Hurley]

In order to safely call tty_ldisc_flush_works(), the ldisc must
already have been halted, so any pending buffer work has already
been cancelled or flushed as part of the halt.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 9f4c7b0..19e088a 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -506,7 +506,6 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
 {
 	flush_work(&tty->hangup_work);
 	flush_work(&tty->SAK_work);
-	flush_work(&tty->port->buf.work);
 }
 
 /**
-- 
1.8.0

[PATCH -next 9/9] tty: Halt both ldiscs concurrently

[Peter Hurley]

The pty driver does not obtain an ldisc reference to the linked
tty when writing. When the ldiscs are sequentially halted, it
is possible for one ldisc to be halted, and before the second
ldisc can be halted, a concurrent write schedules buffer work on
the first ldisc. This can lead to an access-after-free error when
the scheduled buffer work starts on the closed ldisc.

Prevent subsequent use after halt by performing each stage
of the halt alternately the tty pair.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 19e088a..a6d3078 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -527,37 +527,53 @@ static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
 /**
  *	tty_ldisc_halt		-	shut down the line discipline
  *	@tty: tty device
+ *	@o_tty: paired pty device (can be NULL)
  *	@pending: returns true if work was scheduled when cancelled
  *		  (can be set to NULL)
+ *	@o_pending: returns true if work was scheduled when cancelled
+ *		    (can be set to NULL)
  *	@timeout: # of jiffies to wait for ldisc refs to be released
  *
- *	Shut down the line discipline and work queue for this tty device.
- *	The TTY_LDISC flag being cleared ensures no further references can
- *	be obtained while the delayed work queue halt ensures that no more
- *	data is fed to the ldisc.
+ *	Shut down the line discipline and work queue for this tty device and
+ *	its paired pty (if exists). Clearing the TTY_LDISC flag ensures
+ *	no further references can be obtained while the work queue halt
+ *	ensures that no more data is fed to the ldisc.
  *
  *	Furthermore, guarantee that existing ldisc references have been
  *	released, which in turn, guarantees that no future buffer work
  *	can be rescheduled.
  *
- *	You need to do a 'flush_scheduled_work()' (outside the ldisc_mutex)
+ *	You need to do a 'tty_ldisc_flush_works()' (outside the ldisc_mutex)
  *	in order to make sure any currently executing ldisc work is also
  *	flushed.
  */
 
-static int tty_ldisc_halt(struct tty_struct *tty, int *pending, long timeout)
+static int tty_ldisc_halt(struct tty_struct *tty, struct tty_struct *o_tty,
+			  int *pending, int *o_pending, long timeout)
 {
-	int scheduled, retval;
+	int scheduled, o_scheduled, retval;
 
 	clear_bit(TTY_LDISC, &tty->flags);
+	if (o_tty)
+		clear_bit(TTY_LDISC, &o_tty->flags);
+
 	retval = tty_ldisc_wait_idle(tty, timeout);
+	if (!retval && o_tty)
+		retval = tty_ldisc_wait_idle(o_tty, timeout);
 	if (retval)
 		return retval;
 
 	scheduled = cancel_work_sync(&tty->port->buf.work);
 	set_bit(TTY_LDISC_HALTED, &tty->flags);
+	if (o_tty) {
+		o_scheduled = cancel_work_sync(&o_tty->port->buf.work);
+		set_bit(TTY_LDISC_HALTED, &o_tty->flags);
+	}
+
 	if (pending)
 		*pending = scheduled;
+	if (o_tty && o_pending)
+		*o_pending = o_scheduled;
 	return 0;
 }
 
@@ -700,9 +716,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	 *	parallel to the change and re-referencing the tty.
 	 */
 
-	retval = tty_ldisc_halt(tty, &work, 5 * HZ);
-	if (o_tty)
-		tty_ldisc_halt(o_tty, &o_work, 0);
+	retval = tty_ldisc_halt(tty, o_tty, &work, &o_work, 5 * HZ);
 
 	/*
 	 * Wait for ->hangup_work and ->buf.work handlers to terminate.
@@ -964,12 +978,10 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
 	 */
 
 	tty_lock_pair(tty, o_tty);
-	tty_ldisc_halt(tty, NULL, MAX_SCHEDULE_TIMEOUT);
+	tty_ldisc_halt(tty, o_tty, NULL, NULL, MAX_SCHEDULE_TIMEOUT);
 	tty_ldisc_flush_works(tty);
-	if (o_tty) {
-		tty_ldisc_halt(o_tty, NULL, MAX_SCHEDULE_TIMEOUT);
+	if (o_tty)
 		tty_ldisc_flush_works(o_tty);
-	}
 
 	/* This will need doing differently if we need to lock */
 	tty_ldisc_kill(tty);
-- 
1.8.0

Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free

[Ilya Zykov]

On 04.12.2012 11:07, Peter Hurley wrote:
> This patch series addresses the causes of flush_to_ldisc accessing
> the tty after freeing.
> 

I think, it is have sense only if you can take effect,
with this patch or something like. I can't. :)

Signed-off-by: Ilya Zykov <ilya@ilyx.ru>
---
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 2ea176b..f24751d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -170,6 +170,10 @@ struct tty_struct *alloc_tty_struct(void)
 	return kzalloc(sizeof(struct tty_struct), GFP_KERNEL);
 }

+static void flush_to_ldisc2(struct work_struct *work)
+{
+	printk(KERN_WARNING "Possible intrusion detected.\n");
+}
 /**
  *	free_tty_struct		-	free a disused tty
  *	@tty: tty struct to free
@@ -188,6 +192,8 @@ void free_tty_struct(struct tty_struct *tty)
 	kfree(tty->write_buf);
 	tty_buffer_free_all(tty);
 	tty->magic = 0xDEADDEAD;
+	PREPARE_WORK(&tty->buf.work,flush_to_ldisc2);
+	//memset(tty, 0, sizeof(struct tty_struct));
 	kfree(tty);
 }

Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free

[Alan Cox]

On Tue,  4 Dec 2012 02:07:36 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> This patch series addresses the causes of flush_to_ldisc accessing
> the tty after freeing.

Looks good to me. Would be nice to keep a copy of the test that shows
it up in the comments of the patches somewhere.

Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free

[Peter Hurley]

On Tue, 2012-12-04 at 08:54 +0000, Alan Cox wrote:
> On Tue,  4 Dec 2012 02:07:36 -0500
> Peter Hurley <peter@hurleysoftware.com> wrote:
> 
> > This patch series addresses the causes of flush_to_ldisc accessing
> > the tty after freeing.
> 
> Looks good to me. Would be nice to keep a copy of the test that shows
> it up in the comments of the patches somewhere.

I assume you mean somewhere other than ml archives:
https://lkml.org/lkml/2012/12/1/57

Setting up a mini-testsuite is good idea. As I noted in that email
though, that test jig is a derivative work that I'd want a ok from Ilya
Zykov to put somewhere more permanent.

Regards,
Peter Hurley

Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free

[Alan Cox]

> I assume you mean somewhere other than ml archives:
> https://lkml.org/lkml/2012/12/1/57

Putting in the commit message would be idea given how short it is.

> 
> Setting up a mini-testsuite is good idea. As I noted in that email
> though, that test jig is a derivative work that I'd want a ok from Ilya
> Zykov to put somewhere more permanent.

Sure

Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free

[Jiri Slaby]

On 12/04/2012 08:07 AM, Peter Hurley wrote:
> This patch series addresses the causes of flush_to_ldisc accessing
> the tty after freeing.

Hi, thanks for doing the work. The series looks good to me.

> This series also does not include Jiri's debug patch here
> https://lkml.org/lkml/2012/11/2/278 for identifying which itty cleanup
> path is used. I think it may be helpful to include this in -next.

Feel free to add your SOB and repost. Even though I have some patches in
my queue, I'm not going to send anything for the next merge window as I
will be on longish vacation and don't want to spend it by fixing tty
bugs I could introduce 8).

thanks,
-- 
js
suse labs

Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free

[Peter Hurley]

On Tue, 2012-12-04 at 02:07 -0500, Peter Hurley wrote:
> This patch series addresses the causes of flush_to_ldisc accessing
> the tty after freeing.

Well, there's going to be a v2 of this series.

Sasha found that PATCH 3/9 is an insufficient fix. I've already worked
out the correct solution but I've been plagued by the kswapd/flush
problem so that's making this more challenging then it ought to be.

Plus I tripped across a GP fault in the SLUB allocator which turns out
to be a free list corruption (probably because of the low memory
condition brought on by the kswapd problem. Good times...)

There is also going to be a v3 because I just came up with an idea to
clean up the whole ldisc ref situation, but I'm going to take some time
testing it out first.

Anyway, just wanted to hold this off for another respin.

Regards,
Peter Hurley

Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free

[Ilya Zykov]

On 04.12.2012 11:07, Peter Hurley wrote:
> 
> The most common cause stems from the n_tty_close() path spuriously
> scheduling buffer work, when the ldisc has already been halted.
> This is fixed in 'tty: Don't reschedule buffer work while closing'

Thank you, 
very useful.
Fix this:

WARNING: at drivers/tty/tty_buffer.c:429 flush_to_ldisc+0x52/0x192()
Hardware name: P5K Premium
tty is NULL
...
Pid: 1394, comm: kworker/0:2 Tainted: P        W  O 3.7.0-rc8-next-20121210-debtty.1+ #4
Call Trace:
 [<ffffffff8102d2f8>] warn_slowpath_common+0x80/0x98
 [<ffffffff8102d3a4>] warn_slowpath_fmt+0x41/0x43
 [<ffffffff8119c53a>] flush_to_ldisc+0x52/0x192
 [<ffffffff812594bd>] ? __schedule+0x5dd/0x60c
 [<ffffffff8103f146>] process_one_work+0x1c1/0x279
 [<ffffffff8119c4e8>] ? tty_buffer_free_all+0x4d/0x4d
 [<ffffffff8104104b>] worker_thread+0x154/0x24e
 [<ffffffff81040ef7>] ? manage_workers+0x26c/0x26c
 [<ffffffff810446ef>] kthread+0xb0/0xb8
 [<ffffffff8104463f>] ? kthread_parkme+0x1f/0x1f
 [<ffffffff8125f7ec>] ret_from_fork+0x7c/0xb0
 [<ffffffff8104463f>] ? kthread_parkme+0x1f/0x1f