[PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)

[PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)

[Ming Lei]

Commit d29f3ef39be4eec0362b985305fc526d9be318cf(tty_lock:
Localise the lock) introduces tty_lock_pair, in which
may cause lockdep warning because two locks with same lock
class are to be acquired one after another.

This patch uses mutex_lock_nested annotation to avoid
the warning as suggested by Peter.

Cc: Alan Cox <alan@linux.intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/tty/tty_mutex.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 69adc80..fecf592 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -10,7 +10,8 @@
  * Getting the big tty mutex.
  */
 
-void __lockfunc tty_lock(struct tty_struct *tty)
+static void __lockfunc tty_lock_nested(struct tty_struct *tty,
+		int subclass)
 {
 	if (tty->magic != TTY_MAGIC) {
 		printk(KERN_ERR "L Bad %p\n", tty);
@@ -18,7 +19,12 @@ void __lockfunc tty_lock(struct tty_struct *tty)
 		return;
 	}
 	tty_kref_get(tty);
-	mutex_lock(&tty->legacy_mutex);
+	mutex_lock_nested(&tty->legacy_mutex, subclass);
+}
+
+void __lockfunc tty_lock(struct tty_struct *tty)
+{
+	tty_lock_nested(tty, 0);
 }
 EXPORT_SYMBOL(tty_lock);
 
@@ -43,11 +49,14 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty,
 {
 	if (tty < tty2) {
 		tty_lock(tty);
-		tty_lock(tty2);
+		tty_lock_nested(tty2, SINGLE_DEPTH_NESTING);
 	} else {
-		if (tty2 && tty2 != tty)
+		int nested = 0;
+		if (tty2 && tty2 != tty) {
 			tty_lock(tty2);
-		tty_lock(tty);
+			nested = SINGLE_DEPTH_NESTING;
+		}
+		tty_lock_nested(tty, nested);
 	}
 }
 EXPORT_SYMBOL(tty_lock_pair);
-- 
1.7.9.5

Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)

[Ming Lei]

Hi,

On Tue, May 22, 2012 at 9:58 AM, Ming Lei <ming.lei@canonical.com> wrote:
> Commit d29f3ef39be4eec0362b985305fc526d9be318cf(tty_lock:
> Localise the lock) introduces tty_lock_pair, in which
> may cause lockdep warning because two locks with same lock
> class are to be acquired one after another.
>
> This patch uses mutex_lock_nested annotation to avoid
> the warning as suggested by Peter.

Sorry, please ignore the patch because it misses the change on
tty_unlock_pair, and the correct one should be [1].

Even though the patch is applied, there is still one related problem about
mixing tty_lock_pair with tty_unlock and tty_lock. If tty locks are
held by calling
tty_lock_pair, then deadlock warning between legacy_mutex/1 and legacy_mutex
may be triggered if tty_unlock(tty) and tty_lock(tty) are called later
when tty < tty2,
see tty_ldisc_release() in tty_release().


Thanks,
--
Ming Lei


[1],
---
 drivers/tty/tty_mutex.c |   28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 69adc80..c7f4523 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -10,7 +10,8 @@
  * Getting the big tty mutex.
  */

-void __lockfunc tty_lock(struct tty_struct *tty)
+static void __lockfunc tty_lock_nested(struct tty_struct *tty,
+		int subclass)
 {
 	if (tty->magic != TTY_MAGIC) {
 		printk(KERN_ERR "L Bad %p\n", tty);
@@ -18,7 +19,12 @@ void __lockfunc tty_lock(struct tty_struct *tty)
 		return;
 	}
 	tty_kref_get(tty);
-	mutex_lock(&tty->legacy_mutex);
+	mutex_lock_nested(&tty->legacy_mutex, subclass);
+}
+
+void __lockfunc tty_lock(struct tty_struct *tty)
+{
+	tty_lock_nested(tty, 0);
 }
 EXPORT_SYMBOL(tty_lock);

@@ -43,11 +49,14 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty,
 {
 	if (tty < tty2) {
 		tty_lock(tty);
-		tty_lock(tty2);
+		tty_lock_nested(tty2, SINGLE_DEPTH_NESTING);
 	} else {
-		if (tty2 && tty2 != tty)
+		int nested = 0;
+		if (tty2 && tty2 != tty) {
 			tty_lock(tty2);
-		tty_lock(tty);
+			nested = SINGLE_DEPTH_NESTING;
+		}
+		tty_lock_nested(tty, nested);
 	}
 }
 EXPORT_SYMBOL(tty_lock_pair);
@@ -55,8 +64,13 @@ EXPORT_SYMBOL(tty_lock_pair);
 void __lockfunc tty_unlock_pair(struct tty_struct *tty,
 						struct tty_struct *tty2)
 {
-	tty_unlock(tty);
-	if (tty2 && tty2 != tty)
+	if (tty < tty2) {
 		tty_unlock(tty2);
+		tty_unlock(tty);
+	} else {
+		tty_unlock(tty);
+		if (tty2 && tty2 != tty)
+			tty_unlock(tty2);
+	}
 }
 EXPORT_SYMBOL(tty_unlock_pair);
