[RFC] tty/serial: at91: disable uart timer at start of shutdown

Commit Message

Nicolas Ferre Jan. 8, 2014, 10:28 a.m. UTC

From: Marek Roszko <[e-mail]>

The uart timer will schedule a tasklet when it fires. It is possible that it
can fire inside _shutdown before it is killed in the dma and pdc cleanup
routines. This causes a tasklet that exists after the port is shutdown, so when
the kernel finally executes it, it panics as the tty port is NULL.

This is a somewhat rare condition but its possible if a program keeps on
opening/closing the port. It has been observed in particular with systemd
boot messages that were causing a kernel panic because of this behavior.

Moving the timer deletion to the beginning of the function stops a tasklet from
being scheduled unexpectedly.

Signed-off-by: Marek Roszko <[e-mail]>
Cc: stable <[e-mail]> # v3.12
[[e-mail]: modify commit message, call setup_timer() in any case]
Signed-off-by: Nicolas Ferre <[e-mail]>
---
Hi Marek,

This is my modified version of your patch.
* I adapted it to the mainline kernel
* I rewrote the commit message
* I used setup_timer() / del_timer_sync() in any case, even the
  !DMA/!PDC case. Otherwise, the time would have been deleted in
  an uninitialized state which may lead to errors (I didn't checked
  though).

Can you please tell me how do you feel with this patch (and the commit message)
and if it solves the issue that you encounter on your side.

Best regards,

 drivers/tty/serial/atmel_serial.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Comments

Mark Roszko Jan. 8, 2014, 2:29 p.m. UTC | #1

**Sorry for the duplicate mail, I forgot to force plain-text mode in
gmail or the mailing list rejects the mails. Sigh, I need to buy my
own email server at this rate to even deal with the mailing lists and
proper etiquette (no quoteS) :(

Ah, I forgot there was that third mode(no dma and no pdc). Yea it's
probably a bad idea to play with fire and call del_timer_sync on a
non-initialized timer struct.

Alternatively to setting it up in all modes, the mode could also be
checked in _shutdown:
if ( ( atmel_use_dma_rx(port) || atmel_use_pdc_rx(port) )
           && !atmel_port->is_usart)
      del_timer_sync(&atmel_port->uart_timer);

But it doesn't hurt to call setup_timer in the other case either
because it just initializes the fields in timer_list. So I'm not sure
what best practices dictates here.

Commit messages looks fine, thanks for fixing that, I'm still getting
used to the linux dev process and I also do it at 3am at night.

Mark Roszko Jan. 9, 2014, 7:08 a.m. UTC | #2

Just to sumarize the bug after poking before the patch:
   systemd calls open/close on /dev/ttyS0 per line
   atmel_shutdown executes on close
   uart_timer_callback may fire during shutdown before timer is killed
   tasklet gets scheduled after tasklet_kill
   atmel_shutdown returns back to uart_close
dice roll:
   if the tasklet executes before uart_close kills the tty references,
the kernel is fine           <--- I saw this happening more often than
panics, around once every 3 resets
   if the tasklet executes after uart_close kills the tty references, it panics

I tested the patch by old fashion way of manual board resets with a
total of 70 resets each on two individual SAMA5D34-EK boards I have in
my possession programmed with the same patched kernel image and
rootfs. They did not panic once with the patch applied. Typically
before the patch the panic would occur around once every 10 resets if
not more often to the point it occured for 4 resets straight.

I have been trying to automate the testing, even If I make sure
nothing is using ttyS0 and then run a program that spams open()
writev() and close() (systemd uses the syscalls) to print messages, it
will never panic so I'm possibly misunderstanding how the driver
startup/shutdown ops get triggered(which should just be open/close
respectively) or variable length of messages from systemd and timing
between messages contributes to being unlucky and having the timer
fire at the wrong time.

But from manual testing I'm pretty confident the bug is fixed.

Nicolas Ferre Jan. 10, 2014, 9:34 a.m. UTC | #3

On 09/01/2014 08:08, Mark Roszko :
> Just to sumarize the bug after poking before the patch:
>    systemd calls open/close on /dev/ttyS0 per line
>    atmel_shutdown executes on close
>    uart_timer_callback may fire during shutdown before timer is killed
>    tasklet gets scheduled after tasklet_kill
>    atmel_shutdown returns back to uart_close
> dice roll:
>    if the tasklet executes before uart_close kills the tty references,
> the kernel is fine           <--- I saw this happening more often than
> panics, around once every 3 resets
>    if the tasklet executes after uart_close kills the tty references, it panics
> 
> I tested the patch by old fashion way of manual board resets with a
> total of 70 resets each on two individual SAMA5D34-EK boards I have in
> my possession programmed with the same patched kernel image and
> rootfs. They did not panic once with the patch applied. Typically
> before the patch the panic would occur around once every 10 resets if
> not more often to the point it occured for 4 resets straight.
> 
> I have been trying to automate the testing, even If I make sure
> nothing is using ttyS0 and then run a program that spams open()
> writev() and close() (systemd uses the syscalls) to print messages, it
> will never panic so I'm possibly misunderstanding how the driver
> startup/shutdown ops get triggered(which should just be open/close
> respectively) or variable length of messages from systemd and timing
> between messages contributes to being unlucky and having the timer
> fire at the wrong time.
> 
> But from manual testing I'm pretty confident the bug is fixed.

Nice, thanks for the detailed feedback.

So I send the patch for inclusion to Greg.

Thanks for your help Mark. Best regards,

Patch

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 2b6ac1be00d3..a49f10d269b2 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -825,9 +825,6 @@  static void atmel_release_rx_dma(struct uart_port *port)
 	atmel_port->desc_rx = NULL;
 	atmel_port->chan_rx = NULL;
 	atmel_port->cookie_rx = -EINVAL;
-
-	if (!atmel_port->is_usart)
-		del_timer_sync(&atmel_port->uart_timer);
 }
 
 static void atmel_rx_from_dma(struct uart_port *port)
