[PATCH -next 00/10] Fixes to controlling tty handling

[PATCH -next 00/10] Fixes to controlling tty handling

[Peter Hurley]

Hi Greg,

This patch series:
1. removes stale code from the controlling tty handling functions
2. relocates the ctty functions to eliminate forward declarations
3. fixes several unsafe races when setting the controlling tty
4. eliminates holding tty_mutex as a necessary condition of
   setting the controlling terminal

#4 is part of an overall effort to reduce the tty_mutex footprint.

Unfortunately, this series does not fix two other race conditions:
1. disassociate_ctty()/no_tty() does not teardown the tty<->process
associations atomically wrt job control, so it is possible to
observe spurious error conditions from job control (tty_check_change()
and job_control()). I'm looking into inverting the lock order of
tty->ctrl_lock and tsk->sighand->siglock() to see if holding ctrl_lock
is a suitable solution for atomic teardown. Especially now that
ctrl_lock is not used for flow control anymore :)
2. task_pgrp() and task_session() are used unsafely. These fixes
will be clearer after #1 is fixed.

Regards,

Peter Hurley (10):
  tty: Remove tty_pair_get_tty()/tty_pair_get_pty() api
  tty: Reorder proc_set_tty() and related fns
  tty: Remove tsk parameter from proc_set_tty()
  uml: Fix unsafe pid reference to foreground process group
  tty: Replace open-coded tty_get_pgrp()
  tty: Remove !tty condition from __proc_set_tty()
  tty: Fix multiple races when setting the controlling terminal
  tty: Move session_of_pgrp() and make static
  tty: Serialize proc_set_tty() with tty_lock
  tty: Update code comment in __proc_set_tty()

 arch/um/drivers/line.c |   6 +-
 drivers/tty/pty.c      |  24 ++----
 drivers/tty/tty_io.c   | 204 +++++++++++++++++++++++++++----------------------
 include/linux/kernel.h |   3 -
 include/linux/tty.h    |   3 -
 kernel/exit.c          |  21 -----
 6 files changed, 123 insertions(+), 138 deletions(-)

-- 
2.1.1

[PATCH -next 01/10] tty: Remove tty_pair_get_tty()/tty_pair_get_pty() api

[Peter Hurley]

tty_pair_get_pty() has no in-tree users and tty_pair_get_tty()
has only one file-local user. Remove the external declarations,
the export declarations, and declare tty_pair_get_tty() static.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 16 +++++-----------
 include/linux/tty.h  |  3 ---
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 8cce5ab..274e386 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2710,23 +2710,17 @@ static int tty_tiocgicount(struct tty_struct *tty, void __user *arg)
 	return 0;
 }
 
-struct tty_struct *tty_pair_get_tty(struct tty_struct *tty)
+/*
+ * if pty, return the slave side (real_tty)
+ * otherwise, return self
+ */
+static struct tty_struct *tty_pair_get_tty(struct tty_struct *tty)
 {
 	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
 	    tty->driver->subtype == PTY_TYPE_MASTER)
 		tty = tty->link;
 	return tty;
 }
