* Should .dtr_rts do tty_port_tty_get?
@ 2011-03-25 10:45 Jiri Slaby
2011-03-25 10:45 ` [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference Jiri Slaby
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Slaby @ 2011-03-25 10:45 UTC (permalink / raw)
To: Greg KH; +Cc: Alan Cox, linux-serial, LKML, Jiri Slaby
Hi,
I'm playing with a serial device and got a nice oops. Maybe after some
weird stty's, I don't know. But it dies in uart_dtr_rts:
.loc 1 1535 0
movq (%rbx), %r13 # port_1(D)->tty, D.26746
...
.loc 1 1494 0
testb $2, 224(%r13) #, D.26746_10->flags
^^^^^^^^^
HERE
Because r13 (port->tty) is NULL. So the question is about the principle.
Should it call tty_port_tty_get or is it a bug in the TTY layer and
uart_dtr_rts should not be called with port->tty == NULL?
I'll attach a patch which does the former as a reply to this message.
BTW only serial_core needs tty in dtr_rts.
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference
2011-03-25 10:45 Should .dtr_rts do tty_port_tty_get? Jiri Slaby
@ 2011-03-25 10:45 ` Jiri Slaby
2011-03-25 11:00 ` Alan Cox
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Slaby @ 2011-03-25 10:45 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, jirislaby, Alan Cox
Under certain circumstances uart_dtr_rts might cause an oops. It dies
because port->tty is NULL. To fix this, let's take a reference of
port->tty by tty_port_tty_get. And if it is not there already, fail
gracefully.
After all, indeed drop the reference.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg KH <gregkh@suse.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
drivers/tty/serial/serial_core.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d6e7240..88a0472 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1531,8 +1531,12 @@ static void uart_dtr_rts(struct tty_port *port, int onoff)
* If this is the first open to succeed,
* adjust things to suit.
*/
- if (!test_and_set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags))
- uart_update_termios(port->tty, state);
+ if (!test_and_set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags)) {
+ struct tty_struct *tty = tty_port_tty_get(port);
+ if (tty)
+ uart_update_termios(tty, state);
+ tty_kref_put(tty);
+ }
}
else
uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference
2011-03-25 10:45 ` [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference Jiri Slaby
@ 2011-03-25 11:00 ` Alan Cox
2011-03-25 14:43 ` Jiri Slaby
0 siblings, 1 reply; 6+ messages in thread
From: Alan Cox @ 2011-03-25 11:00 UTC (permalink / raw)
To: Jiri Slaby; +Cc: gregkh, linux-serial, jirislaby
On Fri, 25 Mar 2011 11:45:56 +0100
Jiri Slaby <jslaby@suse.cz> wrote:
> Under certain circumstances uart_dtr_rts might cause an oops. It dies
> because port->tty is NULL. To fix this, let's take a reference of
> port->tty by tty_port_tty_get. And if it is not there already, fail
> gracefully.
The uart helper layer assumes here (and a couple of other spots) that the
IRQ handler for the tty takes the port lock.
I think the *right* fix is probably to pass port not port->tty into the
helper in the first place but that seems to ripple into a lot of drivers.
(passing port->tty to things that then go tty->port is braindead and
causes half the mess in the tty/serial code)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference
2011-03-25 11:00 ` Alan Cox
@ 2011-03-25 14:43 ` Jiri Slaby
2011-03-25 20:49 ` Alan Cox
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Slaby @ 2011-03-25 14:43 UTC (permalink / raw)
To: Alan Cox; +Cc: Jiri Slaby, gregkh, linux-serial
On 03/25/2011 12:00 PM, Alan Cox wrote:
> On Fri, 25 Mar 2011 11:45:56 +0100
> Jiri Slaby <jslaby@suse.cz> wrote:
>
>> Under certain circumstances uart_dtr_rts might cause an oops. It dies
>> because port->tty is NULL. To fix this, let's take a reference of
>> port->tty by tty_port_tty_get. And if it is not there already, fail
>> gracefully.
>
> The uart helper layer assumes here (and a couple of other spots) that the
> IRQ handler for the tty takes the port lock.
The oopsing path is through open BTW:
-> uart_open
-> tty_port_block_til_ready
-> tty_port_raise_dtr_rts
-> uart_dtr_rts
> I think the *right* fix is probably to pass port not port->tty into the
> helper in the first place but that seems to ripple into a lot of drivers.
>
> (passing port->tty to things that then go tty->port is braindead and
> causes half the mess in the tty/serial code)
I seem to miss the point. uart_update_termios needs tty, not port.
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference
2011-03-25 14:43 ` Jiri Slaby
@ 2011-03-25 20:49 ` Alan Cox
2011-03-29 15:10 ` Jiri Slaby
0 siblings, 1 reply; 6+ messages in thread
From: Alan Cox @ 2011-03-25 20:49 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Jiri Slaby, gregkh, linux-serial
> > The uart helper layer assumes here (and a couple of other spots) that the
> > IRQ handler for the tty takes the port lock.
>
> The oopsing path is through open BTW:
> -> uart_open
> -> tty_port_block_til_ready
> -> tty_port_raise_dtr_rts
> -> uart_dtr_rts
>
> > I think the *right* fix is probably to pass port not port->tty into the
> > helper in the first place but that seems to ripple into a lot of drivers.
> >
> > (passing port->tty to things that then go tty->port is braindead and
> > causes half the mess in the tty/serial code)
>
> I seem to miss the point. uart_update_termios needs tty, not port.
Actually I think it needs shooting having looked more closely
It does 3 things
1. It copies a flag across as part of a console hack. That should be done
elsewhere - eg in uart_startup
2. It sets the speed, which was already done by uart_startup
3. It goes poking around in the CBAUD flag before playing with DTR and
RTS which will have no effect as we *already* set the flags in
uart_dtr_rts
Am I missing something ?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference
2011-03-25 20:49 ` Alan Cox
@ 2011-03-29 15:10 ` Jiri Slaby
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Slaby @ 2011-03-29 15:10 UTC (permalink / raw)
To: Alan Cox; +Cc: Jiri Slaby, gregkh, linux-serial, Arnd Bergmann
On 03/25/2011 09:49 PM, Alan Cox wrote:
>>> The uart helper layer assumes here (and a couple of other spots) that the
>>> IRQ handler for the tty takes the port lock.
>>
>> The oopsing path is through open BTW:
>> -> uart_open
>> -> tty_port_block_til_ready
>> -> tty_port_raise_dtr_rts
>> -> uart_dtr_rts
>>
>>> I think the *right* fix is probably to pass port not port->tty into the
>>> helper in the first place but that seems to ripple into a lot of drivers.
>>>
>>> (passing port->tty to things that then go tty->port is braindead and
>>> causes half the mess in the tty/serial code)
>>
>> I seem to miss the point. uart_update_termios needs tty, not port.
>
>
> Actually I think it needs shooting having looked more closely
Hmm, looking more closely, it's broken and should be converted to
tty_port in full. I'll take a look if I can do something about that.
> It does 3 things
>
> 1. It copies a flag across as part of a console hack. That should be done
> elsewhere - eg in uart_startup
Perhaps.
> 2. It sets the speed, which was already done by uart_startup
Note that uart_startup is called with init_hw = 0 from uart_open. So the
speed is not set yet. So we should set the speed in uart_startup
unconditionally, I guess.
> 3. It goes poking around in the CBAUD flag before playing with DTR and
> RTS which will have no effect as we *already* set the flags in
> uart_dtr_rts
ACK
.dtr_rts did only dtr_rts when added by you. Then Arnd moved
uart_update_termios into that in 3f582b8c11.
I'll post a patch as a short-term fix shortly.
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-29 15:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-25 10:45 Should .dtr_rts do tty_port_tty_get? Jiri Slaby
2011-03-25 10:45 ` [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference Jiri Slaby
2011-03-25 11:00 ` Alan Cox
2011-03-25 14:43 ` Jiri Slaby
2011-03-25 20:49 ` Alan Cox
2011-03-29 15:10 ` Jiri Slaby
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).