| Message ID | 1362350153-26225-1-git-send-email-sasha.levin@oracle.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Sasha Levin <sasha.levin@oracle.com> Date: Sun, 3 Mar 2013 17:35:53 -0500 > ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep > might take a long time we can prevent other processes from accessing the tty, > causing hung tasks and a dead tty. > > Diagnosed-by: Peter Hurley <peter@hurleysoftware.com> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> But then you invalidate all of the tty state tests made under the lock at the beginning of this function, before enterring the loop. If you drop the lock, those pieces of state could change. I'm not applying this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/03/2013 05:47 PM, David Miller wrote: > From: Sasha Levin <sasha.levin@oracle.com> > Date: Sun, 3 Mar 2013 17:35:53 -0500 > >> ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep >> might take a long time we can prevent other processes from accessing the tty, >> causing hung tasks and a dead tty. >> >> Diagnosed-by: Peter Hurley <peter@hurleysoftware.com> >> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > > But then you invalidate all of the tty state tests made under > the lock at the beginning of this function, before enterring > the loop. If you drop the lock, those pieces of state could > change. > > I'm not applying this. I'm unsure. A similar patch was applied back in 2010 that does the same thing to a bunch of drivers, including the core tty code (e142a31da "tty: release BTM while sleeping in block_til_ready"). This IR code looks very much like tty_port_block_til_ready() where it was okay to do that change, so I should be the same with ircomm_tty_block_til_ready. Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-03-03 at 17:47 -0500, David Miller wrote: > From: Sasha Levin <sasha.levin@oracle.com> > Date: Sun, 3 Mar 2013 17:35:53 -0500 > > > ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep > > might take a long time we can prevent other processes from accessing the tty, > > causing hung tasks and a dead tty. > > > > Diagnosed-by: Peter Hurley <peter@hurleysoftware.com> > > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > > But then you invalidate all of the tty state tests made under > the lock at the beginning of this function, before enterring > the loop. If you drop the lock, those pieces of state could > change. Yes, the state could change. For example, the tty could be hung up while ircomm_tty_block_til_ready() is sleeping. Or the session leader could be exiting and SIGHUPed this task. Or the port could have been shutdown. All these are re-tested in the loop. What state test isn't repeated? > I'm not applying this. That's certainly your perogative. But you should know this bug hangs the entire tty subsystem. This is the correct fix and exactly how this is done by the tty port. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sasha Levin <sasha.levin@oracle.com> Date: Sun, 03 Mar 2013 18:17:38 -0500 > On 03/03/2013 05:47 PM, David Miller wrote: >> From: Sasha Levin <sasha.levin@oracle.com> >> Date: Sun, 3 Mar 2013 17:35:53 -0500 >> >>> ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep >>> might take a long time we can prevent other processes from accessing the tty, >>> causing hung tasks and a dead tty. >>> >>> Diagnosed-by: Peter Hurley <peter@hurleysoftware.com> >>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >> >> But then you invalidate all of the tty state tests made under >> the lock at the beginning of this function, before enterring >> the loop. If you drop the lock, those pieces of state could >> change. >> >> I'm not applying this. > > I'm unsure. A similar patch was applied back in 2010 that does the same thing > to a bunch of drivers, including the core tty code (e142a31da "tty: release > BTM while sleeping in block_til_ready"). > > This IR code looks very much like tty_port_block_til_ready() where it was > okay to do that change, so I should be the same with ircomm_tty_block_til_ready. That assumes that the other changes don't have the same bug. Releasing locks are dangerous, because it invalidates the context in which all previous tests of state have been performed. Anything can happen to the TTY once you drop that lock. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Peter Hurley <peter@hurleysoftware.com> Date: Sun, 03 Mar 2013 19:04:25 -0500 > All these are re-tested in the loop. What state test isn't repeated? One that rechecks the non-blocking filp flag, the TTY_IO_ERROR tty flag and the termios settings. Like I said, all of the state tests performed at the beginning of this function, before enterring the loop. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-03-03 at 19:33 -0500, David Miller wrote: > From: Peter Hurley <peter@hurleysoftware.com> > Date: Sun, 03 Mar 2013 19:04:25 -0500 > > > All these are re-tested in the loop. What state test isn't repeated? > > One that rechecks the non-blocking filp flag, the > TTY_IO_ERROR tty flag and the termios settings. > > Like I said, all of the state tests performed at the beginning of > this function, before enterring the loop. How is O_NONBLOCK going to change? This function is sitting on the user-space open. The filp parameter is only on this task stack. It hasn't been linked in anywhere else. Because of course the file isn't open yet because this function hasn't returned success. The TTY_IO_ERROR flag is used by drivers (this one included) to turn away concurrent reads and writes when shutting down. The tty core does not set this. Now this driver might set this, if commanded to hangup via ircomm_tty_hangup, but like I said that's already handled in the loop by testing tty_hung_up_p. The initial termios setting cflag settings are set by the driver open(). In this driver, its here: driver->init_termios = tty_std_termios; driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL | CLOCAL; Now, it's possible that one could construct an imaginary race, where the tty has already been opened and that task now sets the termios without CLOCAL and meanwhile a second task is racing this termios setting with an open() of its own, but since there is no expectation from userspace that those operations are serialized, there's no reason to serialize them here. But regardless, this function __cannot__ sleep holding the tty_lock(). Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-03-03 at 17:47 -0500, David Miller wrote: > From: Sasha Levin <sasha.levin@oracle.com> > Date: Sun, 3 Mar 2013 17:35:53 -0500 > > > ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep > > might take a long time we can prevent other processes from accessing the tty, > > causing hung tasks and a dead tty. > > > > Diagnosed-by: Peter Hurley <peter@hurleysoftware.com> > > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > > But then you invalidate all of the tty state tests made under > the lock at the beginning of this function, before enterring > the loop. If you drop the lock, those pieces of state could > change. > > I'm not applying this. BTW, Sasha deserves a medal for finding and fixing this. Here's the initial report [1] by him from Halloween. And he doesn't even have an IR device. So this fix needs to be cc'd to stable too. Regards, Peter Hurley [1] On Wed, 2012-10-31 at 16:10 -0400, Sasha Levin wrote: On 10/31/2012 11:32 AM, Jiri Slaby wrote: > > On 10/31/2012 04:30 PM, Sasha Levin wrote: > >> On Wed, Oct 31, 2012 at 8:53 AM, Jiri Slaby <jslaby@suse.cz> wrote: > >>> On 10/25/2012 08:02 PM, Sasha Levin wrote: > >>>> Fuzzing with trinity inside a KVM tools (lkvm) guest with -next kernel > >>>> uncovered the following warning: > >>> > >>> I cannot reproduce that :(. Do you still see it? > >> > >> Yes, it reproduces pretty easily while fuzzing. > > > > What is your exact setup? I tried trinity with 100 000 syscalls inside > > KVM with an LDEP-enabled kernel. How many serial ports do you have in > > the guest? Any USB serials in there? > > btw, I'm also seeing the following lockups, don't know if it's related: > > > [ 2283.070569] INFO: task trinity-child20:9161 blocked for more than 120 seconds. > [ 2283.071775] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > [ 2283.074673] trinity-child20 D ffff8800276cb000 5424 9161 6364 0x00000000 > [ 2283.076018] ffff880059d9da58 0000000000000002 0000000000000002 0000000000000000 > [ 2283.077393] ffff880059d7b000 ffff880059d9dfd8 ffff880059d9dfd8 ffff880059d9dfd8 > [ 2283.078763] ffff8800276cb000 ffff880059d7b000 ffff880059d9da78 ffff88001a095180 > [ 2283.084144] Call Trace: > [ 2283.085039] [<ffffffff83a98bd5>] schedule+0x55/0x60 > [ 2283.086748] [<ffffffff83a98bf3>] schedule_preempt_disabled+0x13/0x20 > [ 2283.089000] [<ffffffff83a9735d>] __mutex_lock_common+0x36d/0x5a0 > [ 2283.090658] [<ffffffff83a9afb3>] ? tty_lock_nested+0x73/0x80 > [ 2283.091691] [<ffffffff83a9afb3>] ? tty_lock_nested+0x73/0x80 > [ 2283.092779] [<ffffffff83a975cf>] mutex_lock_nested+0x3f/0x50 > [ 2283.093875] [<ffffffff83a9afb3>] tty_lock_nested+0x73/0x80 > [ 2283.094872] [<ffffffff83a9afcb>] tty_lock+0xb/0x10 > [ 2283.095443] [<ffffffff81bae880>] tty_open+0x270/0x5f0 > [ 2283.096181] [<ffffffff8127cda8>] chrdev_open+0xf8/0x1d0 > [ 2283.097054] [<ffffffff8127693c>] do_dentry_open+0x1fc/0x310 > [ 2283.098015] [<ffffffff8127ccb0>] ? cdev_put+0x20/0x20 > [ 2283.098943] [<ffffffff8127777a>] finish_open+0x4a/0x60 > [ 2283.099935] [<ffffffff81286947>] do_last+0xb87/0xe70 > [ 2283.100910] [<ffffffff812844b0>] ? link_path_walk+0x70/0x900 > [ 2283.101553] [<ffffffff81286cf2>] path_openat+0xc2/0x500 > [ 2283.102282] [<ffffffff83a9a314>] ? _raw_spin_unlock_irqrestore+0x84/0xb0 > [ 2283.103506] [<ffffffff8128716c>] do_filp_open+0x3c/0xa0 > [ 2283.104282] [<ffffffff81296c11>] ? __alloc_fd+0x1e1/0x200 > [ 2283.105278] [<ffffffff81277c0c>] do_sys_open+0x11c/0x1c0 > [ 2283.106519] [<ffffffff81277ccc>] sys_open+0x1c/0x20 > [ 2283.107241] [<ffffffff81277d01>] sys_creat+0x11/0x20 > [ 2283.107975] [<ffffffff83a9be18>] tracesys+0xe1/0xe6 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Peter Hurley <peter@hurleysoftware.com> Date: Sun, 03 Mar 2013 20:06:18 -0500 > But regardless, this function __cannot__ sleep holding the tty_lock(). So drop it across the schedule(), but recheck the termios after regrabbing it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-03-03 at 21:36 -0500, David Miller wrote: > From: Peter Hurley <peter@hurleysoftware.com> > Date: Sun, 03 Mar 2013 20:06:18 -0500 > > > But regardless, this function __cannot__ sleep holding the tty_lock(). > > So drop it across the schedule(), but recheck the termios after > regrabbing it. I'll have to do some research on that. 1) The code is using a deliberate snapshot. if (tty->termios.c_cflag & CLOCAL) { IRDA_DEBUG(1, "%s(), doing CLOCAL!\n", __func__ ); do_clocal = 1; } ..... while (1) { ......... /* * Check if link is ready now. Even if CLOCAL is * specified, we cannot return before the IrCOMM link is * ready */ if (!test_bit(ASYNCB_CLOSING, &port->flags) && (do_clocal || tty_port_carrier_raised(port)) && self->state == IRCOMM_TTY_READY) { break; } 2) The only reason this driver isn't using tty_port_block_til_ready() is the lone state check: self->state == IRCOMM_TTY_READY I take it IRDA has some kind of virtual cabling protocol. But it's unclear why this can't be implemented in the driver without duplicating tty_port_block_til_ready(). For example, if the device can't do CLOCAL open (meaning no underlying device attached prior to open) then why specify that in the driver flags? Additionally, CLOCAL can be masked out by the driver's set_termios() method. And then it could implement the state check in its .carrier_raised() method. 3) The do_clocal snapshot is universally employed by every tty driver. I don't mean that as some kind of lame excuse. But if this should change, it should change across every tty driver with a really good reason. 4) Rechecking termios will change the way the user-space open() for this device behaves. And I need to think more on how that might or might not be a problem. 5) The code behavior pre-dates the 2005 check-in so I'll probably have to do some code archaeology. That's probably going to take some time. In the meantime, while reviewing that code, I noticed there's a handful of serious bugs in that one function that I'll send a patchset for. Plus, someone could be back to me on if and why the driver needs to be virtually cabled to open(). Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-03-03 at 21:36 -0500, David Miller wrote: > From: Peter Hurley <peter@hurleysoftware.com> > Date: Sun, 03 Mar 2013 20:06:18 -0500 > > > But regardless, this function __cannot__ sleep holding the tty_lock(). > > So drop it across the schedule(), but recheck the termios after > regrabbing it. I'll have to do some research on that. 1) The code is using a deliberate snapshot. if (tty->termios.c_cflag & CLOCAL) { IRDA_DEBUG(1, "%s(), doing CLOCAL!\n", __func__ ); do_clocal = 1; } ..... while (1) { ......... /* * Check if link is ready now. Even if CLOCAL is * specified, we cannot return before the IrCOMM link is * ready */ if (!test_bit(ASYNCB_CLOSING, &port->flags) && (do_clocal || tty_port_carrier_raised(port)) && self->state == IRCOMM_TTY_READY) { break; } 2) The only reason this driver isn't using tty_port_block_til_ready() is the lone state check: self->state == IRCOMM_TTY_READY I take it IRDA has some kind of virtual cabling protocol. But it's unclear why this can't be implemented in the driver without duplicating tty_port_block_til_ready(). For example, if the device can't do CLOCAL open (meaning no underlying device attached prior to open) then why specify that in the driver flags? Additionally, CLOCAL can be masked out by the driver's set_termios() method. And then it could implement the state check in its .carrier_raised() method. The net result of which would obviate the need for ircomm_tty_block_til_ready() at all. 3) The do_clocal snapshot is universally employed by every tty driver. I don't mean that as some kind of lame excuse. But if this should change, it should change across every tty driver with a really good reason. 4) Rechecking termios will change the way the user-space open() for this device behaves. And I need to think more on how that might or might not be a problem. 5) The code behavior pre-dates the 2005 check-in so I'll probably have to do some code archaeology. That's probably going to take some time. In the meantime, while reviewing that code, I noticed there's a handful of serious bugs in that one function that I'll send a patchset for. Plus, someone could be back to me on if and why the driver needs to be virtually cabled to open(). Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-03-03 at 23:24 -0500, Peter Hurley wrote: > On Sun, 2013-03-03 at 21:36 -0500, David Miller wrote: > > From: Peter Hurley <peter@hurleysoftware.com> > > Date: Sun, 03 Mar 2013 20:06:18 -0500 > > > > > But regardless, this function __cannot__ sleep holding the tty_lock(). > > > > So drop it across the schedule(), but recheck the termios after > > regrabbing it. > > I'll have to do some research on that. Still working on this... > In the meantime, while reviewing that code, I noticed there's a handful > of serious bugs in that one function that I'll send a patchset for. As promised. Peter Hurley (4): net/irda: Fix port open counts net/irda: Hold port lock while bumping blocked_open net/irda: Use barrier to set task state net/irda: Raise dtr in non-blocking open net/irda/ircomm/ircomm_tty.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)
From: Peter Hurley <peter@hurleysoftware.com> Date: Tue, 5 Mar 2013 11:09:03 -0500 > On Sun, 2013-03-03 at 23:24 -0500, Peter Hurley wrote: >> In the meantime, while reviewing that code, I noticed there's a handful >> of serious bugs in that one function that I'll send a patchset for. > > As promised. > > > Peter Hurley (4): > net/irda: Fix port open counts > net/irda: Hold port lock while bumping blocked_open > net/irda: Use barrier to set task state > net/irda: Raise dtr in non-blocking open Series looks good, all applied, thanks Peter! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c index 9a5fd3c..7844cb3 100644 --- a/net/irda/ircomm/ircomm_tty.c +++ b/net/irda/ircomm/ircomm_tty.c @@ -355,7 +355,9 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self, IRDA_DEBUG(1, "%s(%d):block_til_ready blocking on %s open_count=%d\n", __FILE__, __LINE__, tty->driver->name, port->count); + tty_unlock(tty); schedule(); + tty_lock(tty); } __set_current_state(TASK_RUNNING);
ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep might take a long time we can prevent other processes from accessing the tty, causing hung tasks and a dead tty. Diagnosed-by: Peter Hurley <peter@hurleysoftware.com> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> --- net/irda/ircomm/ircomm_tty.c | 2 ++ 1 file changed, 2 insertions(+)