linux-kernel - Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACVXFVO_8o_3UuwAHjJbVEk1ZmL+28KVngB7JikNxszct1yjkw@mail.gmail.com>
Date:	Fri, 18 May 2012 09:57:29 +0800
From:	Ming Lei <ming.lei@...onical.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, Alan Cox <alan@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair

On Fri, May 18, 2012 at 2:48 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> On Thu, 2012-05-17 at 11:28 -0700, Greg Kroah-Hartman wrote:
>> > +static void __lockfunc tty_lock_nest_lock(struct tty_struct *tty,
>> > +             struct tty_struct *tty2)
>>
>> Duplicating tty_lock() just for this one issue seems wrong and prone to
>> error, don't you think?
>>
>> > +{
>> > +     if (tty->magic != TTY_MAGIC) {
>> > +             printk(KERN_ERR "L Bad %p\n", tty);
>> > +             WARN_ON(1);
>> > +             return;
>> > +     }
>> > +     tty_kref_get(tty);
>> > +     mutex_lock_nest_lock(&tty->legacy_mutex, &tty2->legacy_mutex);
>
> Yeah, its completely broken, even the lockdep annotation is the wrong
> one.
>
> Something like the (completely untested) below patch is the 'right' way.

Yes, it should be, but the warning is still triggered with the patch, also
it is worse, kernel hanged during boot.

> ---
>  drivers/tty/tty_mutex.c |   35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
> index 69adc80..587330b 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)
> +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 +18,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);
>
> @@ -38,25 +43,25 @@ EXPORT_SYMBOL(tty_unlock);
>  * Getting the big tty mutex for a pair of ttys with lock ordering
>  * On a non pty/tty pair tty2 can be NULL which is just fine.
>  */
> -void __lockfunc tty_lock_pair(struct tty_struct *tty,
> -                                       struct tty_struct *tty2)
> +void __lockfunc tty_lock_pair(struct tty_struct *tty1, struct tty_struct *tty2)
>  {
> -       if (tty < tty2) {
> -               tty_lock(tty);
> -               tty_lock(tty2);
> -       } else {
> -               if (tty2 && tty2 != tty)
> -                       tty_lock(tty2);
> -               tty_lock(tty);
> +       if (!tty2 || tty1 == tty2) {
> +               tty_lock(tty1);
> +               return;
>        }
> +
> +       if (tty2 < tty1)
> +               swap(tty1, tty2);

That is too crazy.

> +
> +       tty_lock(tty1);
> +       tty_lock_nested(tty2, SINGLE_DEPTH_NESTING);
>  }
>  EXPORT_SYMBOL(tty_lock_pair);
>
> -void __lockfunc tty_unlock_pair(struct tty_struct *tty,
> -                                               struct tty_struct *tty2)
> +void __lockfunc tty_unlock_pair(struct tty_struct *tty1, struct tty_struct *tty2)
>  {
> -       tty_unlock(tty);
> -       if (tty2 && tty2 != tty)
> +       tty_unlock(tty1);
> +       if (tty2 && tty2 != tty1)
>                tty_unlock(tty2);
>  }
>  EXPORT_SYMBOL(tty_unlock_pair);
>

So how about the below one(tested OK)?

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);


Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/