-EXPORT_SYMBOL(tty_pair_get_tty);
-
-struct tty_struct *tty_pair_get_pty(struct tty_struct *tty)
-{
-	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
-	    tty->driver->subtype == PTY_TYPE_MASTER)
-	    return tty;
-	return tty->link;
-}
-EXPORT_SYMBOL(tty_pair_get_pty);
 
 /*
  * Split this up, as gcc can choke on it otherwise..
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 5171ef8..b36b0b4 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -498,9 +498,6 @@ extern int tty_init_termios(struct tty_struct *tty);
 extern int tty_standard_install(struct tty_driver *driver,
 		struct tty_struct *tty);
 
-extern struct tty_struct *tty_pair_get_tty(struct tty_struct *tty);
-extern struct tty_struct *tty_pair_get_pty(struct tty_struct *tty);
-
 extern struct mutex tty_mutex;
 extern spinlock_t tty_files_lock;
 
-- 
2.1.1

[PATCH -next 02/10] tty: Reorder proc_set_tty() and related fns

[Peter Hurley]

Move the controlling tty-related functions and remove forward
declarations for __proc_set_tty() and proc_set_tty().

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 274e386..012647b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -153,8 +153,6 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
 static int __tty_fasync(int fd, struct file *filp, int on);
 static int tty_fasync(int fd, struct file *filp, int on);
 static void release_tty(struct tty_struct *tty, int idx);
-static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
-static void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
 
 /**
  *	free_tty_struct		-	free a disused tty
@@ -492,6 +490,68 @@ static const struct file_operations hung_up_tty_fops = {
 static DEFINE_SPINLOCK(redirect_lock);
 static struct file *redirect;
 
+
+void proc_clear_tty(struct task_struct *p)
+{
+	unsigned long flags;
+	struct tty_struct *tty;
+	spin_lock_irqsave(&p->sighand->siglock, flags);
+	tty = p->signal->tty;
+	p->signal->tty = NULL;
+	spin_unlock_irqrestore(&p->sighand->siglock, flags);
+	tty_kref_put(tty);
+}
+
+/* Called under the sighand lock */
+
+static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
+{
+	if (tty) {
+		unsigned long flags;
+		/* We should not have a session or pgrp to put here but.... */
+		spin_lock_irqsave(&tty->ctrl_lock, flags);
+		put_pid(tty->session);
+		put_pid(tty->pgrp);
+		tty->pgrp = get_pid(task_pgrp(tsk));
+		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+		tty->session = get_pid(task_session(tsk));
+		if (tsk->signal->tty) {
+			printk(KERN_DEBUG "tty not NULL!!\n");
+			tty_kref_put(tsk->signal->tty);
+		}
+	}
+	put_pid(tsk->signal->tty_old_pgrp);
+	tsk->signal->tty = tty_kref_get(tty);
+	tsk->signal->tty_old_pgrp = NULL;
+}
+
+static void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
+{
+	spin_lock_irq(&tsk->sighand->siglock);
+	__proc_set_tty(tsk, tty);
+	spin_unlock_irq(&tsk->sighand->siglock);
+}
+
+struct tty_struct *get_current_tty(void)
+{
+	struct tty_struct *tty;
+	unsigned long flags;
+
+	spin_lock_irqsave(&current->sighand->siglock, flags);
+	tty = tty_kref_get(current->signal->tty);
+	spin_unlock_irqrestore(&current->sighand->siglock, flags);
+	return tty;
+}
+EXPORT_SYMBOL_GPL(get_current_tty);
+
+static void session_clear_tty(struct pid *session)
+{
+	struct task_struct *p;
+	do_each_pid_task(session, PIDTYPE_SID, p) {
+		proc_clear_tty(p);
+	} while_each_pid_task(session, PIDTYPE_SID, p);
+}
+
 /**
  *	tty_wakeup	-	request more data
  *	@tty: terminal
@@ -792,14 +852,6 @@ int tty_hung_up_p(struct file *filp)
 
 EXPORT_SYMBOL(tty_hung_up_p);
 
-static void session_clear_tty(struct pid *session)
-{
-	struct task_struct *p;
-	do_each_pid_task(session, PIDTYPE_SID, p) {
-		proc_clear_tty(p);
-	} while_each_pid_task(session, PIDTYPE_SID, p);
-}
-
 /**
  *	disassociate_ctty	-	disconnect controlling tty
  *	@on_exit: true if exiting so need to "hang up" the session
@@ -3433,59 +3485,6 @@ dev_t tty_devnum(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(tty_devnum);
 
-void proc_clear_tty(struct task_struct *p)
-{
-	unsigned long flags;
-	struct tty_struct *tty;
-	spin_lock_irqsave(&p->sighand->siglock, flags);
-	tty = p->signal->tty;
-	p->signal->tty = NULL;
-	spin_unlock_irqrestore(&p->sighand->siglock, flags);
-	tty_kref_put(tty);
-}
-
-/* Called under the sighand lock */
-
-static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
-{
-	if (tty) {
-		unsigned long flags;
-		/* We should not have a session or pgrp to put here but.... */
-		spin_lock_irqsave(&tty->ctrl_lock, flags);
-		put_pid(tty->session);
-		put_pid(tty->pgrp);
-		tty->pgrp = get_pid(task_pgrp(tsk));
-		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
-		tty->session = get_pid(task_session(tsk));
-		if (tsk->signal->tty) {
-			printk(KERN_DEBUG "tty not NULL!!\n");
-			tty_kref_put(tsk->signal->tty);
-		}
-	}
-	put_pid(tsk->signal->tty_old_pgrp);
-	tsk->signal->tty = tty_kref_get(tty);
-	tsk->signal->tty_old_pgrp = NULL;
-}
-
-static void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
-{
-	spin_lock_irq(&tsk->sighand->siglock);
-	__proc_set_tty(tsk, tty);
-	spin_unlock_irq(&tsk->sighand->siglock);
-}
-
-struct tty_struct *get_current_tty(void)
-{
-	struct tty_struct *tty;
-	unsigned long flags;
-
-	spin_lock_irqsave(&current->sighand->siglock, flags);
-	tty = tty_kref_get(current->signal->tty);
-	spin_unlock_irqrestore(&current->sighand->siglock, flags);
-	return tty;
-}
-EXPORT_SYMBOL_GPL(get_current_tty);
-
 void tty_default_fops(struct file_operations *fops)
 {
 	*fops = tty_fops;
-- 
2.1.1

[PATCH -next 03/10] tty: Remove tsk parameter from proc_set_tty()

[Peter Hurley]

Only the current task itself can set its controlling tty (other
than before the task has been forked). Equivalent to existing usage.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 012647b..272d977 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -504,7 +504,7 @@ void proc_clear_tty(struct task_struct *p)
 
 /* Called under the sighand lock */
 
-static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
+static void __proc_set_tty(struct tty_struct *tty)
 {
 	if (tty) {
 		unsigned long flags;
@@ -512,24 +512,24 @@ static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
 		spin_lock_irqsave(&tty->ctrl_lock, flags);
 		put_pid(tty->session);
 		put_pid(tty->pgrp);
-		tty->pgrp = get_pid(task_pgrp(tsk));
+		tty->pgrp = get_pid(task_pgrp(current));
 		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
-		tty->session = get_pid(task_session(tsk));
-		if (tsk->signal->tty) {
+		tty->session = get_pid(task_session(current));
+		if (current->signal->tty) {
 			printk(KERN_DEBUG "tty not NULL!!\n");
-			tty_kref_put(tsk->signal->tty);
+			tty_kref_put(current->signal->tty);
 		}
 	}
-	put_pid(tsk->signal->tty_old_pgrp);
-	tsk->signal->tty = tty_kref_get(tty);
-	tsk->signal->tty_old_pgrp = NULL;
+	put_pid(current->signal->tty_old_pgrp);
+	current->signal->tty = tty_kref_get(tty);
+	current->signal->tty_old_pgrp = NULL;
 }
 
-static void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
+static void proc_set_tty(struct tty_struct *tty)
 {
-	spin_lock_irq(&tsk->sighand->siglock);
-	__proc_set_tty(tsk, tty);
-	spin_unlock_irq(&tsk->sighand->siglock);
+	spin_lock_irq(&current->sighand->siglock);
+	__proc_set_tty(tty);
+	spin_unlock_irq(&current->sighand->siglock);
 }
 
 struct tty_struct *get_current_tty(void)
@@ -2164,7 +2164,7 @@ retry_open:
 	    current->signal->leader &&
 	    !current->signal->tty &&
 	    tty->session == NULL)
-		__proc_set_tty(current, tty);
+		__proc_set_tty(tty);
 	spin_unlock_irq(&current->sighand->siglock);
 	tty_unlock(tty);
 	mutex_unlock(&tty_mutex);
@@ -2489,7 +2489,7 @@ static int tiocsctty(struct tty_struct *tty, int arg)
 			goto unlock;
 		}
 	}
