[PATCH 0/5] Fix session leader exit hang

[PATCH 0/5] Fix session leader exit hang

[Peter Hurley]

When a session leader exits, it hangs up the controlling tty and
then signals the foreground group processes with SIGHUP. This sequence
can hang as reported by Sasha Levin, and noted in the changelog of
'tty: Signal SIGHUP before hanging up ldisc'.


Peter Hurley (5):
  tty: Refactor session leader SIGHUP from __tty_hangup()
  tty: Fix spinlock flavor in non-atomic __tty_hangup()
  tty: Use spin_lock() inside existing critical region
  tty: Signal foreground group processes in hangup
  tty: Signal SIGHUP before hanging up ldisc

 drivers/tty/tty_io.c | 144 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 96 insertions(+), 48 deletions(-)

-- 
1.8.1.2

[PATCH 1/5] tty: Refactor session leader SIGHUP from __tty_hangup()

[Peter Hurley]

Reduce complexity of __tty_hangup(); separate SIGHUP signalling
into tty_signal_session_leader().

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index adc1d01..e454f1a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -533,6 +533,51 @@ void tty_wakeup(struct tty_struct *tty)
 EXPORT_SYMBOL_GPL(tty_wakeup);
 
 /**
+ *	tty_signal_session_leader	- sends SIGHUP to session leader
+ *
+ *	Send SIGHUP and SIGCONT to the session leader and its
+ *	process group.
+ *
+ *	Returns the number of processes in the session with this tty
+ *	as their controlling terminal. This value is used to drop
+ *	tty references for those processes.
+ */
+static int tty_signal_session_leader(struct tty_struct *tty)
+{
+	struct task_struct *p;
+	unsigned long flags;
+	int refs = 0;
+
+	read_lock(&tasklist_lock);
+	if (tty->session) {
+		do_each_pid_task(tty->session, PIDTYPE_SID, p) {
+			spin_lock_irq(&p->sighand->siglock);
+			if (p->signal->tty == tty) {
+				p->signal->tty = NULL;
+				/* We defer the dereferences outside fo
+				   the tasklist lock */
+				refs++;
+			}
+			if (!p->signal->leader) {
+				spin_unlock_irq(&p->sighand->siglock);
+				continue;
+			}
+			__group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p);
+			__group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p);
+			put_pid(p->signal->tty_old_pgrp);  /* A noop */
+			spin_lock_irqsave(&tty->ctrl_lock, flags);
+			if (tty->pgrp)
+				p->signal->tty_old_pgrp = get_pid(tty->pgrp);
+			spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+			spin_unlock_irq(&p->sighand->siglock);
+		} while_each_pid_task(tty->session, PIDTYPE_SID, p);
+	}
+	read_unlock(&tasklist_lock);
+
+	return refs;
+}
+
+/**
  *	__tty_hangup		-	actual handler for hangup events
  *	@work: tty device
  *
@@ -558,11 +603,10 @@ static void __tty_hangup(struct tty_struct *tty)
 {
 	struct file *cons_filp = NULL;
 	struct file *filp, *f = NULL;
-	struct task_struct *p;
 	struct tty_file_private *priv;
 	int    closecount = 0, n;
 	unsigned long flags;
-	int refs = 0;
+	int refs;
 
 	if (!tty)
 		return;
@@ -605,31 +649,10 @@ static void __tty_hangup(struct tty_struct *tty)
 	 */
 	tty_ldisc_hangup(tty);
 
-	read_lock(&tasklist_lock);
-	if (tty->session) {
-		do_each_pid_task(tty->session, PIDTYPE_SID, p) {
-			spin_lock_irq(&p->sighand->siglock);
-			if (p->signal->tty == tty) {
-				p->signal->tty = NULL;
-				/* We defer the dereferences outside fo
-				   the tasklist lock */
-				refs++;
-			}
-			if (!p->signal->leader) {
-				spin_unlock_irq(&p->sighand->siglock);
-				continue;
-			}
-			__group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p);
-			__group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p);
-			put_pid(p->signal->tty_old_pgrp);  /* A noop */
-			spin_lock_irqsave(&tty->ctrl_lock, flags);
-			if (tty->pgrp)
-				p->signal->tty_old_pgrp = get_pid(tty->pgrp);
-			spin_unlock_irqrestore(&tty->ctrl_lock, flags);
-			spin_unlock_irq(&p->sighand->siglock);
-		} while_each_pid_task(tty->session, PIDTYPE_SID, p);
-	}
-	read_unlock(&tasklist_lock);
+	refs = tty_signal_session_leader(tty);
+	/* Account for the p->signal references we killed */
+	while (refs--)
+		tty_kref_put(tty);
 
 	spin_lock_irqsave(&tty->ctrl_lock, flags);
 	clear_bit(TTY_THROTTLED, &tty->flags);
