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

[PATCH 1/3] serial: core, move termios handling to uart_startup

[Jiri Slaby]

We should not fiddle with speed and cflags in .dtr_rts hook. Actually
we might not have tty at that moment already.

So move the console cflag copy and speed setup into uart_startup.
Actually the speed setup is already there, but we need to call it
unconditionally (uart_startup is called from uart_open with hw_init =
0).

This means we move uart_change_speed before dtr/rts setup in .dtr_rts.
But this should not matter as the setup should be called after
uart_change_speed anyway.
Before:                             After:
dtr/rts setup (dtr_rts)             uart_change_speed (startup)
uart_change_speed (update_termios)  dtr/rts setup (dtr_rts)
dtr/rts setup (update_termios)      dtr/rts setup (update_termios)

The second setup will dismiss with the next patch.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 drivers/tty/serial/serial_core.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d6e7240..47657cf 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -172,12 +172,16 @@ static int uart_startup(struct tty_struct *tty, struct uart_state *state, int in
 
 	retval = uport->ops->startup(uport);
 	if (retval == 0) {
-		if (init_hw) {
-			/*
-			 * Initialise the hardware port settings.
-			 */
-			uart_change_speed(tty, state, NULL);
+		if (uart_console(uport) && uport->cons->cflag) {
+			tty->termios->c_cflag = uport->cons->cflag;
+			uport->cons->cflag = 0;
+		}
+		/*
+		 * Initialise the hardware port settings.
+		 */
+		uart_change_speed(tty, state, NULL);
 
+		if (init_hw) {
 			/*
 			 * Setup the RTS and DTR signals once the
 			 * port is open and ready to respond.
@@ -1481,11 +1485,6 @@ static void uart_update_termios(struct tty_struct *tty,
 {
 	struct uart_port *port = state->uart_port;
 
-	if (uart_console(port) && port->cons->cflag) {
-		tty->termios->c_cflag = port->cons->cflag;
-		port->cons->cflag = 0;
-	}
-
 	/*
 	 * If the device failed to grab its irq resources,
 	 * or some other error occurred, don't try to talk
@@ -1493,11 +1492,6 @@ static void uart_update_termios(struct tty_struct *tty,
 	 */
 	if (!(tty->flags & (1 << TTY_IO_ERROR))) {
 		/*
-		 * Make termios settings take effect.
-		 */
-		uart_change_speed(tty, state, NULL);
-
-		/*
 		 * And finally enable the RTS and DTR signals.
 		 */
 		if (tty->termios->c_cflag & CBAUD)
-- 
1.7.4.1

[PATCH 2/3] serial: core, do not set DTR/RTS twice on startup

[Jiri Slaby]

In .dtr_rts we do:
  uart_set_mctrl(uport, TIOCM_DTR | TIOCM_RTS)
and call uart_update_termios. It does:
  uart_set_mctrl(port, TIOCM_DTR | TIOCM_RTS)
once again. As the only callsite of uart_update_termios is .dtr_rts,
remove the uart_set_mctrl from uart_update_termios to not set it twice.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 drivers/tty/serial/serial_core.c |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 47657cf..3aae8ed 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1483,20 +1483,6 @@ static void uart_hangup(struct tty_struct *tty)
 static void uart_update_termios(struct tty_struct *tty,
 						struct uart_state *state)
 {
-	struct uart_port *port = state->uart_port;
-
-	/*
-	 * If the device failed to grab its irq resources,
-	 * or some other error occurred, don't try to talk
-	 * to the port hardware.
-	 */
-	if (!(tty->flags & (1 << TTY_IO_ERROR))) {
-		/*
-		 * And finally enable the RTS and DTR signals.
-		 */
-		if (tty->termios->c_cflag & CBAUD)
-			uart_set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
-	}
 }
 
 static int uart_carrier_raised(struct tty_port *port)
-- 
1.7.4.1

[PATCH 3/3] serial: core, remove uart_update_termios

[Jiri Slaby]

Now, uart_update_termios is empty, so it's time to remove it. We no
longer need a live tty in .dtr_rts. So this should prune all the bugs
where tty is zeroed in port->tty during tty_port_block_til_ready.

There is one thing to note. We don't set ASYNC_NORMAL_ACTIVE now. It's
because this is done already in tty_port_block_til_ready.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 drivers/tty/serial/serial_core.c |   25 +------------------------
 1 files changed, 1 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3aae8ed..d4bd465 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1470,21 +1470,6 @@ static void uart_hangup(struct tty_struct *tty)
 	mutex_unlock(&port->mutex);
 }
 
-/**
- *	uart_update_termios	-	update the terminal hw settings
- *	@tty: tty associated with UART
- *	@state: UART to update
- *
- *	Copy across the serial console cflag setting into the termios settings
- *	for the initial open of the port.  This allows continuity between the
- *	kernel settings, and the settings init adopts when it opens the port
- *	for the first time.
- */
-static void uart_update_termios(struct tty_struct *tty,
-						struct uart_state *state)
-{
-}
-
 static int uart_carrier_raised(struct tty_port *port)
 {
 	struct uart_state *state = container_of(port, struct uart_state, port);
@@ -1504,16 +1489,8 @@ static void uart_dtr_rts(struct tty_port *port, int onoff)
 	struct uart_state *state = container_of(port, struct uart_state, port);
 	struct uart_port *uport = state->uart_port;
 
-	if (onoff) {
+	if (onoff)
 		uart_set_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
-
-		/*
-		 * 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);
-	}
 	else
 		uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
 }
-- 
1.7.4.1