[PATCH] tty: annotate tty_lock() for lockdep

[PATCH] tty: annotate tty_lock() for lockdep

[Jiri Kosina]

=============================================
[ INFO: possible recursive locking detected ]
3.4.0-08218-gb48b2c3 #7 Not tainted
---------------------------------------------
blogd/279 is trying to acquire lock:
 (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff8150f51e>] tty_lock+0x3e/0x90

but task is already holding lock:
 (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff8150f51e>] tty_lock+0x3e/0x90

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&tty->legacy_mutex);
  lock(&tty->legacy_mutex);

 *** DEADLOCK ***

[ .. stacktraces removed .. ]

This is a false positive stemming from the fact that lockdep doesn't
understand that proper lock ordering is already achieved in tty_lock_pair()
by ordering by struct tty address.

Introduce tty_lock_nested() to be able to express this explicit
ordering to lockdep and teach tty_lock_pair() to use it.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/tty/tty_mutex.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 69adc80..90d43ad 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -10,7 +10,7 @@
  * Getting the big tty mutex.
  */
 
-void __lockfunc tty_lock(struct tty_struct *tty)
+void tty_lock_common(struct tty_struct *tty)
 {
 	if (tty->magic != TTY_MAGIC) {
 		printk(KERN_ERR "L Bad %p\n", tty);
@@ -18,10 +18,22 @@ void __lockfunc tty_lock(struct tty_struct *tty)
 		return;
 	}
 	tty_kref_get(tty);
+}
+
+void __lockfunc tty_lock(struct tty_struct *tty)
+{
+	tty_lock_common(tty);
 	mutex_lock(&tty->legacy_mutex);
 }
 EXPORT_SYMBOL(tty_lock);
 
+void __lockfunc tty_lock_nested(struct tty_struct *tty,
+					unsigned int subclass)
+{
+	tty_lock_common(tty);
+	mutex_lock_nested(&tty->legacy_mutex, subclass);
+}
+
 void __lockfunc tty_unlock(struct tty_struct *tty)
 {
 	if (tty->magic != TTY_MAGIC) {
@@ -42,12 +54,12 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty,
 					struct tty_struct *tty2)
 {
 	if (tty < tty2) {
-		tty_lock(tty);
-		tty_lock(tty2);
+		tty_lock_nested(tty, 1);
+		tty_lock_nested(tty2, 2);
 	} else {
 		if (tty2 && tty2 != tty)
-			tty_lock(tty2);
-		tty_lock(tty);
+			tty_lock_nested(tty2, 2);
+		tty_lock_nested(tty, 1);
 	}
 }
 EXPORT_SYMBOL(tty_lock_pair);

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] tty: annotate tty_lock() for lockdep

[Srikar Dronamraju]

Hi Jiri, 



> =============================================
> [ INFO: possible recursive locking detected ]
> 3.4.0-08218-gb48b2c3 #7 Not tainted
> ---------------------------------------------
> blogd/279 is trying to acquire lock:
>  (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff8150f51e>] tty_lock+0x3e/0x90
> 
> but task is already holding lock:
>  (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff8150f51e>] tty_lock+0x3e/0x90
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(&tty->legacy_mutex);
>   lock(&tty->legacy_mutex);
> 
>  *** DEADLOCK ***
> 

I saw a similar problem and applied your patch, but that doesnt seem to
help me.

INFO: possible recursive locking detected ]
3.4.0-uprobes-debug+ #86 Not tainted
---------------------------------------------
plymouthd/710 is trying to acquire lock:
 (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff813b5054>] tty_lock+0x6c/0x70

but task is already holding lock:
 (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff813b5054>] tty_lock+0x6c/0x70

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&tty->legacy_mutex);
  lock(&tty->legacy_mutex);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

2 locks held by plymouthd/710:
 #0:  (tty_mutex){+.+.+.}, at: [<ffffffff8123f452>] tty_release+0x25a/0x536
 #1:  (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff813b5054>]
tty_lock+0x6c/0x70