@@ -1229,9 +1226,6 @@  static void atmel_release_rx_pdc(struct uart_port *port)
 				 DMA_FROM_DEVICE);
 		kfree(pdc->buf);
 	}
-
-	if (!atmel_port->is_usart)
-		del_timer_sync(&atmel_port->uart_timer);
 }
 
 static void atmel_rx_from_pdc(struct uart_port *port)
@@ -1604,12 +1598,13 @@  static int atmel_startup(struct uart_port *port)
 	/* enable xmit & rcvr */
 	UART_PUT_CR(port, ATMEL_US_TXEN | ATMEL_US_RXEN);
 
+	setup_timer(&atmel_port->uart_timer,
+			atmel_uart_timer_callback,
+			(unsigned long)port);
+
 	if (atmel_use_pdc_rx(port)) {
 		/* set UART timeout */
 		if (!atmel_port->is_usart) {
-			setup_timer(&atmel_port->uart_timer,
-					atmel_uart_timer_callback,
-					(unsigned long)port);
 			mod_timer(&atmel_port->uart_timer,
 					jiffies + uart_poll_timeout(port));
 		/* set USART timeout */
@@ -1624,9 +1619,6 @@  static int atmel_startup(struct uart_port *port)
 	} else if (atmel_use_dma_rx(port)) {
 		/* set UART timeout */
 		if (!atmel_port->is_usart) {
-			setup_timer(&atmel_port->uart_timer,
-					atmel_uart_timer_callback,
-					(unsigned long)port);
 			mod_timer(&atmel_port->uart_timer,
 					jiffies + uart_poll_timeout(port));
 		/* set USART timeout */
@@ -1652,6 +1644,12 @@  static void atmel_shutdown(struct uart_port *port)
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 
 	/*
+	 * Prevent any tasklets being scheduled during
+	 * cleanup
+	 */
+	del_timer_sync(&atmel_port->uart_timer);
+
+	/*
 	 * Clear out any scheduled tasklets before
 	 * we destroy the buffers
 	 */