[PATCH 0/4] tty/serial: at91: fixes dealing with port shutdown

[PATCH 0/4] tty/serial: at91: fixes dealing with port shutdown

[Nicolas Ferre]

Hi,

These four fixes are pretty important for our atmel_serial driver as they
deal with closing/re-opening of ports. They fix race condition and
null pointers dereference.
I added the "stable" tag to each of them (v3.12).

Even if we are late in the development cycle, can you please consider including
these patches for 3.13?

Thanks, best regards,
   Nicolas Ferre

Marek Roszko (3):
  tty/serial: at91: Handle shutdown more safely
  tty/serial: at91: fix race condition in atmel_serial_remove
  tty/serial: at91: prevent null dereference in tasklet function

Mark Deneen (1):
  tty/serial: at91: reset rx_ring when port is shutdown

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

-- 
1.8.2.2

[PATCH 1/4] tty/serial: at91: Handle shutdown more safely

[Nicolas Ferre]

From: Marek Roszko <mark.roszko@gmail.com>

Interrupts were being cleaned up late in the shutdown handler, it is possible
that an interrupt can occur and schedule a tasklet that runs after the port is
cleaned up. There is a null dereference due to this race condition with the
following stacktrace:

[<c02092b0>] (atmel_tasklet_func+0x514/0x814) from [<c001fd34>] (tasklet_action+0x70/0xa8)
[<c001fd34>] (tasklet_action+0x70/0xa8) from [<c001f60c>] (__do_softirq+0x90/0x144)
[<c001f60c>] (__do_softirq+0x90/0x144) from [<c001fa18>] (irq_exit+0x40/0x4c)
[<c001fa18>] (irq_exit+0x40/0x4c) from [<c000e298>] (handle_IRQ+0x64/0x84)
[<c000e298>] (handle_IRQ+0x64/0x84) from [<c000d6c0>] (__irq_svc+0x40/0x50)
[<c000d6c0>] (__irq_svc+0x40/0x50) from [<c0208060>] (atmel_rx_dma_release+0x88/0xb8)
[<c0208060>] (atmel_rx_dma_release+0x88/0xb8) from [<c0209740>] (atmel_shutdown+0x104/0x160)
[<c0209740>] (atmel_shutdown+0x104/0x160) from [<c0205e8c>] (uart_port_shutdown+0x2c/0x38)