-	proc_set_tty(current, tty);
+	proc_set_tty(tty);
 unlock:
 	mutex_unlock(&tty_mutex);
 	return ret;
-- 
2.1.1

[PATCH -next 04/10] uml: Fix unsafe pid reference to foreground process group

[Peter Hurley]

Although the tty core maintains a pid reference for the foreground
process group, if the foreground process group is changed that
pid reference is dropped. Thus, the pid reference used for signalling
could become stale.

Safely obtain a pid reference to the foreground process group and
release the reference after signalling is complete.

cc: Jeff Dike <jdike@addtoit.com>
cc: Richard Weinberger <richard@nod.at>
cc: user-mode-linux-devel@lists.sourceforge.net
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 arch/um/drivers/line.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
index 8035145..6208702 100644
--- a/arch/um/drivers/line.c
+++ b/arch/um/drivers/line.c
@@ -632,6 +632,7 @@ static irqreturn_t winch_interrupt(int irq, void *data)
 	int fd = winch->fd;
 	int err;
 	char c;
+	struct pid *pgrp;
 
 	if (fd != -1) {
 		err = generic_read(fd, &c, NULL);
@@ -657,7 +658,10 @@ static irqreturn_t winch_interrupt(int irq, void *data)
 		if (line != NULL) {
 			chan_window_size(line, &tty->winsize.ws_row,
 					 &tty->winsize.ws_col);
-			kill_pgrp(tty->pgrp, SIGWINCH, 1);
+			pgrp = tty_get_pgrp(tty);
+			if (pgrp)
+				kill_pgrp(pgrp, SIGWINCH, 1);
+			put_pid(pgrp);
 		}
 		tty_kref_put(tty);
 	}
