[PATCH 1/5] TTY: ldisc, allow waiting for ldisc arbitrarily long

[PATCH 1/5] TTY: ldisc, allow waiting for ldisc arbitrarily long

[Jiri Slaby]

To fix a nasty bug in ldisc hup vs. reinit we need to wait infinitely
long for ldisc to be gone. So here we add a parameter to
tty_ldisc_wait_idle to allow that.

This is only a preparation for the real fix which is done in the
following patches.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Dave Young <hidave.darkstar@gmail.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Dmitriy Matrosov <sgf.dma@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: stable <stable@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
1-3 in this series are about the bug I mentioned at the summit.
4-5 are not critical at all.

 drivers/tty/tty_ldisc.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 512c49f..534d176 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -547,15 +547,16 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
 /**
  *	tty_ldisc_wait_idle	-	wait for the ldisc to become idle
  *	@tty: tty to wait for
+ *	@timeout: for how long to wait at most
  *
  *	Wait for the line discipline to become idle. The discipline must
  *	have been halted for this to guarantee it remains idle.
  */
-static int tty_ldisc_wait_idle(struct tty_struct *tty)
+static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
 {
-	int ret;
+	long ret;
 	ret = wait_event_timeout(tty_ldisc_idle,
-			atomic_read(&tty->ldisc->users) == 1, 5 * HZ);
+			atomic_read(&tty->ldisc->users) == 1, timeout);
 	if (ret < 0)
 		return ret;
 	return ret > 0 ? 0 : -EBUSY;
@@ -665,7 +666,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	tty_ldisc_flush_works(tty);
 
-	retval = tty_ldisc_wait_idle(tty);
+	retval = tty_ldisc_wait_idle(tty, 5 * HZ);
 
 	tty_lock();
 	mutex_lock(&tty->ldisc_mutex);
@@ -762,7 +763,7 @@ static int tty_ldisc_reinit(struct tty_struct *tty, int ldisc)
 	if (IS_ERR(ld))
 		return -1;
 
-	WARN_ON_ONCE(tty_ldisc_wait_idle(tty));
+	WARN_ON_ONCE(tty_ldisc_wait_idle(tty, 5 * HZ));
 
 	tty_ldisc_close(tty, tty->ldisc);
 	tty_ldisc_put(tty->ldisc);
-- 
1.7.7.3

[PATCH 2/5] TTY: ldisc, move wait idle to caller

[Jiri Slaby]

It is the only place where reinit is called from. And we really need
to wait for the old ldisc to go once. Actually this is the place where
the waiting originally was (before removed and re-added later).

This will make the fix in the following patch easier to implement.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Dave Young <hidave.darkstar@gmail.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Dmitriy Matrosov <sgf.dma@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: stable <stable@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/tty_ldisc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 534d176..a69a755 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -763,8 +763,6 @@ static int tty_ldisc_reinit(struct tty_struct *tty, int ldisc)
 	if (IS_ERR(ld))
 		return -1;
 
-	WARN_ON_ONCE(tty_ldisc_wait_idle(tty, 5 * HZ));
-
 	tty_ldisc_close(tty, tty->ldisc);
 	tty_ldisc_put(tty->ldisc);
 	tty->ldisc = NULL;