Signed-off-by: Marek Roszko <mark.roszko@gmail.com>
Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
Cc: <stable@vger.kernel.org> # v3.12
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/tty/serial/atmel_serial.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index c7d99af46a96..48ea47a32d5f 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1650,12 +1650,24 @@ static int atmel_startup(struct uart_port *port)
 static void atmel_shutdown(struct uart_port *port)
 {
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+
 	/*
-	 * Ensure everything is stopped.
+	 * Clear out any scheduled tasklets before
+	 * we destroy the buffers
+	 */
+	tasklet_kill(&atmel_port->tasklet);
+
+	/*
+	 * Ensure everything is stopped and
+	 * disable all interrupts, port and break condition.
 	 */
 	atmel_stop_rx(port);
 	atmel_stop_tx(port);
 
+	UART_PUT_CR(port, ATMEL_US_RSTSTA);
+	UART_PUT_IDR(port, -1);
+
+
 	/*
 	 * Shut-down the DMA.
 	 */
@@ -1665,12 +1677,6 @@ static void atmel_shutdown(struct uart_port *port)
 		atmel_port->release_tx(port);
 
 	/*
-	 * Disable all interrupts, port and break condition.
-	 */
-	UART_PUT_CR(port, ATMEL_US_RSTSTA);
-	UART_PUT_IDR(port, -1);
-
-	/*
 	 * Free the interrupt
 	 */
 	free_irq(port->irq, port);
-- 
1.8.2.2

[PATCH 2/4] tty/serial: at91: fix race condition in atmel_serial_remove

[Nicolas Ferre]

From: Marek Roszko <mark.roszko@gmail.com>

The _remove callback could be called when a tasklet is scheduled. tasklet_kill
was called inside the function in order to free up any scheduled tasklets.
However it was called after uart_remove_one_port which destroys tty references
needed in the port for atmel_tasklet_func.
Simply putting the tasklet_kill at the start of the function will prevent this
conflict.

Signed-off-by: Marek Roszko <mark.roszko@gmail.com>
Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
Cc: <stable@vger.kernel.org> # v3.12
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/tty/serial/atmel_serial.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 48ea47a32d5f..c421d11b3d4c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2447,11 +2447,12 @@ static int atmel_serial_remove(struct platform_device *pdev)
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 	int ret = 0;
 
+	tasklet_kill(&atmel_port->tasklet);
+
 	device_init_wakeup(&pdev->dev, 0);
 
 	ret = uart_remove_one_port(&atmel_uart, port);
 
-	tasklet_kill(&atmel_port->tasklet);
 	kfree(atmel_port->rx_ring.buf);
 
 	/* "port" is allocated statically, so we shouldn't free it */
-- 
1.8.2.2

[PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function

[Nicolas Ferre]

From: Marek Roszko <mark.roszko@gmail.com>

Something asks a tasklet to be scheduled when the uart port is closed.
Need to supress the kernel panic for now by checking if the port is NULL or
not.

Signed-off-by: Marek Roszko <mark.roszko@gmail.com>
Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
Cc: <stable@vger.kernel.org> # v3.12
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/tty/serial/atmel_serial.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index c421d11b3d4c..6e68486c83cb 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1360,6 +1360,10 @@ static void atmel_tasklet_func(unsigned long data)
 	unsigned int status;
 	unsigned int status_change;
 
+	if(!port->state || !port->state->port.tty)
+		/* uart has been closed */
+		return;
+
 	/* The interrupt handler does not take the lock */
 	spin_lock(&port->lock);
 
-- 
1.8.2.2

Re: [PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function

[Greg KH]

On Tue, Jan 07, 2014 at 11:45:08AM +0100, Nicolas Ferre wrote:
> From: Marek Roszko <mark.roszko@gmail.com>
> 
> Something asks a tasklet to be scheduled when the uart port is closed.

What is that something?  Shouldn't you track that down and find the real
problem here?

> Need to supress the kernel panic for now by checking if the port is NULL or
> not.
> 
> Signed-off-by: Marek Roszko <mark.roszko@gmail.com>
> Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
> Cc: <stable@vger.kernel.org> # v3.12
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index c421d11b3d4c..6e68486c83cb 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -1360,6 +1360,10 @@ static void atmel_tasklet_func(unsigned long data)
>  	unsigned int status;
>  	unsigned int status_change;
>  
> +	if(!port->state || !port->state->port.tty)
> +		/* uart has been closed */
> +		return;

Did you really test this?

How about running it through checkpatch?

thanks,

greg k-h

Re: [PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function

[Greg KH]

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Tue, Jan 07, 2014 at 08:52:34PM -0500, Mark Roszko wrote:
> This patch I was somewhat hesitant to pushing to Atmel when I did the other two
> patches.

Then why did you?  :)

> The main issue is the use of systemd causes the serial driver to somehow get
> out of sync on startups randomly. i.e. on one bootup it's fine, and on other
> it'll kernel panic. 
> It occurs because systemd causes the startup and shutdown driver ops to be
> fired in rapid succession. 

How does systemd cause this?  Is this when the serial port is being used
as a console or something else?

> Every single service start message causes the _startup callback first, then the
> message prints and _shutdown callbacks fires. 

So something is opening and closing the port quickly?  That should be
easy to test without systemd.

Any console involved?

> And a kernel panic always occurs somewhere during the service status output,
> never before when it's just the kernel startup and never after once systemd
> finishes and getty for example takes over. 
> 
> The stacktrace looked like this:
> Unable to handle kernel NULL pointer dereference at virtual address 00000088
> pgd = c0004000
> [00000088] *pgd=00000000
> Internal error: Oops: 17 [#1] ARM
> Modules linked in: autofs4
> CPU: 0    Not tainted  (3.6.9-yocto-standard #1)

3.6.9 is _very_ old, loads of things have happened in the tty layer
since then, can you duplicate this on 3.12 or 3.13-rc?


> PC is at tty_wakeup+0x8/0x58
> LR is at atmel_tasklet_func+0x238/0x80c
> pc : [<c01efd84>]    lr : [<c0208fc0>]    psr: a00f0013
> sp : df84ff28  ip : 00000000  fp : 00000100
> r10: c05d1ec0  r9 : 04208040  r8 : c05d1e80
> r7 : 0000000a  r6 : 00000000  r5 : dedf7c00  r4 : 00000000
> r3 : dedf7c00  r2 : 00000000  r1 : 600f0013  r0 : 00000000
> Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> Control: 10c53c7d  Table: 3fb0c059  DAC: 00000015
> Process ksoftirqd/0 (pid: 3, stack limit = 0xdf84e2f0)
> Stack: (0xdf84ff28 to 0xdf850000)
> ff20:                   c05e4378 dedf7c00 00000000 c0208fc0 c05aa458 df8496b0
> ff40: df849680 c05aa458 df8496b0 c00394a8 c0039420 c05a8d04 00000000 00000000
> ff60: 0000000a c05d1e80 04208040 c05d1ec0 00000100 c001fd34 00000009 00000018
> ff80: df84e000 c001f60c df84ffbc c03f8e60 00000013 00000006 00000000 c05d1e80
> ffa0: df84e000 00000000 00000013 00000000 00000000 00000000 00000000 c001f728
> ffc0: df84bf3c 00000000 c001f6c0 c0030870 00000000 00000000 00000000 00000000
> ffe0: df84ffe0 df84ffe0 df84bf3c c00307f0 c000e348 c000e348 fff73fbf 3fbeffff
> [<c01efd84>] (tty_wakeup+0x8/0x58) from [<c0208fc0>] (atmel_tasklet_func+0x238/
> 0x80c)
> [<c0208fc0>] (atmel_tasklet_func+0x238/0x80c) from [<c001fd34>]
> (tasklet_action+0x70/0xa8)
> [<c001fd34>] (tasklet_action+0x70/0xa8) from [<c001f60c>] (__do_softirq+0x90/
> 0x144)
> [<c001f60c>] (__do_softirq+0x90/0x144) from [<c001f728>] (run_ksoftirqd+0x68/
> 0x104)
> [<c001f728>] (run_ksoftirqd+0x68/0x104) from [<c0030870>] (kthread+0x80/0x90)
> [<c0030870>] (kthread+0x80/0x90) from [<c000e348>] (kernel_thread_exit+0x0/0x8)
> Code: e8bd8070 c05ac0b8 e92d4070 e1a04000 (e5903088)
> ---[ end trace 15b8a9aeaf7b457f ]---
> 
> 
> Testing wise I originally used a BUG_ON statement in atmel_tasklet_func to
> panic before the null deference hit. BUG_ON confirmed that tty was NULL at the
> very start of the tasklet being called. 

And did you test that this patch actually fixed it?

> The atmel_shutdown function should be killing the tasklet (after patch "Handle
> shutdown more safely") and does disable interrupts so I've been at a loss at
> where the race condition was occurring that a tasklet could escape beyond
> shutdown.

Are you sure you aren't just racing with shutdown?  Need a lock
somewhere?

greg k-h

[PATCH 4/4] tty/serial: at91: reset rx_ring when port is shutdown

[Nicolas Ferre]

From: Mark Deneen <mdeneen@gmail.com>

When using RX DMA, the driver won't pass any data to the uart layer
until the buffer is flipped. When the port is shutdown, the dma buffers
are unmapped, but the head and tail of the ring buffer are not reseted.
Since the serial console will keep the port open, this will only
present itself when the uart is not shared.

To reproduce the issue, with an unpatched driver, run a getty on /dev/ttyS0
with no serial console and exit. Getty will exit, and when the new one returns
you will be unable to log in.  If you hold down a key long enough to fill the
DMA buffer and flip it, you can then log in.

Signed-off-by: Mark Deneen <mdeneen@gmail.com>
Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
[nicolas.ferre@atmel.com: adapt to mainline kernel, handle !DMA case]
Cc: <stable@vger.kernel.org> # v3.12
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/tty/serial/atmel_serial.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 6e68486c83cb..2d925455c1ec 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1681,6 +1681,12 @@ static void atmel_shutdown(struct uart_port *port)
 		atmel_port->release_tx(port);
 
 	/*
+	 * Reset ring buffer pointers
+	 */
+	atmel_port->rx_ring.head = 0;
+	atmel_port->rx_ring.tail = 0;
+
+	/*
 	 * Free the interrupt
 	 */
 	free_irq(port->irq, port);
-- 
1.8.2.2

Re: [PATCH 0/4] tty/serial: at91: fixes dealing with port shutdown

[Nicolas Ferre]

On 07/01/2014 11:41, Nicolas Ferre :
> Hi,

Sorry for the noise: I messed up with git-send-email...

I re-sent the whole series.

Bye,


> These four fixes are pretty important for our atmel_serial driver as they
> deal with closing/re-opening of ports. They fix race condition and
> null pointers dereference.
> I added the "stable" tag to each of them (v3.12).
> 
> Even if we are late in the development cycle, can you please consider including
> these patches for 3.13?
> 
> Thanks, best regards,
>    Nicolas Ferre
> 
> Marek Roszko (3):
>   tty/serial: at91: Handle shutdown more safely
>   tty/serial: at91: fix race condition in atmel_serial_remove
>   tty/serial: at91: prevent null dereference in tasklet function
> 
> Mark Deneen (1):
>   tty/serial: at91: reset rx_ring when port is shutdown
> 
>  drivers/tty/serial/atmel_serial.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 


-- 
Nicolas Ferre