Should .dtr_rts do tty_port_tty_get?

Should .dtr_rts do tty_port_tty_get?

[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

[PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference

[Jiri Slaby]

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

Re: [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference

[Alan Cox]

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)

Re: [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference

[Jiri Slaby]

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

Re: [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference

[Alan Cox]

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

Re: [PATCH 1/1] TTY: serial_core, fix dtr_rts NULL dereference

[Jiri Slaby]

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