linux-kernel - TTY/n_gsm: Removing the wrong tty_unlock/lock() in gsm_dlci

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>] [day] [month] [year] [list]

Date:	Wed, 18 Dec 2013 13:30:11 +0800
From:	Chuansheng Liu <chuansheng.liu@...el.com>
To:	gregkh@...uxfoundation.org, jslaby@...e.cz
Cc:	linux-kernel@...r.kernel.org, chao.bi@...el.com,
	chuansheng.liu@...el.com
Subject: TTY/n_gsm: Removing the wrong tty_unlock/lock() in
 gsm_dlci_release()


Commit 4d9b109060f690f5c835(tty: Prevent deadlock in n_gsm driver)
tried to close all the virtual ports synchronously before closing the
phycial ports, so the tty_vhangup() is used.

But the tty_unlock/lock() is wrong:
tty_release
 tty_ldisc_release
  tty_lock_pair(tty, o_tty)  < == Here the tty is for physical port
  tty_ldisc_kill
    gsmld_close
      gsm_cleanup_mux
        gsm_dlci_release
          tty = tty_port_tty_get(&dlci->port)
                            < == Here the tty(s) are for virtual port

They are different ttys, so before tty_vhangup(virtual tty), do not need
to call the tty_unlock(virtual tty) at all which causes unbalanced unlock
warning.

When enabling mutex debugging option, we will hit the below warning also:
[   99.276903] =====================================
[   99.282172] [ BUG: bad unlock balance detected! ]
[   99.287442] 3.10.20-261976-gaec5ba0 #44 Tainted: G           O
[   99.293972] -------------------------------------
[   99.299240] mmgr/152 is trying to release lock (&tty->legacy_mutex) at:
[   99.306693] [<c1b2dcad>] mutex_unlock+0xd/0x10
[   99.311669] but there are no more locks to release!
[   99.317131]
[   99.317131] other info that might help us debug this:
[   99.324440] 3 locks held by mmgr/152:
[   99.328542]  #0:  (&tty->legacy_mutex/1){......}, at: [<c1b30ab0>] tty_lock_nested+0x40/0x90
[   99.338116]  #1:  (&tty->ldisc_mutex){......}, at: [<c15dbd02>] tty_ldisc_kill+0x22/0xd0
[   99.347284]  #2:  (&gsm->mutex){......}, at: [<c15e3d83>] gsm_cleanup_mux+0x73/0x170
[   99.356060]
[   99.356060] stack backtrace:
[   99.360932] CPU: 0 PID: 152 Comm: mmgr Tainted: G           O 3.10.20-261976-gaec5ba0 #44
[   99.370086]  ef4a4de0 ef4a4de0 ef4c1d98 c1b27b91 ef4c1db8 c1292655 c1dd10f5 c1b2dcad
[   99.378921]  c1b2dcad ef4a4de0 ef4a528c ffffffff ef4c1dfc c12930dd 00000246 00000000
[   99.387754]  00000000 00000000 c15e1926 00000000 00000001 ddfa7530 00000003 c1b2dcad
[   99.396588] Call Trace:
[   99.399326]  [<c1b27b91>] dump_stack+0x16/0x18
[   99.404307]  [<c1292655>] print_unlock_imbalance_bug+0xe5/0xf0
[   99.410840]  [<c1b2dcad>] ? mutex_unlock+0xd/0x10
[   99.416110]  [<c1b2dcad>] ? mutex_unlock+0xd/0x10
[   99.421382]  [<c12930dd>] lock_release_non_nested+0x1cd/0x210
[   99.427818]  [<c15e1926>] ? gsm_destroy_network+0x36/0x130
[   99.433964]  [<c1b2dcad>] ? mutex_unlock+0xd/0x10
[   99.439235]  [<c12931a2>] lock_release+0x82/0x1c0
[   99.444505]  [<c1b2dcad>] ? mutex_unlock+0xd/0x10
[   99.449776]  [<c1b2dcad>] ? mutex_unlock+0xd/0x10
[   99.455047]  [<c1b2dc2f>] __mutex_unlock_slowpath+0x5f/0xd0
[   99.461288]  [<c1b2dcad>] mutex_unlock+0xd/0x10
[   99.466365]  [<c1b30bb1>] tty_unlock+0x21/0x50
[   99.471345]  [<c15e3dd1>] gsm_cleanup_mux+0xc1/0x170
[   99.476906]  [<c15e44d2>] gsmld_close+0x52/0x90
[   99.481983]  [<c15db905>] tty_ldisc_close.isra.1+0x35/0x50
[   99.488127]  [<c15dbd0c>] tty_ldisc_kill+0x2c/0xd0
[   99.493494]  [<c15dc7af>] tty_ldisc_release+0x2f/0x50
[   99.499152]  [<c15d572c>] tty_release+0x37c/0x4b0
[   99.504424]  [<c1b2dcad>] ? mutex_unlock+0xd/0x10
[   99.509695]  [<c1b2dcad>] ? mutex_unlock+0xd/0x10
[   99.514967]  [<c1372f6e>] ? eventpoll_release_file+0x7e/0x90
[   99.521307]  [<c1335849>] __fput+0xd9/0x200
[   99.525996]  [<c133597d>] ____fput+0xd/0x10
[   99.530685]  [<c125c731>] task_work_run+0x81/0xb0
[   99.535957]  [<c12019e9>] do_notify_resume+0x49/0x70
[   99.541520]  [<c1b30dc4>] work_notifysig+0x29/0x31
[   99.546897] ------------[ cut here ]------------

So here we can call tty_vhangup() directly which is for virtual port.

Reviewed-by: Chao Bi <chao.bi@...el.com>
Signed-off-by: Liu, Chuansheng <chuansheng.liu@...el.com>
---
 drivers/tty/n_gsm.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c0f76da..8187ae6 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1704,11 +1704,8 @@ static void gsm_dlci_release(struct gsm_dlci *dlci)
 		gsm_destroy_network(dlci);
 		mutex_unlock(&dlci->mutex);
 
-		/* tty_vhangup needs the tty_lock, so unlock and
-		   relock after doing the hangup. */
-		tty_unlock(tty);
 		tty_vhangup(tty);
-		tty_lock(tty);
+
 		tty_port_tty_set(&dlci->port, NULL);
 		tty_kref_put(tty);
 	}
-- 
1.7.9.5



--
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/