@@ -848,6 +846,8 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	   it means auditing a lot of other paths so this is
 	   a FIXME */
 	if (tty->ldisc) {	/* Not yet closed */
+		WARN_ON_ONCE(tty_ldisc_wait_idle(tty, 5 * HZ));
+
 		if (reset == 0) {
 
 			if (!tty_ldisc_reinit(tty, tty->termios->c_line))
-- 
1.7.7.3

Re: [PATCH 2/5] TTY: ldisc, move wait idle to caller

[Tony Luck]

One of my systems (HP rx2620 - ia64) has started hanging during shutdown.
Last messages on the serial console are:

Shutting down service (localfs) network  .  .  .  .  .  .  .  .  .   done
Shutting down D-Bus daemon                                           done
Running /etc/ini

and there it hangs forever in mid word (totally repeatable stops at "ini" every
time).

git bisect blames:

commit 300420722e0734a4254f3b634e0f82664495d210
Author: Jiri Slaby <jslaby@suse.cz>
Date:   Wed Nov 16 16:27:08 2011 +0100

    TTY: ldisc, move wait idle to caller

[Though the following commit "TTY: ldisc, wait for ldisc infinitely in hangup"
 sounds like it fits the symptoms]

Ideas?

-Tony

Re: [PATCH 2/5] TTY: ldisc, move wait idle to caller

[Tony Luck]

On Thu, Dec 1, 2011 at 3:01 PM, Tony Luck <tony.luck@intel.com> wrote:
> [Though the following commit "TTY: ldisc, wait for ldisc infinitely in hangup"
>  sounds like it fits the symptoms]

Confirmed - reverting the "wait for ldisc infinitely" cures the hang problem (I
do seem to be hitting the 5 second delay, and there is a pause that feels
about 5 seconds long)

-Tony

Re: [PATCH 2/5] TTY: ldisc, move wait idle to caller

[Jiri Slaby]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

On 12/02/2011 12:01 AM, Tony Luck wrote:
> One of my systems (HP rx2620 - ia64) has started hanging during shutdown.
> Last messages on the serial console are:
> 
> Shutting down service (localfs) network  .  .  .  .  .  .  .  .  .   done
> Shutting down D-Bus daemon                                           done
> Running /etc/ini
> 
> and there it hangs forever in mid word (totally repeatable stops at "ini" every
> time).
> 
> git bisect blames:
> 
> commit 300420722e0734a4254f3b634e0f82664495d210
> Author: Jiri Slaby <jslaby@suse.cz>
> Date:   Wed Nov 16 16:27:08 2011 +0100
> 
>     TTY: ldisc, move wait idle to caller
> 
> [Though the following commit "TTY: ldisc, wait for ldisc infinitely in hangup"
>  sounds like it fits the symptoms]
> 
> Ideas?

Thanks. What distro and init do you use? Does the attached patch help?

-- 
js
suse labs

[-- Attachment #2: 0001-TTY-ldisc-don-t-wait-with-ldisc_mutex.patch --]
[-- Type: text/x-patch, Size: 1954 bytes --]

>From 6f778e80388f8b1ad65f0c8bfbc874704d77b3eb Mon Sep 17 00:00:00 2001
From: Jiri Slaby <jslaby@suse.cz>
Date: Fri, 2 Dec 2011 09:19:05 +0100
Subject: [PATCH 1/1] TTY: ldisc, don't wait with ldisc_mutex

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/tty_ldisc.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 24b95db..c93e113 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -527,6 +527,14 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
 	flush_work_sync(&tty->buf.work);
 }
 
+static bool tty_ldisc_is_idle(struct tty_struct *tty)
+{
+	bool idle;
+	mutex_lock(&tty->ldisc_mutex);
+	idle = !tty->ldisc || atomic_read(&tty->ldisc->users) == 1;
+	mutex_unlock(&tty->ldisc_mutex);
+	return idle;
+}
 /**
  *	tty_ldisc_wait_idle	-	wait for the ldisc to become idle
  *	@tty: tty to wait for
@@ -534,12 +542,14 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
  *
  *	Wait for the line discipline to become idle. The discipline must
  *	have been halted for this to guarantee it remains idle.
+ *
+ *	Locking: neither BTM, nor tty->ldisc_mutex must be held
  */
 static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
 {
 	long ret;
-	ret = wait_event_timeout(tty_ldisc_idle,
-			atomic_read(&tty->ldisc->users) == 1, timeout);
+	ret = wait_event_timeout(tty_ldisc_idle, tty_ldisc_is_idle(tty),
+			timeout);
 	return ret > 0 ? 0 : -EBUSY;
 }
 
@@ -830,6 +840,7 @@ retry:
 		if (atomic_read(&tty->ldisc->users) != 1) {
 			char cur_n[TASK_COMM_LEN], tty_n[64];
 			long timeout = 3 * HZ;
+			mutex_unlock(&tty->ldisc_mutex);
 			tty_unlock();
 
 			while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
@@ -839,7 +850,6 @@ retry:
 					__func__, get_task_comm(cur_n, current),
 					tty_name(tty, tty_n));
 			}
-			mutex_unlock(&tty->ldisc_mutex);
 			goto retry;
 		}
 
-- 
1.7.7.3

Re: [PATCH 2/5] TTY: ldisc, move wait idle to caller

[Tony Luck]

On Fri, Dec 2, 2011 at 12:21 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> Thanks. What distro and init do you use? Does the attached patch help?

Patch didn't apply cleanly ... it looks like you have another patch that
deleted these lines:
        if (ret < 0)
                return ret;
from tty_ldisc_wait_idle() ... so I think I hand applied around this by dropping
those lines too - but I might have goofed.

Anyway - the patch as modified by me didn't work. I still see the hang.

Base distro is SLES11-SP1
Init version: sysvinit-2.86-198.20

System is headless (no graphic card) so serial console is all I have.

ia64 early serial console is enabled with boot argument:
console=uart,mmio,0xff5e0000

-Tony

[PATCH 3/5] TTY: ldisc, wait for ldisc infinitely in hangup

[Jiri Slaby]

For /dev/console case, we do not kill all ldisc users. It's due to
redirected_tty_write test in __tty_hangup. In that case there still
might be a process waiting e.g. in n_tty_read for input.

We wait for such processes to disappear. The problem is that we use a
timeout. After this timeout, we continue closing the ldisc and start
freeing tty resources. It obviously leads to crashes when the other
process is woken.

So to fix this, we wait infinitely before reiniting the ldisc. (The
tiocsetd remains untouched -- times out after 5s.)

This is nicely reproducible with this run from shell:
  exec 0<>/dev/console 1<>/dev/console 2<>/dev/console
