linux-kernel - Re: [PATCH v3 00/10] BKL conversion in tty layer

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201006172348.08180.arnd@arndb.de>
Date:	Thu, 17 Jun 2010 23:48:07 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Tony Luck <tony.luck@...el.com>
Cc:	Alan Cox <alan@...rguk.ukuu.org.uk>, linux-kernel@...r.kernel.org,
	Greg KH <gregkh@...e.de>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	John Kacur <jkacur@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>, Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH v3 00/10] BKL conversion in tty layer

On Thursday 17 June 2010 22:15:32 Tony Luck wrote:
> Call Trace:
>  [<a000000100015990>] show_stack+0x50/0xa0
>  [<a00000010090f1f0>] dump_stack+0x30/0x50
>  [<a00000010008e280>] warn_slowpath_common+0xc0/0x120
>  [<a00000010008e320>] warn_slowpath_null+0x40/0x60
>  [<a00000010053d910>] tty_release+0x90/0xbc0
>  [<a0000001001ab200>] __fput+0x260/0x420
>  [<a0000001001ab400>] fput+0x40/0x60
>  [<a00000010053b3b0>] tty_vhangup_locked+0x870/0x8a0
>  [<a00000010054f3f0>] pty_close+0x350/0x3a0
>  [<a00000010053ddd0>] tty_release+0x550/0xbc0
>  [<a0000001001ab200>] __fput+0x260/0x420
>  [<a0000001001ab400>] fput+0x40/0x60
>  [<a0000001001a4dc0>] filp_close+0x120/0x140
>  [<a0000001001a4f90>] sys_close+0x1b0/0x2c0
>  [<a00000010000b940>] ia64_ret_from_syscall+0x0/0x20
> 

Ah, this sucks. I think Alan actually tried to warn me of this problem and I
thought I had it right, but obviously I got it wrong in the end. I really
should have run into this during testing though, not sure why I didn't.

The good news is that the warning message is harmless for the normal
case where CONFIG_TTY_MUTEX remains disabled, it's only debugging code
to warn that there is a bug once the option gets turned on.

Unfortunately however the only fix I see is to push the BTM further down
into the hangup function, which makes the pty code slightly more complex
and which I'm sure is an equivalent transformation.

I'll try doing some more tests with this patch and CONFIG_TTY_LOCK disabled.

Signed-off-by: Arnd Bergmann <arnd@...db.de>

diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index c9af9ff..0902127 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -62,7 +62,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 		if (tty->driver == ptm_driver)
 			devpts_pty_kill(tty->link);
 #endif
-		tty_vhangup_locked(tty->link);
+		unlock_kernel();
+		tty_vhangup(tty->link);
+		lock_kernel();
 	}
 }
 
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 5db354d..852ccb4 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -471,7 +471,7 @@ void tty_wakeup(struct tty_struct *tty)
 EXPORT_SYMBOL_GPL(tty_wakeup);
 
 /**
- *	do_tty_hangup		-	actual handler for hangup events
+ *	__tty_hangup		-	actual handler for hangup events
  *	@work: tty device
  *
  *	This can be called by the "eventd" kernel thread.  That is process
@@ -492,7 +492,7 @@ EXPORT_SYMBOL_GPL(tty_wakeup);
  *		  tasklist_lock to walk task list for hangup event
  *		    ->siglock to protect ->signal/->sighand
  */
-void tty_vhangup_locked(struct tty_struct *tty)
+void __tty_hangup(struct tty_struct *tty)
 {
 	struct file *cons_filp = NULL;
 	struct file *filp, *f = NULL;
@@ -512,10 +512,12 @@ void tty_vhangup_locked(struct tty_struct *tty)
 	}
 	spin_unlock(&redirect_lock);
 
+	tty_lock();
+
 	/* inuse_filps is protected by the single tty lock,
 	   this really needs to change if we want to flush the
 	   workqueue with the lock held */
-	check_tty_count(tty, "do_tty_hangup");
+	check_tty_count(tty, "tty_hangup");
 
 	file_list_lock();
 	/* This breaks for file handles being sent over AF_UNIX sockets ? */
@@ -594,6 +596,9 @@ void tty_vhangup_locked(struct tty_struct *tty)
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
 	tty_ldisc_enable(tty);
+
+	tty_unlock();
+
 	if (f)
 		fput(f);
 }
@@ -603,9 +608,7 @@ static void do_tty_hangup(struct work_struct *work)
 	struct tty_struct *tty =
 		container_of(work, struct tty_struct, hangup_work);
 
-	tty_lock();
-	tty_vhangup_locked(tty);
-	tty_unlock();
+	__tty_hangup(tty);
 }
 
 /**
@@ -643,13 +646,12 @@ void tty_vhangup(struct tty_struct *tty)
 
 	printk(KERN_DEBUG "%s vhangup...\n", tty_name(tty, buf));
 #endif
-	tty_lock();
-	tty_vhangup_locked(tty);
-	tty_unlock();
+	__tty_hangup(tty);
 }
 
 EXPORT_SYMBOL(tty_vhangup);
 
+
 /**
  *	tty_vhangup_self	-	process vhangup for own ctty
  *
@@ -727,10 +729,8 @@ void disassociate_ctty(int on_exit)
 	if (tty) {
 		tty_pgrp = get_pid(tty->pgrp);
 		if (on_exit) {
-			tty_lock();
 			if (tty->driver->type != TTY_DRIVER_TYPE_PTY)
-				tty_vhangup_locked(tty);
-			tty_unlock();
+				tty_vhangup(tty);
 		}
 		tty_kref_put(tty);
 	} else if (on_exit) {
--
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/