@@ -642,10 +665,6 @@ static void __tty_hangup(struct tty_struct *tty)
 	tty->ctrl_status = 0;
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 
-	/* Account for the p->signal references we killed */
-	while (refs--)
-		tty_kref_put(tty);
-
 	/*
 	 * If one of the devices matches a console pointer, we
 	 * cannot just call hangup() because that will cause
-- 
1.8.1.2

[PATCH 2/5] tty: Fix spinlock flavor in non-atomic __tty_hangup()

[Peter Hurley]

__tty_hangup() and tty_vhangup() cannot be called from atomic context,
so locks do not need to preserve the interrupt state (although,
still disable interrupts).

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e454f1a..f9d4acf 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -605,7 +605,6 @@ static void __tty_hangup(struct tty_struct *tty)
 	struct file *filp, *f = NULL;
 	struct tty_file_private *priv;
 	int    closecount = 0, n;
-	unsigned long flags;
 	int refs;
 
 	if (!tty)
@@ -654,7 +653,7 @@ static void __tty_hangup(struct tty_struct *tty)
 	while (refs--)
 		tty_kref_put(tty);
 
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	spin_lock_irq(&tty->ctrl_lock);
 	clear_bit(TTY_THROTTLED, &tty->flags);
 	clear_bit(TTY_PUSH, &tty->flags);
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
@@ -663,7 +662,7 @@ static void __tty_hangup(struct tty_struct *tty)
 	tty->session = NULL;
 	tty->pgrp = NULL;
 	tty->ctrl_status = 0;
-	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+	spin_unlock_irq(&tty->ctrl_lock);
 
 	/*
 	 * If one of the devices matches a console pointer, we
-- 
1.8.1.2

[PATCH 3/5] tty: Use spin_lock() inside existing critical region

[Peter Hurley]

The interrupt state does not need to be saved, disabled and
restored here; interrupts are already off because this lock
is bracketed by spin_lock_irq/spin_unlock_irq.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index f9d4acf..40d3a7a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -545,7 +545,6 @@ EXPORT_SYMBOL_GPL(tty_wakeup);
 static int tty_signal_session_leader(struct tty_struct *tty)
 {
 	struct task_struct *p;
-	unsigned long flags;
 	int refs = 0;
 
 	read_lock(&tasklist_lock);
@@ -565,10 +564,10 @@ static int tty_signal_session_leader(struct tty_struct *tty)
 			__group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p);
 			__group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p);
 			put_pid(p->signal->tty_old_pgrp);  /* A noop */
-			spin_lock_irqsave(&tty->ctrl_lock, flags);
+			spin_lock(&tty->ctrl_lock);
 			if (tty->pgrp)
 				p->signal->tty_old_pgrp = get_pid(tty->pgrp);
-			spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+			spin_unlock(&tty->ctrl_lock);
 			spin_unlock_irq(&p->sighand->siglock);
 		} while_each_pid_task(tty->session, PIDTYPE_SID, p);
 	}
-- 
1.8.1.2

[PATCH 4/5] tty: Signal foreground group processes in hangup

[Peter Hurley]

When the session leader is exiting, signal the foreground group
processes as part of the hangup sequence, instead of after the
hangup is complete. This prepares for hanging up the
line discipline _after_ signalling processes which
may be blocking on ldisc i/o.

Parameterize __tty_hangup() to distinguish between when the
session leader is exiting and all other hangups; signal the
foreground group after signalling the session leader and its
process group, which preserves the original signal order.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 40d3a7a..cc6727e 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -534,18 +534,21 @@ EXPORT_SYMBOL_GPL(tty_wakeup);
 
 /**
  *	tty_signal_session_leader	- sends SIGHUP to session leader
+ *	@tty		controlling tty
+ *	@exit_session	if non-zero, signal all foreground group processes
  *
- *	Send SIGHUP and SIGCONT to the session leader and its
- *	process group.
+ *	Send SIGHUP and SIGCONT to the session leader and its process group.
+ *	Optionally, signal all processes in the foreground process group.
  *
  *	Returns the number of processes in the session with this tty
  *	as their controlling terminal. This value is used to drop
  *	tty references for those processes.
  */
