All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Hurley <peter@hurleysoftware.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>,
	One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	Peter Hurley <peter@hurleysoftware.com>
Subject: [PATCH -next v2 07/26] tty: Re-open /dev/tty without tty_mutex
Date: Wed,  5 Nov 2014 12:12:50 -0500	[thread overview]
Message-ID: <1415207589-15967-8-git-send-email-peter@hurleysoftware.com> (raw)
In-Reply-To: <1415207589-15967-1-git-send-email-peter@hurleysoftware.com>

Opening /dev/tty (ie., the controlling tty for the current task)
is always a re-open of the underlying tty. Because holding the
tty_lock is sufficient for safely re-opening a tty, and because
having a tty kref is sufficient for safely acquiring the tty_lock [1],
tty_open_current_tty() does not require holding tty_mutex.

Repurpose tty_open_current_tty() to perform the re-open itself and
refactor tty_open().

[1] Analysis of safely re-opening the current tty w/o tty_mutex

get_current_tty() gets a tty kref from the already kref'ed tty value of
current->signal->tty while holding the sighand lock for the current
task. This guarantees that the tty pointer returned from
get_current_tty() points to a tty which remains referenceable
while holding the kref.

Although release_tty() may run concurrently, and thus the driver
reference may be removed, release_one_tty() cannot have run, and
won't while holding the tty kref.

This, in turn, guarantees the tty_lock() can safely be acquired
(since tty->magic and tty->legacy_mutex are still a valid dereferences).
The tty_lock() also gets a tty kref to prevent the tty_unlock() from
dereferencing a released tty. Thus, the kref returned from
get_current_tty() can be released.

Lastly, the first operation of tty_reopen() is to check the tty count.
If non-zero, this ensures release_tty() is not running concurrently,
and the driver references have not been removed.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 53 ++++++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 97445c2..fb4583c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1944,20 +1944,20 @@ int tty_release(struct inode *inode, struct file *filp)
 }
 
 /**
- *	tty_open_current_tty - get tty of current task for open
+ *	tty_open_current_tty - get locked tty of current task
  *	@device: device number
  *	@filp: file pointer to tty
- *	@return: tty of the current task iff @device is /dev/tty
+ *	@return: locked tty of the current task iff @device is /dev/tty
+ *
+ *	Performs a re-open of the current task's controlling tty.
  *
  *	We cannot return driver and index like for the other nodes because
  *	devpts will not work then. It expects inodes to be from devpts FS.
- *
- *	We need to move to returning a refcounted object from all the lookup
- *	paths including this one.
  */
 static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
 {
 	struct tty_struct *tty;
+	int retval;
 
 	if (device != MKDEV(TTYAUX_MAJOR, 0))
 		return NULL;
@@ -1968,9 +1968,14 @@ static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
 
 	filp->f_flags |= O_NONBLOCK; /* Don't let /dev/tty block */
 	/* noctty = 1; */
-	tty_kref_put(tty);
-	/* FIXME: we put a reference and return a TTY! */
-	/* This is only safe because the caller holds tty_mutex */
+	tty_lock(tty);
+	tty_kref_put(tty);	/* safe to drop the kref now */
+
+	retval = tty_reopen(tty);
+	if (retval < 0) {
+		tty_unlock(tty);
+		tty = ERR_PTR(retval);
+	}
 	return tty;
 }
 
@@ -2068,13 +2073,9 @@ retry_open:
 	index  = -1;
 	retval = 0;
 
-	mutex_lock(&tty_mutex);
-	/* This is protected by the tty_mutex */
 	tty = tty_open_current_tty(device, filp);
