* TTY: NULL dereference when closing a pty file
@ 2012-02-19 20:36 Sasha Levin
2012-02-19 21:08 ` [PATCH 1/1] TTY: fix PTY hangup vs close race Jiri Slaby
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Sasha Levin @ 2012-02-19 20:36 UTC (permalink / raw)
To: Jiri Slaby, Greg KH; +Cc: linux-kernel@vger.kernel.org
Hi all,
I got the following BUG() when running trinity on the KVM tool:
[ 665.738774] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
[ 665.739651] IP: [<ffffffff81257e2b>] devpts_pty_kill+0x1b/0xa0
[ 665.739651] PGD 22eba067 PUD 22eaf067 PMD 0
[ 665.739651] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 665.739651] CPU 5
[ 665.739651] Pid: 3061, comm: trinity Not tainted 3.3.0-rc3-next-20120217-sasha-00001-gfa56acb #18
[ 665.739651] RIP: 0010:[<ffffffff81257e2b>] [<ffffffff81257e2b>] devpts_pty_kill+0x1b/0xa0
[ 665.739651] RSP: 0018:ffff880022ecfd88 EFLAGS: 00010286
[ 665.739651] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[ 665.739651] RDX: 0000000000000001 RSI: 0000000000000001 RDI: ffff880022921148
[ 665.739651] RBP: ffff880022ecfda8 R08: 0000000000000000 R09: 0000000000000001
[ 665.739651] R10: 0000000000000001 R11: 0000000000000001 R12: ffff880022921148
[ 665.739651] R13: ffff880022934840 R14: ffff880026831970 R15: ffff880026831970
[ 665.739651] FS: 00007f29c344e700(0000) GS:ffff88002a400000(0000) knlGS:0000000000000000
[ 665.739651] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 665.739651] CR2: 0000000000000028 CR3: 0000000022e1c000 CR4: 00000000000406e0
[ 665.739651] DR0: ffffffff810adc50 DR1: 0000000000000000 DR2: 0000000000000000
[ 665.739651] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
[ 665.739651] Process trinity (pid: 3061, threadinfo ffff880022ece000, task ffff880022d88000)
[ 665.739651] Stack:
[ 665.739651] ffff880022ecfd98 ffff8800229267b0 ffff880022921148 ffff880022934840
[ 665.739651] ffff880022ecfdc8 ffffffff819407f1 ffff880022934840 ffff8800229267b0
[ 665.739651] ffff880022ecfeb8 ffffffff819374a6 ffff880022ecfe18 ffffffff81078e76
[ 665.739651] Call Trace:
[ 665.739651] [<ffffffff819407f1>] pty_close+0x121/0x140
[ 665.739651] [<ffffffff819374a6>] tty_release+0x186/0x610
[ 665.739651] [<ffffffff81078e76>] ? kvm_clock_read+0x46/0x80
[ 665.739651] [<ffffffff81054973>] ? sched_clock+0x13/0x20
[ 665.739651] [<ffffffff811d9b16>] fput+0xf6/0x330
[ 665.739651] [<ffffffff811d68b4>] filp_close+0x64/0x90
[ 665.739651] [<ffffffff811d699b>] sys_close+0xbb/0x1b0
[ 665.739651] [<ffffffff8267c079>] system_call_fastpath+0x16/0x1b
[ 665.739651] Code: 4c 8b a3 08 04 00 00 eb df 0f 0b 0f 1f 44 00 00 55 48 89 e5 48 83 ec 20 48 89 5d e8 4c 89 65 f0 4c 89 6d f8 48 8b 9f 28 04 00 00 <48> 8b 43 28 48 81 78 58 d1 1c 00 00 74 0b 48 8b 05 f0 17 26 03
[ 665.739651] RIP [<ffffffff81257e2b>] devpts_pty_kill+0x1b/0xa0
[ 665.739651] RSP <ffff880022ecfd88>
[ 665.739651] CR2: 0000000000000028
[ 665.782570] ---[ end trace 5b128b9a8217de35 ]---
Looking further, it looks like devpts_pty_kill was called with tty->link==NULL.
I've bisected it down to d3bda529 ("TTY: get rid of BTM around devpts_*"), which has moved devpts_pty_kill() out of the tty_lock() protection.
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/1] TTY: fix PTY hangup vs close race 2012-02-19 20:36 TTY: NULL dereference when closing a pty file Sasha Levin @ 2012-02-19 21:08 ` Jiri Slaby 2012-02-19 21:10 ` TTY: NULL dereference when closing a pty file Jiri Slaby 2012-02-19 21:19 ` [PATCH 1/1] TTY: fix PTY hangup vs close race Jiri Slaby 2 siblings, 0 replies; 9+ messages in thread From: Jiri Slaby @ 2012-02-19 21:08 UTC (permalink / raw) To: gregkh; +Cc: linux-kernel, Sasha Levin, jirislaby Commit d3bda5298 (TTY: get rid of BTM around devpts_*) moved devpts_pty_kill out of BTM, but the BTM was not protecting only devpts_pty_kill, but also tty->link. Hence move the function back at this late stage until this gets resolved properly some time later. I was confused by tty_vhangup(tty->link) outside BTM. But inside of tty_vhangup, there is a check for tty == NULL. But we cannot add such a check here. We have to have the tty and free the devpts node... Signed-off-by: Jiri Slaby <jslaby@suse.cz> Reported-by: Sasha Levin <levinsasha928@gmail.com> --- drivers/tty/pty.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index fa1bd2e..95037aa 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -54,8 +54,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp) wake_up_interruptible(&tty->link->write_wait); if (tty->driver->subtype == PTY_TYPE_MASTER) { set_bit(TTY_OTHER_CLOSED, &tty->flags); - tty_unlock(); + /* BTM protects tty->link here */ devpts_pty_kill(tty->link); + tty_unlock(); tty_vhangup(tty->link); tty_lock(); } -- 1.7.9 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: TTY: NULL dereference when closing a pty file 2012-02-19 20:36 TTY: NULL dereference when closing a pty file Sasha Levin 2012-02-19 21:08 ` [PATCH 1/1] TTY: fix PTY hangup vs close race Jiri Slaby @ 2012-02-19 21:10 ` Jiri Slaby 2012-02-19 23:08 ` Sasha Levin 2012-02-19 21:19 ` [PATCH 1/1] TTY: fix PTY hangup vs close race Jiri Slaby 2 siblings, 1 reply; 9+ messages in thread From: Jiri Slaby @ 2012-02-19 21:10 UTC (permalink / raw) To: Sasha Levin; +Cc: Greg KH, linux-kernel@vger.kernel.org On 02/19/2012 09:36 PM, Sasha Levin wrote: > I got the following BUG() when running trinity on the KVM tool: Hi, I've sent a patch as a reply to your message. Could you test it? And where can one obtain the tool? thanks, -- js ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: TTY: NULL dereference when closing a pty file 2012-02-19 21:10 ` TTY: NULL dereference when closing a pty file Jiri Slaby @ 2012-02-19 23:08 ` Sasha Levin 0 siblings, 0 replies; 9+ messages in thread From: Sasha Levin @ 2012-02-19 23:08 UTC (permalink / raw) To: Jiri Slaby Cc: Greg KH, linux-kernel@vger.kernel.org, Pekka Enberg, Dave Jones On Sun, 2012-02-19 at 22:10 +0100, Jiri Slaby wrote: > On 02/19/2012 09:36 PM, Sasha Levin wrote: > > I got the following BUG() when running trinity on the KVM tool: > > Hi, I've sent a patch as a reply to your message. Could you test it? Sure. I'll re-run and report back tomorrow. > And where can one obtain the tool? The KVM tool is developed at https://github.com/penberg/linux-kvm Trinity is developed at http://codemonkey.org.uk/projects/trinity/trinity.git If you have any questions about how to use them both feel free to message me privately. > thanks, ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] TTY: fix PTY hangup vs close race 2012-02-19 20:36 TTY: NULL dereference when closing a pty file Sasha Levin 2012-02-19 21:08 ` [PATCH 1/1] TTY: fix PTY hangup vs close race Jiri Slaby 2012-02-19 21:10 ` TTY: NULL dereference when closing a pty file Jiri Slaby @ 2012-02-19 21:19 ` Jiri Slaby 2012-02-19 21:41 ` Jiri Slaby 2 siblings, 1 reply; 9+ messages in thread From: Jiri Slaby @ 2012-02-19 21:19 UTC (permalink / raw) To: gregkh; +Cc: linux-kernel, Sasha Levin, jirislaby Commit d3bda5298 (TTY: get rid of BTM around devpts_*) moved devpts_pty_kill out of BTM, but the BTM was not protecting only devpts_pty_kill, but also tty->link. Hence move the function back at this late stage until this gets resolved properly some time later. I was confused by tty_vhangup(tty->link) outside BTM. But inside of tty_vhangup, there is a check for tty == NULL. But we cannot add such a check here. We have to have the tty and free the devpts node... Signed-off-by: Jiri Slaby <jslaby@suse.cz> Reported-by: Sasha Levin <levinsasha928@gmail.com> --- Gee, I messed up Greg's address again... drivers/tty/pty.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index fa1bd2e..95037aa 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -54,8 +54,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp) wake_up_interruptible(&tty->link->write_wait); if (tty->driver->subtype == PTY_TYPE_MASTER) { set_bit(TTY_OTHER_CLOSED, &tty->flags); - tty_unlock(); + /* BTM protects tty->link here */ devpts_pty_kill(tty->link); + tty_unlock(); tty_vhangup(tty->link); tty_lock(); } -- 1.7.9 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] TTY: fix PTY hangup vs close race 2012-02-19 21:19 ` [PATCH 1/1] TTY: fix PTY hangup vs close race Jiri Slaby @ 2012-02-19 21:41 ` Jiri Slaby 2012-02-20 10:20 ` Sasha Levin 0 siblings, 1 reply; 9+ messages in thread From: Jiri Slaby @ 2012-02-19 21:41 UTC (permalink / raw) To: Jiri Slaby; +Cc: gregkh, linux-kernel, Sasha Levin On 02/19/2012 10:19 PM, Jiri Slaby wrote: > Commit d3bda5298 (TTY: get rid of BTM around devpts_*) moved > devpts_pty_kill out of BTM, but the BTM was not protecting only > devpts_pty_kill, but also tty->link. Hence move the function back at > this late stage until this gets resolved properly some time later. > > I was confused by tty_vhangup(tty->link) outside BTM. But inside of > tty_vhangup, there is a check for tty == NULL. But we cannot add such > a check here. We have to have the tty and free the devpts node... > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Reported-by: Sasha Levin <levinsasha928@gmail.com> > --- > > Gee, I messed up Greg's address again... > > drivers/tty/pty.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > index fa1bd2e..95037aa 100644 > --- a/drivers/tty/pty.c > +++ b/drivers/tty/pty.c > @@ -54,8 +54,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp) > wake_up_interruptible(&tty->link->write_wait); > if (tty->driver->subtype == PTY_TYPE_MASTER) { > set_bit(TTY_OTHER_CLOSED, &tty->flags); > - tty_unlock(); > + /* BTM protects tty->link here */ > devpts_pty_kill(tty->link); > + tty_unlock(); I'm afraid this won't help. As this is based on an assumption that tty->link is NULL [*] and that is not just true. Greg, please revert commit d3bda5298 completely. [*] Your dump reveals that the code fetches tty->driver_data (mov 0x428(%rdi),%rbx) and traps at a fetch of inode->i_sbm because inode is NULL (mov 0x28(%rbx),%rax). Anyway I'm still interested in the tool you triggered this, because we will need to get rid of BTM eventually. thanks, -- js ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] TTY: fix PTY hangup vs close race 2012-02-19 21:41 ` Jiri Slaby @ 2012-02-20 10:20 ` Sasha Levin 2012-02-20 11:15 ` Jiri Slaby 0 siblings, 1 reply; 9+ messages in thread From: Sasha Levin @ 2012-02-20 10:20 UTC (permalink / raw) To: Jiri Slaby; +Cc: Jiri Slaby, gregkh, linux-kernel On Sun, Feb 19, 2012 at 11:41 PM, Jiri Slaby <jirislaby@gmail.com> wrote: > On 02/19/2012 10:19 PM, Jiri Slaby wrote: >> Commit d3bda5298 (TTY: get rid of BTM around devpts_*) moved >> devpts_pty_kill out of BTM, but the BTM was not protecting only >> devpts_pty_kill, but also tty->link. Hence move the function back at >> this late stage until this gets resolved properly some time later. >> >> I was confused by tty_vhangup(tty->link) outside BTM. But inside of >> tty_vhangup, there is a check for tty == NULL. But we cannot add such >> a check here. We have to have the tty and free the devpts node... >> >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> >> Reported-by: Sasha Levin <levinsasha928@gmail.com> >> --- >> >> Gee, I messed up Greg's address again... >> >> drivers/tty/pty.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c >> index fa1bd2e..95037aa 100644 >> --- a/drivers/tty/pty.c >> +++ b/drivers/tty/pty.c >> @@ -54,8 +54,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp) >> wake_up_interruptible(&tty->link->write_wait); >> if (tty->driver->subtype == PTY_TYPE_MASTER) { >> set_bit(TTY_OTHER_CLOSED, &tty->flags); >> - tty_unlock(); >> + /* BTM protects tty->link here */ >> devpts_pty_kill(tty->link); >> + tty_unlock(); > > I'm afraid this won't help. As this is based on an assumption that > tty->link is NULL [*] and that is not just true. > > Greg, please revert commit d3bda5298 completely. > > [*] Your dump reveals that the code fetches tty->driver_data (mov > 0x428(%rdi),%rbx) and traps at a fetch of inode->i_sbm because inode is > NULL (mov 0x28(%rbx),%rax). It actually looks even more complex than that. I reverted the patch above, but still got the error. A quick bisection pointed me to a50f724a432997321cabb6c9e665c28e34850f78. Looks like reverting both actually solves the problem. Reverting just one of them doesn't. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] TTY: fix PTY hangup vs close race 2012-02-20 10:20 ` Sasha Levin @ 2012-02-20 11:15 ` Jiri Slaby 2012-02-24 21:57 ` Greg KH 0 siblings, 1 reply; 9+ messages in thread From: Jiri Slaby @ 2012-02-20 11:15 UTC (permalink / raw) To: Sasha Levin; +Cc: Jiri Slaby, gregkh, linux-kernel On 02/20/2012 11:20 AM, Sasha Levin wrote: >> [*] Your dump reveals that the code fetches tty->driver_data (mov >> 0x428(%rdi),%rbx) and traps at a fetch of inode->i_sbm because inode is >> NULL (mov 0x28(%rbx),%rax). > > It actually looks even more complex than that. I reverted the patch > above, but still got the error. A quick bisection pointed me to > a50f724a432997321cabb6c9e665c28e34850f78. > Looks like reverting both actually solves the problem. Reverting just > one of them doesn't. Ah, I see. I suppose you have CONFIG_LEGACY_PTYS=y. I somehow overlooked that case. So both of them should be reverted in upstream: d3bda5298aad98c7a27678bdd0dd9d008ab9e685 a50f724a432997321cabb6c9e665c28e34850f78 thanks, -- js suse labs ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] TTY: fix PTY hangup vs close race 2012-02-20 11:15 ` Jiri Slaby @ 2012-02-24 21:57 ` Greg KH 0 siblings, 0 replies; 9+ messages in thread From: Greg KH @ 2012-02-24 21:57 UTC (permalink / raw) To: Jiri Slaby; +Cc: Sasha Levin, Jiri Slaby, linux-kernel On Mon, Feb 20, 2012 at 12:15:31PM +0100, Jiri Slaby wrote: > On 02/20/2012 11:20 AM, Sasha Levin wrote: > >> [*] Your dump reveals that the code fetches tty->driver_data (mov > >> 0x428(%rdi),%rbx) and traps at a fetch of inode->i_sbm because inode is > >> NULL (mov 0x28(%rbx),%rax). > > > > It actually looks even more complex than that. I reverted the patch > > above, but still got the error. A quick bisection pointed me to > > a50f724a432997321cabb6c9e665c28e34850f78. > > > Looks like reverting both actually solves the problem. Reverting just > > one of them doesn't. > > Ah, I see. I suppose you have CONFIG_LEGACY_PTYS=y. I somehow overlooked > that case. > > So both of them should be reverted in upstream: > d3bda5298aad98c7a27678bdd0dd9d008ab9e685 > a50f724a432997321cabb6c9e665c28e34850f78 Both now reverted, thanks. greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-02-24 21:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-19 20:36 TTY: NULL dereference when closing a pty file Sasha Levin 2012-02-19 21:08 ` [PATCH 1/1] TTY: fix PTY hangup vs close race Jiri Slaby 2012-02-19 21:10 ` TTY: NULL dereference when closing a pty file Jiri Slaby 2012-02-19 23:08 ` Sasha Levin 2012-02-19 21:19 ` [PATCH 1/1] TTY: fix PTY hangup vs close race Jiri Slaby 2012-02-19 21:41 ` Jiri Slaby 2012-02-20 10:20 ` Sasha Levin 2012-02-20 11:15 ` Jiri Slaby 2012-02-24 21:57 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).