stack backtrace:
Pid: 710, comm: plymouthd Not tainted 3.4.0-uprobes-debug+ #86
Call Trace:
 [<ffffffff810346a6>] ? vprintk_emit+0x48e/0x4c8
 [<ffffffff8107847d>] validate_chain+0x6c7/0xe57
 [<ffffffff8106175e>] ? local_clock+0x41/0x5a
 [<ffffffff81074c71>] ? trace_hardirqs_off_caller+0x1f/0x9e
 [<ffffffff81079444>] __lock_acquire+0x837/0x8a8
 [<ffffffff810795b7>] lock_acquire+0x102/0x12f
 [<ffffffff813b5054>] ? tty_lock+0x6c/0x70
 [<ffffffff813b25b3>] ? mutex_lock_nested+0x2ee/0x320
 [<ffffffff813b5054>] ? tty_lock+0x6c/0x70
 [<ffffffff813b233a>] mutex_lock_nested+0x75/0x320
 [<ffffffff813b5054>] ? tty_lock+0x6c/0x70
 [<ffffffff81076116>] ? trace_hardirqs_on_caller+0x12f/0x166
 [<ffffffff813b5054>] tty_lock+0x6c/0x70
 [<ffffffff813b5093>] tty_lock_pair+0x3b/0x40
 [<ffffffff8123f45d>] tty_release+0x265/0x536
 [<ffffffff8110eaee>] fput+0x127/0x282
 [<ffffffff8123eb11>] tty_ioctl+0x3d6/0xa89
 [<ffffffff8110579a>] ? kmem_cache_free+0x1db/0x242
 [<ffffffff8111d0f6>] do_vfs_ioctl+0x466/0x4ac
 [<ffffffff813b4f51>] ? _raw_spin_unlock+0x2b/0x3f
 [<ffffffff813bc115>] ? sysret_check+0x22/0x5d
 [<ffffffff8111d192>] sys_ioctl+0x56/0x79
 [<ffffffff813bc0e9>] system_call_fastpath+0x16/0x1b

Re: [PATCH] tty: annotate tty_lock() for lockdep

[Eric Dumazet]

On Thu, 2012-05-31 at 17:01 +0530, Srikar Dronamraju wrote:
> Hi Jiri, 
> 
> 
> 
> > =============================================
> > [ INFO: possible recursive locking detected ]
> > 3.4.0-08218-gb48b2c3 #7 Not tainted
> > ---------------------------------------------
> > blogd/279 is trying to acquire lock:
> >  (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff8150f51e>] tty_lock+0x3e/0x90
> > 
> > but task is already holding lock:
> >  (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff8150f51e>] tty_lock+0x3e/0x90
> > 
> > other info that might help us debug this:
> >  Possible unsafe locking scenario:
> > 
> >        CPU0
> >        ----
> >   lock(&tty->legacy_mutex);
> >   lock(&tty->legacy_mutex);
> > 
> >  *** DEADLOCK ***
> > 
> 
> I saw a similar problem and applied your patch, but that doesnt seem to
> help me.

I was not aware of Jiri patch

Try this one instead :

https://lkml.org/lkml/2012/5/31/125

Re: [PATCH] tty: annotate tty_lock() for lockdep

[Srikar Dronamraju]

> > 
> > I saw a similar problem and applied your patch, but that doesnt seem to
> > help me.
> 
> I was not aware of Jiri patch
> 
> Try this one instead :
> 
> https://lkml.org/lkml/2012/5/31/125
> 

Yes, this works for me.

-- 
Thanks and Regards
Srikar
> 

Re: [PATCH] tty: annotate tty_lock() for lockdep

[Srikar Dronamraju]

* Srikar Dronamraju <srikar@linux.vnet.ibm.com> [2012-05-31 18:51:06]:

> > > 
> > > I saw a similar problem and applied your patch, but that doesnt seem to
> > > help me.
> > 
> > I was not aware of Jiri patch
> > 
> > Try this one instead :
> > 
> > https://lkml.org/lkml/2012/5/31/125
> > 
> 
> Yes, this works for me.
> 

Just to update, I saw the problem on 3 different boxes on 3 different archs and your patch fixes it on all  of them.

-- 
Thanks and Regards
Srikar