-	if (IS_ERR(tty)) {
-		retval = PTR_ERR(tty);
-		goto err_unlock;
-	} else if (!tty) {
+	if (!tty) {
+		mutex_lock(&tty_mutex);
 		driver = tty_lookup_driver(device, filp, &noctty, &index);
 		if (IS_ERR(driver)) {
 			retval = PTR_ERR(driver);
@@ -2087,21 +2088,21 @@ retry_open:
 			retval = PTR_ERR(tty);
 			goto err_unlock;
 		}
-	}
 
-	if (tty) {
-		tty_lock(tty);
-		retval = tty_reopen(tty);
-		if (retval < 0) {
-			tty_unlock(tty);
-			tty = ERR_PTR(retval);
-		}
-	} else	/* Returns with the tty_lock held for now */
-		tty = tty_init_dev(driver, index);
+		if (tty) {
+			tty_lock(tty);
+			retval = tty_reopen(tty);
+			if (retval < 0) {
+				tty_unlock(tty);
+				tty = ERR_PTR(retval);
+			}
+		} else	/* Returns with the tty_lock held for now */
+			tty = tty_init_dev(driver, index);
 
-	mutex_unlock(&tty_mutex);
-	if (driver)
+		mutex_unlock(&tty_mutex);
 		tty_driver_kref_put(driver);
+	}
+
 	if (IS_ERR(tty)) {
 		retval = PTR_ERR(tty);
 		goto err_file;
-- 
2.1.3

  parent reply	other threads:[~2014-11-05 17:12 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-16 20:24 [PATCH -next 00/27] tty locking changes Peter Hurley
2014-10-16 20:24 ` [PATCH -next 01/27] tty: Don't hold tty_lock for ldisc release Peter Hurley
2014-10-16 20:25 ` [PATCH -next 02/27] tty: Invert tty_lock/ldisc_sem lock order Peter Hurley
2014-10-16 20:25 ` [PATCH -next 03/27] tty: Remove TTY_HUPPING Peter Hurley
2014-10-16 20:25 ` [PATCH -next 04/27] tty: Clarify re-open behavior of master ptys Peter Hurley
2014-10-16 20:25 ` [PATCH -next 05/27] tty: Check tty->count instead of TTY_CLOSING in tty_reopen() Peter Hurley
2014-10-16 20:25 ` [PATCH -next 06/27] pty: Always return -EIO if slave BSD pty opened first Peter Hurley
2014-10-16 20:25 ` [PATCH -next 07/27] tty: Re-open /dev/tty without tty_mutex Peter Hurley
2014-10-16 20:25 ` [PATCH -next 08/27] tty: Drop tty_mutex before tty reopen Peter Hurley
2014-10-16 20:25 ` [PATCH -next 09/27] tty: Remove TTY_CLOSING Peter Hurley
2014-10-16 20:25 ` [PATCH -next 10/27] tty: Don't take tty_mutex for tty count changes Peter Hurley
2014-10-16 20:25 ` [PATCH -next 11/27] tty: Don't release tty locks for wait queue sanity check Peter Hurley
2014-10-22 15:29   ` One Thousand Gnomes
2014-10-22 17:34     ` Peter Hurley
2014-10-23 11:30       ` One Thousand Gnomes
2014-10-16 20:25 ` [PATCH -next 12/27] tty: Document check_tty_count() requires tty_lock held Peter Hurley
2014-10-16 20:25 ` [PATCH -next 13/27] tty: Simplify pty pair teardown logic Peter Hurley
2014-10-16 20:25 ` [PATCH -next 14/27] tty: Fold pty pair handling into tty_flush_works() Peter Hurley
2014-10-16 20:25 ` [PATCH -next 15/27] tty: Simplify tty_ldisc_release() interface Peter Hurley
2014-10-16 20:25 ` [PATCH -next 16/27] tty: Simplify tty_release_checks() interface Peter Hurley
2014-10-16 20:25 ` [PATCH -next 17/27] tty: Simplify tty_release() state checks Peter Hurley
2014-10-16 20:25 ` [PATCH -next 18/27] tty: Change tty lock order to master->slave Peter Hurley
2014-10-16 20:25 ` [PATCH -next 19/27] tty: Remove tty_unhangup() declaration Peter Hurley
2014-10-16 20:25 ` [PATCH -next 20/27] tty: Refactor __tty_hangup to enable lockdep annotation Peter Hurley
2014-10-27 22:13   ` Peter Hurley
2014-10-16 20:25 ` [PATCH -next 21/27] pty: Don't drop pty master tty lock to hangup slave Peter Hurley
2014-10-16 20:25 ` [PATCH -next 22/27] tty: Document hangup call tree Peter Hurley
2014-10-16 20:25 ` [PATCH -next 23/27] pty, n_tty: Simplify input processing on final close Peter Hurley
2014-10-16 20:25 ` [PATCH -next 24/27] tty: Prefix tty_ldisc_{lock,lock_nested,unlock} functions Peter Hurley
2014-10-16 20:25 ` [PATCH -next 25/27] tty: Fix hung task on pty hangup Peter Hurley
2014-10-16 20:25 ` [PATCH -next 26/27] tty: Fix timeout on pty set ldisc Peter Hurley
2014-10-16 20:25 ` [PATCH -next 27/27] tty: Flush ldisc buffer atomically with tty flip buffers Peter Hurley
2014-10-22 15:31 ` [PATCH -next 00/27] tty locking changes One Thousand Gnomes
2014-11-05 17:12 ` [PATCH -next v2 00/26] " Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 01/26] tty: Don't hold tty_lock for ldisc release Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 02/26] tty: Invert tty_lock/ldisc_sem lock order Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 03/26] tty: Remove TTY_HUPPING Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 04/26] tty: Clarify re-open behavior of master ptys Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 05/26] tty: Check tty->count instead of TTY_CLOSING in tty_reopen() Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 06/26] pty: Always return -EIO if slave BSD pty opened first Peter Hurley
2014-11-05 17:12   ` Peter Hurley [this message]
2014-11-05 17:12   ` [PATCH -next v2 08/26] tty: Drop tty_mutex before tty reopen Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 09/26] tty: Remove TTY_CLOSING Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 10/26] tty: Don't take tty_mutex for tty count changes Peter Hurley
2014-11-06  2:33     ` Greg Kroah-Hartman
2014-11-06  2:39       ` Peter Hurley
2014-11-06  2:50         ` Peter Hurley
2014-11-06  3:46           ` Greg Kroah-Hartman
2014-11-05 17:12   ` [PATCH -next v2 11/26] tty: Don't release tty locks for wait queue sanity check Peter Hurley
2014-11-06  2:40     ` Greg Kroah-Hartman
2014-11-05 17:12   ` [PATCH -next v2 12/26] tty: Document check_tty_count() requires tty_lock held Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 13/26] tty: Simplify pty pair teardown logic Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 14/26] tty: Fold pty pair handling into tty_flush_works() Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 15/26] tty: Simplify tty_ldisc_release() interface Peter Hurley
2014-11-05 17:12   ` [PATCH -next v2 16/26] tty: Simplify tty_release_checks() interface Peter Hurley
2014-11-05 17:13   ` [PATCH -next v2 17/26] tty: Simplify tty_release() state checks Peter Hurley
2014-11-05 17:13   ` [PATCH -next v2 18/26] tty: Change tty lock order to master->slave Peter Hurley
2014-11-05 17:13   ` [PATCH -next v2 19/26] tty: Preset lock subclass for nested tty locks Peter Hurley
2014-11-05 17:13   ` [PATCH -next v2 20/26] tty: Remove tty_unhangup() declaration Peter Hurley
2014-11-05 17:13   ` [PATCH -next v2 21/26] pty: Don't drop pty master tty lock to hangup slave Peter Hurley
2014-11-11 15:49     ` One Thousand Gnomes
2014-11-05 17:13   ` [PATCH -next v2 22/26] pty, n_tty: Simplify input processing on final close Peter Hurley
2014-11-05 17:13   ` [PATCH -next v2 23/26] tty: Prefix tty_ldisc_{lock,lock_nested,unlock} functions Peter Hurley
2014-11-05 17:13   ` [PATCH -next v2 24/26] tty: Fix hung task on pty hangup Peter Hurley
2014-11-05 17:13   ` [PATCH -next v2 25/26] tty: Fix timeout on pty set ldisc Peter Hurley
2014-11-05 17:13   ` [PATCH -next v2 26/26] tty: Flush ldisc buffer atomically with tty flip buffers Peter Hurley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1415207589-15967-8-git-send-email-peter@hurleysoftware.com \
    --to=peter@hurleysoftware.com \
    --cc=gnomes@lxorguk.ukuu.org.uk \
    --cc=gregkh@linuxfoundation.org \
    --cc=jslaby@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.