-- 
1.7.9.5

Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)

[Peter Zijlstra]

On Wed, 2012-05-23 at 14:01 +0800, Ming Lei wrote:

> Even though the patch is applied, there is still one related problem about
> mixing tty_lock_pair with tty_unlock and tty_lock. If tty locks are
> held by calling
> tty_lock_pair, then deadlock warning between legacy_mutex/1 and legacy_mutex
> may be triggered if tty_unlock(tty) and tty_lock(tty) are called later
> when tty < tty2,
> see tty_ldisc_release() in tty_release().

This just gives me a head-ache instead of explaining anything.

Having looked at the source I still don't see how it could possibly
work,.. So the problem with tty_release() -> tty_ldisc_release() is that
tty_ldisc_release() does an unlock/lock of tty.

However your tty_lock_pair() can still result in tty being subclass 1,
see your else branch, nested case.

That said, how is this not a real deadlock? If you rely on tty pointer
ordering to avoid deadlocks, you always need to lock them in the same
order. The unlock+lock in ldisc_release violates that.

If we don't rely on the order, then why bother with the _pair()
primitive?

Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)

[Peter Zijlstra]

On Fri, 2012-05-25 at 15:28 +0200, Peter Zijlstra wrote:
> On Wed, 2012-05-23 at 14:01 +0800, Ming Lei wrote:
> 
> > Even though the patch is applied, there is still one related problem about
> > mixing tty_lock_pair with tty_unlock and tty_lock. If tty locks are
> > held by calling
> > tty_lock_pair, then deadlock warning between legacy_mutex/1 and legacy_mutex
> > may be triggered if tty_unlock(tty) and tty_lock(tty) are called later
> > when tty < tty2,
> > see tty_ldisc_release() in tty_release().
> 
> This just gives me a head-ache instead of explaining anything.
> 
> Having looked at the source I still don't see how it could possibly
> work,.. So the problem with tty_release() -> tty_ldisc_release() is that
> tty_ldisc_release() does an unlock/lock of tty.
> 
> However your tty_lock_pair() can still result in tty being subclass 1,
> see your else branch, nested case.
> 
> That said, how is this not a real deadlock? If you rely on tty pointer
> ordering to avoid deadlocks, you always need to lock them in the same
> order. The unlock+lock in ldisc_release violates that.
> 
> If we don't rely on the order, then why bother with the _pair()
> primitive?

A git grep reveals tty_release() is the only user of tty_lock_pair() and
while we hold tty_mutex over the tty_lock_pair() its not held over
ldisc_release().

Thus afaict we can create the following deadlock:


	cpu-A			cpu-B

lock tty_mutex
  lock tty
  lock o_tty
unlock tty_mutex

  unlock tty
			lock tty_mutex
			  lock tty
			  lock o_tty -> block on A
  lock tty -> block on B



Also, what is that plain call to schedule() doing in tty_release()?!

Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)

[Alan Cox]

> Having looked at the source I still don't see how it could possibly
> work,.. So the problem with tty_release() -> tty_ldisc_release() is
> that tty_ldisc_release() does an unlock/lock of tty.

Yes it should do the pair, see the patch I posted restructing it, and
the second one restructing it right.

> However your tty_lock_pair() can still result in tty being subclass 1,
> see your else branch, nested case.
> 
> That said, how is this not a real deadlock? If you rely on tty pointer
> ordering to avoid deadlocks, you always need to lock them in the same
> order. The unlock+lock in ldisc_release violates that.

Which was a bug.

Alan

Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)

[Peter Zijlstra]

On Fri, 2012-05-25 at 14:47 +0100, Alan Cox wrote:
> > Having looked at the source I still don't see how it could possibly
> > work,.. So the problem with tty_release() -> tty_ldisc_release() is
> > that tty_ldisc_release() does an unlock/lock of tty.
> 
> Yes it should do the pair, see the patch I posted restructing it, and
> the second one restructing it right.

http://marc.info/?l=linux-kernel&m=133794355529930

That one? To what tree does one apply that? Because the tty_lock_pair()
in Linus is still missing a lockdep annotation afaict.

Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)

[Alan Cox]

On Fri, 25 May 2012 15:52:02 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, 2012-05-25 at 14:47 +0100, Alan Cox wrote:
> > > Having looked at the source I still don't see how it could possibly
> > > work,.. So the problem with tty_release() -> tty_ldisc_release() is
> > > that tty_ldisc_release() does an unlock/lock of tty.
> > 
> > Yes it should do the pair, see the patch I posted restructing it, and
> > the second one restructing it right.
> 
> http://marc.info/?l=linux-kernel&m=133794355529930
> 
> That one? To what tree does one apply that? Because the tty_lock_pair()
> in Linus is still missing a lockdep annotation afaict.

It applies on top of the other patches being tested in the thread on the
lockdep warning.

Alan

Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)

[Peter Zijlstra]

On Fri, 2012-05-25 at 15:01 +0100, Alan Cox wrote:
> 
> It applies on top of the other patches being tested in the thread on the
> lockdep warning.

You're really going to make me hunt and peck patches from lkml?

/me looses interest and gets on with reducing his inbox.