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 <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
net/irda/ircomm/ircomm_tty.c | 2 ++
1 file changed, 2 insertions(+)
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);
--
1.8.1.4
From: Sasha Levin <[email protected]>
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 <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
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.
On 03/03/2013 05:47 PM, David Miller wrote:
> From: Sasha Levin <[email protected]>
> 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 <[email protected]>
>> Signed-off-by: Sasha Levin <[email protected]>
>
> 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
On Sun, 2013-03-03 at 17:47 -0500, David Miller wrote:
> From: Sasha Levin <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Sasha Levin <[email protected]>
>
> 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
From: Sasha Levin <[email protected]>
Date: Sun, 03 Mar 2013 18:17:38 -0500
> On 03/03/2013 05:47 PM, David Miller wrote:
>> From: Sasha Levin <[email protected]>
>> 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 <[email protected]>
>>> Signed-off-by: Sasha Levin <[email protected]>
>>
>> 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.
From: Peter Hurley <[email protected]>
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.
On Sun, 2013-03-03 at 19:33 -0500, David Miller wrote:
> From: Peter Hurley <[email protected]>
> 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
On Sun, 2013-03-03 at 17:47 -0500, David Miller wrote:
> From: Sasha Levin <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Sasha Levin <[email protected]>
>
> 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 <[email protected]> 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
From: Peter Hurley <[email protected]>
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.
On Sun, 2013-03-03 at 21:36 -0500, David Miller wrote:
> From: Peter Hurley <[email protected]>
> 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
On Sun, 2013-03-03 at 21:36 -0500, David Miller wrote:
> From: Peter Hurley <[email protected]>
> 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
Without a memory and compiler barrier, the task state change
can migrate relative to the condition testing in a blocking loop.
However, the task state change must be visible across all cpus
prior to testing those conditions. Failing to do this can result
in the familiar 'lost wakeup' and this task will hang until killed.
Signed-off-by: Peter Hurley <[email protected]>
---
net/irda/ircomm/ircomm_tty.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index d282bbe..522543d 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -324,7 +324,7 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
if (tty->termios.c_cflag & CBAUD)
tty_port_raise_dtr_rts(port);
- current->state = TASK_INTERRUPTIBLE;
+ set_current_state(TASK_INTERRUPTIBLE);
if (tty_hung_up_p(filp) ||
!test_bit(ASYNCB_INITIALIZED, &port->flags)) {
--
1.8.1.2
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 <[email protected]>
> > 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(-)
--
1.8.1.2
Saving the port count bump is unsafe. If the tty is hung up while
this open was blocking, the port count is zeroed.
Explicitly check if the tty was hung up while blocking, and correct
the port count if not.
Signed-off-by: Peter Hurley <[email protected]>
---
net/irda/ircomm/ircomm_tty.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 9a5fd3c..1721dc7 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -280,7 +280,7 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
struct tty_port *port = &self->port;
DECLARE_WAITQUEUE(wait, current);
int retval;
- int do_clocal = 0, extra_count = 0;
+ int do_clocal = 0;
unsigned long flags;
IRDA_DEBUG(2, "%s()\n", __func__ );
@@ -315,10 +315,8 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
__FILE__, __LINE__, tty->driver->name, port->count);
spin_lock_irqsave(&port->lock, flags);
- if (!tty_hung_up_p(filp)) {
- extra_count = 1;
+ if (!tty_hung_up_p(filp))
port->count--;
- }
spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open++;
@@ -361,12 +359,10 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
__set_current_state(TASK_RUNNING);
remove_wait_queue(&port->open_wait, &wait);
- if (extra_count) {
- /* ++ is not atomic, so this should be protected - Jean II */
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ if (!tty_hung_up_p(filp))
port->count++;
- spin_unlock_irqrestore(&port->lock, flags);
- }
+ spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open--;
IRDA_DEBUG(1, "%s(%d):block_til_ready after blocking on %s open_count=%d\n",
--
1.8.1.2
DTR/RTS need to be raised, regardless of the open() mode, but not
if the port has already shutdown.
Signed-off-by: Peter Hurley <[email protected]>
---
net/irda/ircomm/ircomm_tty.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 522543d..362ba47 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -289,8 +289,15 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
* If non-blocking mode is set, or the port is not enabled,
* then make the check up front and then exit.
*/
- if (filp->f_flags & O_NONBLOCK || tty->flags & (1 << TTY_IO_ERROR)){
- /* nonblock mode is set or port is not enabled */
+ if (test_bit(TTY_IO_ERROR, &tty->flags)) {
+ port->flags |= ASYNC_NORMAL_ACTIVE;
+ return 0;
+ }
+
+ if (filp->f_flags & O_NONBLOCK) {
+ /* nonblock mode is set */
+ if (tty->termios.c_cflag & CBAUD)
+ tty_port_raise_dtr_rts(port);
port->flags |= ASYNC_NORMAL_ACTIVE;
IRDA_DEBUG(1, "%s(), O_NONBLOCK requested!\n", __func__ );
return 0;
--
1.8.1.2
Although tty_lock() already protects concurrent update to
blocked_open, that fails to meet the separation-of-concerns between
tty_port and tty.
Signed-off-by: Peter Hurley <[email protected]>
---
net/irda/ircomm/ircomm_tty.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 1721dc7..d282bbe 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -317,8 +317,8 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
spin_lock_irqsave(&port->lock, flags);
if (!tty_hung_up_p(filp))
port->count--;
- spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open++;
+ spin_unlock_irqrestore(&port->lock, flags);
while (1) {
if (tty->termios.c_cflag & CBAUD)
@@ -362,8 +362,8 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
spin_lock_irqsave(&port->lock, flags);
if (!tty_hung_up_p(filp))
port->count++;
- spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open--;
+ spin_unlock_irqrestore(&port->lock, flags);
IRDA_DEBUG(1, "%s(%d):block_til_ready after blocking on %s open_count=%d\n",
__FILE__, __LINE__, tty->driver->name, port->count);
--
1.8.1.2
From: Peter Hurley <[email protected]>
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!