[PATCH 0/2] tty changes for the locking warnings.

[PATCH 0/2] tty changes for the locking warnings.

[Alan Cox]

Fix the tty locking warnings. This doesn't include the patches to the
lock markup as I'm not sure which are the final valid ones there.

---

Alan Cox (2):
      tty: fix ldisc lock inversion trace
      pty: Fix lock inversion


 drivers/tty/pty.c       |    2 --
 drivers/tty/tty_ldisc.c |   41 +++++++++++++++++++++++++----------------
 2 files changed, 25 insertions(+), 18 deletions(-)

-- 

[PATCH 1/2] pty: Fix lock inversion

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

The ptmx_open path takes the tty and devpts locks in the wrong order
because tty_init_dev locks and returns a locked tty. As far as I can tell
this is actually safe anyway because the tty being returned is new so
nobody can get a reference to lock it at this point.

However we don't even need the devpts lock at this point, it's only held as
a byproduct of the way the locks were pushed down.

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

 drivers/tty/pty.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)


diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 59af394..65c7c62 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -633,7 +633,6 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 	mutex_unlock(&devpts_mutex);
 
 	mutex_lock(&tty_mutex);
-	mutex_lock(&devpts_mutex);
 	tty = tty_init_dev(ptm_driver, index);
 
 	if (IS_ERR(tty)) {
@@ -643,7 +642,6 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 
 	/* The tty returned here is locked so we can safely
 	   drop the mutex */
-	mutex_unlock(&devpts_mutex);
 	mutex_unlock(&tty_mutex);
 
 	set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */

[PATCH 2/2] tty: fix ldisc lock inversion trace

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

This is caused by tty_release using tty_lock_pair to lock both
sides of the pty/tty pair, and then tty_ldisc_release dropping
and relocking one side only. We can drop both fine, so drop both
to avoid any lock ordering concerns.

Rework the release path to fix the new locking model.

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

 drivers/tty/tty_ldisc.c |   41 +++++++++++++++++++++++++----------------
 1 files changed, 25 insertions(+), 16 deletions(-)


diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 173a900..ba8be39 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -894,6 +894,23 @@ int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty)
 	tty_ldisc_enable(tty);
 	return 0;
 }
+
+static void tty_ldisc_kill(struct tty_struct *tty)
+{
+	mutex_lock(&tty->ldisc_mutex);
+	/*
+	 * Now kill off the ldisc
+	 */
+	tty_ldisc_close(tty, tty->ldisc);
+	tty_ldisc_put(tty->ldisc);
+	/* Force an oops if we mess this up */
+	tty->ldisc = NULL;
+
+	/* Ensure the next open requests the N_TTY ldisc */
+	tty_set_termios_ldisc(tty, N_TTY);
+	mutex_unlock(&tty->ldisc_mutex);
+}
+
 /**
  *	tty_ldisc_release		-	release line discipline
  *	@tty: tty being shut down
@@ -912,27 +929,19 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
 	 * race with the set_ldisc code path.
 	 */
 
-	tty_unlock(tty);
+	tty_unlock_pair(tty, o_tty);
 	tty_ldisc_halt(tty);
 	tty_ldisc_flush_works(tty);
-	tty_lock(tty);
-
-	mutex_lock(&tty->ldisc_mutex);
-	/*
-	 * Now kill off the ldisc
-	 */
-	tty_ldisc_close(tty, tty->ldisc);
-	tty_ldisc_put(tty->ldisc);
-	/* Force an oops if we mess this up */
-	tty->ldisc = NULL;
+	if (o_tty) {
+		tty_ldisc_halt(o_tty);
+		tty_ldisc_flush_works(o_tty);
+	}
+	tty_lock_pair(tty, o_tty);
 
-	/* Ensure the next open requests the N_TTY ldisc */
-	tty_set_termios_ldisc(tty, N_TTY);
-	mutex_unlock(&tty->ldisc_mutex);
 
-	/* This will need doing differently if we need to lock */
+	tty_ldisc_kill(tty);
 	if (o_tty)
-		tty_ldisc_release(o_tty, NULL);
+		tty_ldisc_kill(o_tty);
 
 	/* And the memory resources remaining (buffers, termios) will be
 	   disposed of when the kref hits zero */

Re: [PATCH 0/2] tty changes for the locking warnings.

[Greg KH]

On Mon, May 28, 2012 at 10:48:37AM +0100, Alan Cox wrote:
> Fix the tty locking warnings. This doesn't include the patches to the
> lock markup as I'm not sure which are the final valid ones there.

So this should take care of the issues people have reported?  Or are
Peter's patches still needed?

thanks,

greg k-h

Re: [PATCH 0/2] tty changes for the locking warnings.

[Alan Cox]

On Mon, 28 May 2012 20:29:06 +0900
Greg KH <greg@kroah.com> wrote:

> On Mon, May 28, 2012 at 10:48:37AM +0100, Alan Cox wrote:
> > Fix the tty locking warnings. This doesn't include the patches to the
> > lock markup as I'm not sure which are the final valid ones there.
> 
> So this should take care of the issues people have reported?  Or are
> Peter's patches still needed?


These fix all the known actual locking issues. I'm not clear if one of
the other various mark up patches are also needed to babysit the lockdep
analyser

Re: [PATCH 0/2] tty changes for the locking warnings.

[Greg KH]

On Mon, May 28, 2012 at 01:13:58PM +0100, Alan Cox wrote:
> On Mon, 28 May 2012 20:29:06 +0900
> Greg KH <greg@kroah.com> wrote:
> 
> > On Mon, May 28, 2012 at 10:48:37AM +0100, Alan Cox wrote:
> > > Fix the tty locking warnings. This doesn't include the patches to the
> > > lock markup as I'm not sure which are the final valid ones there.
> > 
> > So this should take care of the issues people have reported?  Or are
> > Peter's patches still needed?
> 
> 
> These fix all the known actual locking issues. I'm not clear if one of
> the other various mark up patches are also needed to babysit the lockdep
> analyser

Ok, great, thanks for fixing this up.

But, it turns out I don't think I'll have internet access at all for the
next 4 days, due to travel arrangements, so can you send these directly
to Linus before 3.5-rc1 is out so they get in and we don't see a zillion
people complaining about it?

Feel free to add:
	Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

to them.

thanks,

greg k-h