and stopping a getty like:
  systemctl stop serial-getty@ttyS0.service

The crash proper may be produced only under load or with constified
timing the same as for 92f6fa09b.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Dave Young <hidave.darkstar@gmail.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Dmitriy Matrosov <sgf.dma@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: stable <stable@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/tty_ldisc.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index a69a755..8e0924f 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -36,6 +36,7 @@
 
 #include <linux/kmod.h>
 #include <linux/nsproxy.h>
+#include <linux/ratelimit.h>
 
 /*
  *	This guards the refcounted line discipline lists. The lock
@@ -837,7 +838,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	tty_unlock();
 	cancel_work_sync(&tty->buf.work);
 	mutex_unlock(&tty->ldisc_mutex);
-
+retry:
 	tty_lock();
 	mutex_lock(&tty->ldisc_mutex);
 
@@ -846,7 +847,21 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	   it means auditing a lot of other paths so this is
 	   a FIXME */
 	if (tty->ldisc) {	/* Not yet closed */
-		WARN_ON_ONCE(tty_ldisc_wait_idle(tty, 5 * HZ));
+		if (atomic_read(&tty->ldisc->users) != 1) {
+			char cur_n[TASK_COMM_LEN], tty_n[64];
+			long timeout = 3 * HZ;
+			tty_unlock();
+
+			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 (reset == 0) {
 
-- 
1.7.7.3

[PATCH 4/5] TTY: ldisc, remove some unneeded includes

[Jiri Slaby]

They were cut&pasted from tty_io. Many of them are not needed in
tty_ldisc.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@linux.intel.com>
---
 drivers/tty/tty_ldisc.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 8e0924f..6b344ae 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -1,19 +1,11 @@
 #include <linux/types.h>
-#include <linux/major.h>
 #include <linux/errno.h>
-#include <linux/signal.h>
-#include <linux/fcntl.h>
+#include <linux/kmod.h>
 #include <linux/sched.h>
 #include <linux/interrupt.h>
 #include <linux/tty.h>
 #include <linux/tty_driver.h>
-#include <linux/tty_flip.h>
-#include <linux/devpts_fs.h>
 #include <linux/file.h>
-#include <linux/console.h>
-#include <linux/timer.h>
-#include <linux/ctype.h>
-#include <linux/kd.h>
 #include <linux/mm.h>
 #include <linux/string.h>
 #include <linux/slab.h>
@@ -24,19 +16,9 @@
 #include <linux/device.h>
 #include <linux/wait.h>
 #include <linux/bitops.h>
-#include <linux/delay.h>
 #include <linux/seq_file.h>
-
-#include <linux/uaccess.h>
-#include <asm/system.h>
-
-#include <linux/kbd_kern.h>
-#include <linux/vt_kern.h>
-#include <linux/selection.h>
-
-#include <linux/kmod.h>
-#include <linux/nsproxy.h>
 #include <linux/ratelimit.h>
+#include <linux/uaccess.h>
 
 /*
  *	This guards the refcounted line discipline lists. The lock
-- 
1.7.7.3

[PATCH 5/5] TTY: pty, cleanup the pty counting

[Jiri Slaby]

Instead of the hackish way of counting ptys, let's define a specific
->remove hook both from slave and master. And decrease the count only
for master.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@linux.intel.com>
---
 drivers/tty/pty.c |   26 +++++++++-----------------
 1 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index e18604b..d8653ab 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -446,19 +446,8 @@ static inline void legacy_pty_init(void) { }
 int pty_limit = NR_UNIX98_PTY_DEFAULT;
 static int pty_limit_min;
 static int pty_limit_max = NR_UNIX98_PTY_MAX;
-static int tty_count;
 static int pty_count;
 
-static inline void pty_inc_count(void)
-{
-	pty_count = (++tty_count) / 2;
-}
-
-static inline void pty_dec_count(void)
-{
-	pty_count = (--tty_count) / 2;
-}
-
 static struct cdev ptmx_cdev;
 
 static struct ctl_table pty_table[] = {
@@ -600,8 +589,7 @@ static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
 	 */
 	tty_driver_kref_get(driver);
 	tty->count++;
-	pty_inc_count(); /* tty */
-	pty_inc_count(); /* tty->link */
+	pty_count++;
 	return 0;
 err_free_mem:
 	deinitialize_tty_struct(o_tty);
@@ -613,15 +601,19 @@ err_free_tty:
 	return -ENOMEM;
 }
 
-static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
+static void ptm_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
+{
+	pty_count--;
+}
+
+static void pts_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
 {
-	pty_dec_count();
 }
 
 static const struct tty_operations ptm_unix98_ops = {
 	.lookup = ptm_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
+	.remove = ptm_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,
@@ -638,7 +630,7 @@ static const struct tty_operations ptm_unix98_ops = {
 static const struct tty_operations pty_unix98_ops = {
 	.lookup = pts_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
+	.remove = pts_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,
-- 
1.7.7.3