-static int tty_signal_session_leader(struct tty_struct *tty)
+static int tty_signal_session_leader(struct tty_struct *tty, int exit_session)
 {
 	struct task_struct *p;
 	int refs = 0;
+	struct pid *tty_pgrp = NULL;
 
 	read_lock(&tasklist_lock);
 	if (tty->session) {
@@ -565,6 +568,7 @@ static int tty_signal_session_leader(struct tty_struct *tty)
 			__group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p);
 			put_pid(p->signal->tty_old_pgrp);  /* A noop */
 			spin_lock(&tty->ctrl_lock);
+			tty_pgrp = get_pid(tty->pgrp);
 			if (tty->pgrp)
 				p->signal->tty_old_pgrp = get_pid(tty->pgrp);
 			spin_unlock(&tty->ctrl_lock);
@@ -573,6 +577,12 @@ static int tty_signal_session_leader(struct tty_struct *tty)
 	}
 	read_unlock(&tasklist_lock);
 
+	if (tty_pgrp) {
+		if (exit_session)
+			kill_pgrp(tty_pgrp, SIGHUP, exit_session);
+		put_pid(tty_pgrp);
+	}
+
 	return refs;
 }
 
@@ -598,7 +608,7 @@ static int tty_signal_session_leader(struct tty_struct *tty)
  *		  tasklist_lock to walk task list for hangup event
  *		    ->siglock to protect ->signal/->sighand
  */
-static void __tty_hangup(struct tty_struct *tty)
+static void __tty_hangup(struct tty_struct *tty, int exit_session)
 {
 	struct file *cons_filp = NULL;
 	struct file *filp, *f = NULL;
@@ -647,7 +657,7 @@ static void __tty_hangup(struct tty_struct *tty)
 	 */
 	tty_ldisc_hangup(tty);
 
-	refs = tty_signal_session_leader(tty);
+	refs = tty_signal_session_leader(tty, exit_session);
 	/* Account for the p->signal references we killed */
 	while (refs--)
 		tty_kref_put(tty);
@@ -695,7 +705,7 @@ static void do_tty_hangup(struct work_struct *work)
 	struct tty_struct *tty =
 		container_of(work, struct tty_struct, hangup_work);
 
-	__tty_hangup(tty);
+	__tty_hangup(tty, 0);
 }
 
 /**
@@ -733,7 +743,7 @@ void tty_vhangup(struct tty_struct *tty)
 
 	printk(KERN_DEBUG "%s vhangup...\n", tty_name(tty, buf));
 #endif
-	__tty_hangup(tty);
+	__tty_hangup(tty, 0);
 }
 
 EXPORT_SYMBOL(tty_vhangup);
@@ -757,6 +767,27 @@ void tty_vhangup_self(void)
 }
 
 /**
+ *	tty_vhangup_session		-	hangup session leader exit
+ *	@tty: tty to hangup
+ *
+ *	The session leader is exiting and hanging up its controlling terminal.
+ *	Every process in the foreground process group is signalled SIGHUP.
+ *
+ *	We do this synchronously so that when the syscall returns the process
+ *	is complete. That guarantee is necessary for security reasons.
+ */
+
+void tty_vhangup_session(struct tty_struct *tty)
+{
+#ifdef TTY_DEBUG_HANGUP
+	char	buf[64];
+
+	printk(KERN_DEBUG "%s vhangup session...\n", tty_name(tty, buf));
+#endif
+	__tty_hangup(tty, 1);
+}
+
+/**
  *	tty_hung_up_p		-	was tty hung up
  *	@filp: file pointer of tty
  *
@@ -813,18 +844,18 @@ void disassociate_ctty(int on_exit)
 
 	tty = get_current_tty();
 	if (tty) {
-		struct pid *tty_pgrp = get_pid(tty->pgrp);
-		if (on_exit) {
-			if (tty->driver->type != TTY_DRIVER_TYPE_PTY)
-				tty_vhangup(tty);
-		}
-		tty_kref_put(tty);
-		if (tty_pgrp) {
-			kill_pgrp(tty_pgrp, SIGHUP, on_exit);
-			if (!on_exit)
+		if (on_exit && tty->driver->type != TTY_DRIVER_TYPE_PTY) {
+			tty_vhangup_session(tty);
+		} else {
+			struct pid *tty_pgrp = tty_get_pgrp(tty);
+			if (tty_pgrp) {
+				kill_pgrp(tty_pgrp, SIGHUP, on_exit);
 				kill_pgrp(tty_pgrp, SIGCONT, on_exit);
-			put_pid(tty_pgrp);
+				put_pid(tty_pgrp);
+			}
 		}
+		tty_kref_put(tty);
+
 	} else if (on_exit) {
 		struct pid *old_pgrp;
 		spin_lock_irq(&current->sighand->siglock);
-- 
1.8.1.2

[PATCH 5/5] tty: Signal SIGHUP before hanging up ldisc

[Peter Hurley]

An exiting session leader can hang if a foreground process is
blocking for line discipline i/o, eg. in n_tty_read(). This happens
because the blocking reader is holding an ldisc reference (indicating
the line discipline is in-use) which prevents __tty_hangup() from
recycling the line discipline. Although waiters are woken before
attempting to gain exclusive access for changing the ldisc, the
blocking reader in this case will not exit the i/o loop since it
has not yet received SIGHUP (because it has not been sent).

Instead, perform signalling first, then recycle the line discipline.

Fixes:

INFO: task init:1 blocked for more than 120 seconds.
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
init            D 00000000001d7180  2688     1      0 0x00000002
 ffff8800b9acfba8 0000000000000002 00000000001d7180 ffff8800b9b10048
 ffff8800b94cb000 ffff8800b9b10000 00000000001d7180 00000000001d7180
 ffff8800b9b10000 ffff8800b9acffd8 00000000001d7180 00000000001d7180
Call Trace:
 [<ffffffff83db9909>] __schedule+0x2e9/0x3b0
 [<ffffffff83db9b35>] schedule+0x55/0x60
 [<ffffffff83db74ba>] schedule_timeout+0x3a/0x370
 [<ffffffff81182349>] ? mark_held_locks+0xf9/0x130
 [<ffffffff83dbab38>] ? down_failed+0x108/0x200
 [<ffffffff83dbb7ab>] ? _raw_spin_unlock_irq+0x2b/0x80
 [<ffffffff81182608>] ? trace_hardirqs_on_caller+0x128/0x160
 [<ffffffff83dbab61>] down_failed+0x131/0x200
 [<ffffffff83dbbfad>] ? tty_ldisc_lock_pair_timeout+0xcd/0x120
 [<ffffffff83dbae03>] ldsem_down_write+0xd3/0x113
 [<ffffffff83dbbfad>] ? tty_ldisc_lock_pair_timeout+0xcd/0x120
 [<ffffffff8118264d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff83dbbfad>] tty_ldisc_lock_pair_timeout+0xcd/0x120
 [<ffffffff81c3df60>] tty_ldisc_hangup+0xd0/0x220
 [<ffffffff81c35bd7>] __tty_hangup+0x137/0x4f0
 [<ffffffff81c37c7c>] disassociate_ctty+0x6c/0x230
 [<ffffffff8111290c>] do_exit+0x41c/0x590
 [<ffffffff8107ad34>] ? syscall_trace_enter+0x24/0x2e0
 [<ffffffff81112b4a>] do_group_exit+0x8a/0xc0
 [<ffffffff81112b92>] sys_exit_group+0x12/0x20
 [<ffffffff83dc49d8>] tracesys+0xe1/0xe6
1 lock held by init/1:
 #0: (&tty->ldisc_sem){++++++}, at: [<ffffffff83dbbfad>] tty_ldisc_lock_pair_timeout+0xcd/0x120

Reported-by: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index cc6727e..d93573c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -651,17 +651,17 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 	}
 	spin_unlock(&tty_files_lock);
 
+	refs = tty_signal_session_leader(tty, exit_session);
+	/* Account for the p->signal references we killed */
+	while (refs--)
+		tty_kref_put(tty);
+
 	/*
 	 * it drops BTM and thus races with reopen
 	 * we protect the race by TTY_HUPPING
 	 */
 	tty_ldisc_hangup(tty);
 