-- 
2.1.1

[PATCH -next 05/10] tty: Replace open-coded tty_get_pgrp()

[Peter Hurley]

Replace open-coded instances of tty_get_pgrp().

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c    | 24 ++++++------------------
 drivers/tty/tty_io.c |  8 ++------
 2 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 7c4447a..4c0218d 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -207,15 +207,12 @@ static int pty_get_pktmode(struct tty_struct *tty, int __user *arg)
 /* Send a signal to the slave */
 static int pty_signal(struct tty_struct *tty, int sig)
 {
-	unsigned long flags;
 	struct pid *pgrp;
 
 	if (tty->link) {
-		spin_lock_irqsave(&tty->link->ctrl_lock, flags);
-		pgrp = get_pid(tty->link->pgrp);
-		spin_unlock_irqrestore(&tty->link->ctrl_lock, flags);
-
-		kill_pgrp(pgrp, sig, 1);
+		pgrp = tty_get_pgrp(tty->link);
+		if (pgrp)
+			kill_pgrp(pgrp, sig, 1);
 		put_pid(pgrp);
 	}
 	return 0;
@@ -278,7 +275,6 @@ static void pty_set_termios(struct tty_struct *tty,
 static int pty_resize(struct tty_struct *tty,  struct winsize *ws)
 {
 	struct pid *pgrp, *rpgrp;
-	unsigned long flags;
 	struct tty_struct *pty = tty->link;
 
 	/* For a PTY we need to lock the tty side */
@@ -286,17 +282,9 @@ static int pty_resize(struct tty_struct *tty,  struct winsize *ws)
 	if (!memcmp(ws, &tty->winsize, sizeof(*ws)))
 		goto done;
 
-	/* Get the PID values and reference them so we can
-	   avoid holding the tty ctrl lock while sending signals.
-	   We need to lock these individually however. */
-
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	pgrp = get_pid(tty->pgrp);
-	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
-
-	spin_lock_irqsave(&pty->ctrl_lock, flags);
-	rpgrp = get_pid(pty->pgrp);
-	spin_unlock_irqrestore(&pty->ctrl_lock, flags);
+	/* Signal the foreground process group of both ptys */
+	pgrp = tty_get_pgrp(tty);
+	rpgrp = tty_get_pgrp(pty);
 
 	if (pgrp)
 		kill_pgrp(pgrp, SIGWINCH, 1);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 272d977..c224488 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2331,18 +2331,14 @@ static int tiocgwinsz(struct tty_struct *tty, struct winsize __user *arg)
 int tty_do_resize(struct tty_struct *tty, struct winsize *ws)
 {
 	struct pid *pgrp;
-	unsigned long flags;
 
 	/* Lock the tty */
 	mutex_lock(&tty->winsize_mutex);
 	if (!memcmp(ws, &tty->winsize, sizeof(*ws)))
 		goto done;
-	/* Get the PID values and reference them so we can
-	   avoid holding the tty ctrl lock while sending signals */
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	pgrp = get_pid(tty->pgrp);
-	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 
+	/* Signal the foreground process group */
+	pgrp = tty_get_pgrp(tty);
 	if (pgrp)
 		kill_pgrp(pgrp, SIGWINCH, 1);
 	put_pid(pgrp);
-- 
2.1.1

[PATCH -next 06/10] tty: Remove !tty condition from __proc_set_tty()

[Peter Hurley]

The tty parameter to __proc_set_tty() cannot be NULL; all
call sites have already dereferenced tty.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c224488..e33edfa 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -506,19 +506,18 @@ void proc_clear_tty(struct task_struct *p)
 
 static void __proc_set_tty(struct tty_struct *tty)
 {
-	if (tty) {
-		unsigned long flags;
-		/* We should not have a session or pgrp to put here but.... */
-		spin_lock_irqsave(&tty->ctrl_lock, flags);
-		put_pid(tty->session);
-		put_pid(tty->pgrp);
-		tty->pgrp = get_pid(task_pgrp(current));
-		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
-		tty->session = get_pid(task_session(current));
-		if (current->signal->tty) {
-			printk(KERN_DEBUG "tty not NULL!!\n");
-			tty_kref_put(current->signal->tty);
-		}
+	unsigned long flags;
+
+	/* We should not have a session or pgrp to put here but.... */
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	put_pid(tty->session);
+	put_pid(tty->pgrp);
+	tty->pgrp = get_pid(task_pgrp(current));
+	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+	tty->session = get_pid(task_session(current));
+	if (current->signal->tty) {
+		printk(KERN_DEBUG "tty not NULL!!\n");
+		tty_kref_put(current->signal->tty);
 	}
 	put_pid(current->signal->tty_old_pgrp);
 	current->signal->tty = tty_kref_get(tty);
-- 
2.1.1

[PATCH -next 07/10] tty: Fix multiple races when setting the controlling terminal

[Peter Hurley]

Claim a read lock on the tasklist_lock while setting the controlling
terminal for the session leader. This fixes multiple races:
1. task_pgrp() and task_session() cannot be safely dereferenced, such
   as passing to get_pid(), without holding either rcu_read_lock() or
   tasklist_lock
2. setsid() unwisely allows any thread in the thread group to
   make the thread group leader the session leader; this makes the
   unlocked reads of ->signal->leader and signal->tty potentially
   unordered, stale or even have spurious values.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e33edfa..4226ae7 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -502,8 +502,15 @@ void proc_clear_tty(struct task_struct *p)
 	tty_kref_put(tty);
 }
 
-/* Called under the sighand lock */
-
+/**
+ * proc_set_tty -  set the controlling terminal
+ *
+ * Only callable by the session leader and only if it does not already have
+ * a controlling terminal.
+ *
+ * Caller must hold:  a readlock on tasklist_lock
+ *		      sighand lock
+ */
 static void __proc_set_tty(struct tty_struct *tty)
 {
 	unsigned long flags;
@@ -2158,6 +2165,7 @@ retry_open:
 
 	mutex_lock(&tty_mutex);
 	tty_lock(tty);
+	read_lock(&tasklist_lock);
 	spin_lock_irq(&current->sighand->siglock);
 	if (!noctty &&
 	    current->signal->leader &&
@@ -2165,6 +2173,7 @@ retry_open:
 	    tty->session == NULL)
 		__proc_set_tty(tty);
 	spin_unlock_irq(&current->sighand->siglock);
+	read_unlock(&tasklist_lock);
 	tty_unlock(tty);
 	mutex_unlock(&tty_mutex);
 	return 0;
@@ -2454,10 +2463,13 @@ static int fionbio(struct file *file, int __user *p)
 static int tiocsctty(struct tty_struct *tty, int arg)
 {
 	int ret = 0;
-	if (current->signal->leader && (task_session(current) == tty->session))
-		return ret;
 
 	mutex_lock(&tty_mutex);
+	read_lock(&tasklist_lock);
+
+	if (current->signal->leader && (task_session(current) == tty->session))
+		goto unlock;
+
 	/*
 	 * The process must be a session leader and
 	 * not have a controlling tty already.
@@ -2476,9 +2488,7 @@ static int tiocsctty(struct tty_struct *tty, int arg)
 			/*
 			 * Steal it away
 			 */
-			read_lock(&tasklist_lock);
 			session_clear_tty(tty->session);
-			read_unlock(&tasklist_lock);
 		} else {
 			ret = -EPERM;
 			goto unlock;
@@ -2486,6 +2496,7 @@ static int tiocsctty(struct tty_struct *tty, int arg)
 	}
 	proc_set_tty(tty);
 unlock:
+	read_unlock(&tasklist_lock);
 	mutex_unlock(&tty_mutex);
 	return ret;
 }
-- 
2.1.1

[PATCH -next 08/10] tty: Move session_of_pgrp() and make static

[Peter Hurley]

tiocspgrp() is the lone caller of session_of_pgrp(); relocate and
limit to file scope.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c   | 21 +++++++++++++++++++++
 include/linux/kernel.h |  3 ---
 kernel/exit.c          | 21 ---------------------
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 4226ae7..c15421df 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2522,6 +2522,27 @@ struct pid *tty_get_pgrp(struct tty_struct *tty)
 }
 EXPORT_SYMBOL_GPL(tty_get_pgrp);
 
+/*
+ * This checks not only the pgrp, but falls back on the pid if no
+ * satisfactory pgrp is found. I dunno - gdb doesn't work correctly
+ * without this...
+ *
+ * The caller must hold rcu lock or the tasklist lock.
+ */
+static struct pid *session_of_pgrp(struct pid *pgrp)
+{
+	struct task_struct *p;
+	struct pid *sid = NULL;
+
+	p = pid_task(pgrp, PIDTYPE_PGID);
+	if (p == NULL)
+		p = pid_task(pgrp, PIDTYPE_PID);
+	if (p != NULL)
+		sid = task_session(p);
+
+	return sid;
+}
+
 /**
  *	tiocgpgrp		-	get process group
  *	@tty: tty passed by user
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 95624be..70c1e12 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -414,9 +414,6 @@ extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
-struct pid;
-extern struct pid *session_of_pgrp(struct pid *pgrp);
-
 unsigned long int_sqrt(unsigned long);
 
 extern void bust_spinlocks(int yes);
diff --git a/kernel/exit.c b/kernel/exit.c
index 32c58f7..0973f4c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -214,27 +214,6 @@ repeat:
 }
 
 /*
- * This checks not only the pgrp, but falls back on the pid if no
- * satisfactory pgrp is found. I dunno - gdb doesn't work correctly
- * without this...
- *
- * The caller must hold rcu lock or the tasklist lock.
- */
-struct pid *session_of_pgrp(struct pid *pgrp)
-{
-	struct task_struct *p;
-	struct pid *sid = NULL;
-
-	p = pid_task(pgrp, PIDTYPE_PGID);
-	if (p == NULL)
-		p = pid_task(pgrp, PIDTYPE_PID);
-	if (p != NULL)
-		sid = task_session(p);
-
-	return sid;
-}
-
-/*
  * Determine if a process group is "orphaned", according to the POSIX
  * definition in 2.2.2.52.  Orphaned process groups are not to be affected
  * by terminal-generated stop signals.  Newly orphaned process groups are
-- 
2.1.1

[PATCH -next 09/10] tty: Serialize proc_set_tty() with tty_lock

[Peter Hurley]

Setting the controlling terminal for a session occurs with either
the first open of a non-pty master tty or with ioctl(TIOCSCTTY).
Since only the session leader can set the controlling terminal for
a session (and the session leader cannot change), it is not
necessary to prevent a process from attempting to set different
ttys as the controlling terminal concurrently.

So it's only necessary to prevent the same tty from becoming the
controlling terminal for different session leaders. The tty_lock()
is sufficient to prevent concurrent proc_set_tty() for the same
tty.

Remove the tty_mutex lock region; add tty_lock() to tiocsctty().

While this may appear to allow a race condition between opening
the controlling tty via tty_open_current_tty() and stealing the
controlling tty via ioctl(TIOCSCTTY, 1), that race condition already
existed. Even if the tty_mutex prevented stealing the controlling tty
while tty_open_current_tty() returned the original controlling tty,
it cannot prevent stealing the controlling tty before tty_open() returns.
Thus, tty_open() could already return a no-longer-controlling tty when
opening /dev/tty.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c15421df..7def510 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -508,7 +508,8 @@ void proc_clear_tty(struct task_struct *p)
  * Only callable by the session leader and only if it does not already have
  * a controlling terminal.
  *
- * Caller must hold:  a readlock on tasklist_lock
+ * Caller must hold:  tty_lock()
+ *		      a readlock on tasklist_lock
  *		      sighand lock
  */
 static void __proc_set_tty(struct tty_struct *tty)
@@ -2160,11 +2161,8 @@ retry_open:
 		goto retry_open;
 	}
 	clear_bit(TTY_HUPPED, &tty->flags);
-	tty_unlock(tty);
 
 
-	mutex_lock(&tty_mutex);
-	tty_lock(tty);
 	read_lock(&tasklist_lock);
 	spin_lock_irq(&current->sighand->siglock);
 	if (!noctty &&
@@ -2175,7 +2173,6 @@ retry_open:
 	spin_unlock_irq(&current->sighand->siglock);
 	read_unlock(&tasklist_lock);
 	tty_unlock(tty);
-	mutex_unlock(&tty_mutex);
 	return 0;
 err_unlock:
 	mutex_unlock(&tty_mutex);
@@ -2455,7 +2452,7 @@ static int fionbio(struct file *file, int __user *p)
  *	leader to set this tty as the controlling tty for the session.
  *
  *	Locking:
- *		Takes tty_mutex() to protect tty instance
+ *		Takes tty_lock() to serialize proc_set_tty() for this tty
  *		Takes tasklist_lock internally to walk sessions
  *		Takes ->siglock() when updating signal->tty
  */
@@ -2464,7 +2461,7 @@ static int tiocsctty(struct tty_struct *tty, int arg)
 {
 	int ret = 0;
 
-	mutex_lock(&tty_mutex);
+	tty_lock(tty);
 	read_lock(&tasklist_lock);
 
 	if (current->signal->leader && (task_session(current) == tty->session))
@@ -2497,7 +2494,7 @@ static int tiocsctty(struct tty_struct *tty, int arg)
 	proc_set_tty(tty);
 unlock:
 	read_unlock(&tasklist_lock);
-	mutex_unlock(&tty_mutex);
+	tty_unlock(tty);
 	return ret;
 }
 
-- 
2.1.1

[PATCH -next 10/10] tty: Update code comment in __proc_set_tty()

[Peter Hurley]

The session and foreground process group pid references will be
non-NULL if tiocsctty() is stealing the controlling tty from another
session (ie., arg == 1 in tiocsctty()).

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 7def510..e18491a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -516,8 +516,11 @@ static void __proc_set_tty(struct tty_struct *tty)
 {
 	unsigned long flags;
 
-	/* We should not have a session or pgrp to put here but.... */
 	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	/*
+	 * The session and fg pgrp references will be non-NULL if
+	 * tiocsctty() is stealing the controlling tty
+	 */
 	put_pid(tty->session);
 	put_pid(tty->pgrp);
 	tty->pgrp = get_pid(task_pgrp(current));
-- 
2.1.1

Re: [PATCH -next 04/10] uml: Fix unsafe pid reference to foreground process group

[Richard Weinberger]

Am 16.10.2014 um 20:59 schrieb Peter Hurley:
> Although the tty core maintains a pid reference for the foreground
> process group, if the foreground process group is changed that
> pid reference is dropped. Thus, the pid reference used for signalling
> could become stale.
> 
> Safely obtain a pid reference to the foreground process group and
> release the reference after signalling is complete.
> 
> cc: Jeff Dike <jdike@addtoit.com>
> cc: Richard Weinberger <richard@nod.at>
> cc: user-mode-linux-devel@lists.sourceforge.net
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

Acked-by: Richard Weinberger <richard@nod.at>

> ---
>  arch/um/drivers/line.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
> index 8035145..6208702 100644
> --- a/arch/um/drivers/line.c
> +++ b/arch/um/drivers/line.c
> @@ -632,6 +632,7 @@ static irqreturn_t winch_interrupt(int irq, void *data)
>  	int fd = winch->fd;
>  	int err;
>  	char c;
> +	struct pid *pgrp;
>  
>  	if (fd != -1) {
>  		err = generic_read(fd, &c, NULL);
> @@ -657,7 +658,10 @@ static irqreturn_t winch_interrupt(int irq, void *data)
>  		if (line != NULL) {
>  			chan_window_size(line, &tty->winsize.ws_row,
>  					 &tty->winsize.ws_col);
> -			kill_pgrp(tty->pgrp, SIGWINCH, 1);
> +			pgrp = tty_get_pgrp(tty);
> +			if (pgrp)
> +				kill_pgrp(pgrp, SIGWINCH, 1);
> +			put_pid(pgrp);
>  		}
>  		tty_kref_put(tty);
>  	}
> 

Re: [PATCH -next 00/10] Fixes to controlling tty handling

[One Thousand Gnomes]

On Thu, 16 Oct 2014 14:59:40 -0400
Peter Hurley <peter@hurleysoftware.com> wrote:

> Hi Greg,
> 
> This patch series:
> 1. removes stale code from the controlling tty handling functions
> 2. relocates the ctty functions to eliminate forward declarations
> 3. fixes several unsafe races when setting the controlling tty
> 4. eliminates holding tty_mutex as a necessary condition of
>    setting the controlling terminal
> 
> #4 is part of an overall effort to reduce the tty_mutex footprint.
> 
> Unfortunately, this series does not fix two other race conditions:
> 1. disassociate_ctty()/no_tty() does not teardown the tty<->process
> associations atomically wrt job control, so it is possible to
> observe spurious error conditions from job control (tty_check_change()
> and job_control()). I'm looking into inverting the lock order of
> tty->ctrl_lock and tsk->sighand->siglock() to see if holding ctrl_lock
> is a suitable solution for atomic teardown. Especially now that
> ctrl_lock is not used for flow control anymore :)
> 2. task_pgrp() and task_session() are used unsafely. These fixes
> will be clearer after #1 is fixed.


Reviewed-by: Alan Cox <alan@linux.intel.com>

I can't prove entirely to my satisfaction that the claim in #9 is true in
the presence of simultaenous hangups opens and setsid but the locking
appears to be correct for the cases I was trying to figure out anyway.

Makes my head hurt just reviewing bits of this so thanks for doing all
this work !