-	refs = tty_signal_session_leader(tty, exit_session);
-	/* Account for the p->signal references we killed */
-	while (refs--)
-		tty_kref_put(tty);
-
 	spin_lock_irq(&tty->ctrl_lock);
 	clear_bit(TTY_THROTTLED, &tty->flags);
 	clear_bit(TTY_PUSH, &tty->flags);
-- 
1.8.1.2

Re: [PATCH 0/5] Fix session leader exit hang

[Jiri Slaby]

On 03/06/2013 01:20 PM, Peter Hurley wrote:
> When a session leader exits, it hangs up the controlling tty and
> then signals the foreground group processes with SIGHUP. This sequence
> can hang as reported by Sasha Levin, and noted in the changelog of
> 'tty: Signal SIGHUP before hanging up ldisc'.
> 
> 
> Peter Hurley (5):
>   tty: Refactor session leader SIGHUP from __tty_hangup()
>   tty: Fix spinlock flavor in non-atomic __tty_hangup()
>   tty: Use spin_lock() inside existing critical region
>   tty: Signal foreground group processes in hangup
>   tty: Signal SIGHUP before hanging up ldisc
> 
>  drivers/tty/tty_io.c | 144 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 96 insertions(+), 48 deletions(-)
> 

Hi, all three series look good to me.

-- 
js
suse labs