[PATCH] tty: serial: amba-pl011: cancel RS485 trigger hrtimers before removal

[PATCH] tty: serial: amba-pl011: cancel RS485 trigger hrtimers before removal

[Fan Wu]

pl011_shutdown() calls pl011_rs485_tx_stop() when RS485 is enabled and
the transmit state machine is still active.  That function may re-arm
the trigger_stop_tx hrtimer via hrtimer_start() when the TX FIFO is not
yet empty, or when delay_rts_after_send is configured, and the trigger
timers can equally be left armed from a prior pl011_rs485_tx_start().

On removal, uart_remove_one_port() can finish without draining the RS485
hrtimers, and devm frees the uart_amba_port once pl011_remove() returns.
The existing teardown syncs do not help here: pl011_disable_interrupts()
only masks the UART IMSC register, pl011_dma_shutdown() cancels a
different (DMA rx) timer_list, and free_irq() plus serial_core's
synchronize_irq() only drain the UART interrupt handler, not the hrtimer
softirq callbacks.  Either trigger callback can therefore recover uap
from its embedded hrtimer and access uap->port after the free.

Cancel both RS485 trigger hrtimers in pl011_shutdown(), after the
re-arming pl011_rs485_tx_stop() call and before free_irq(), and again
in pl011_remove() after uart_remove_one_port().  The remove-side
calls cover paths such as suspend followed by unbind, where serial
core need not invoke the driver's shutdown callback.  The
callbacks pl011_trigger_start_tx and pl011_trigger_stop_tx return
HRTIMER_NORESTART, so hrtimer_cancel() fully drains any pending and
concurrently running callback without a self-restart loop.  As
pl011_trigger_start_tx() may re-enable the TX interrupt via
pl011_start_tx(), call pl011_disable_interrupts() again after the
cancels to re-mask IMSC before free_irq().  The cancel site does not
hold uart_port_lock across the cancel
(pl011_disable_interrupts() already released it) and the callbacks take
that lock internally, so there is no self-deadlock.  This mirrors the
RS485 teardown pattern in 8250_port.c (serial8250_em485_destroy),
which cancels both hrtimers before the embedding object is freed.

This issue was found by an in-house static analysis tool.

Fixes: 2c1fd53af21b ("serial: amba-pl011: Fix RTS handling in RS485 mode")
Cc: stable@vger.kernel.org   # v6.14+
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/tty/serial/amba-pl011.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8ed91e1da22b..021b542b3d99 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2083,6 +2083,16 @@ static void pl011_shutdown(struct uart_port *port)
 	if ((port->rs485.flags & SER_RS485_ENABLED && uap->rs485_tx_state != OFF))
 		pl011_rs485_tx_stop(uap);
 
+	/*
+	 * pl011_rs485_tx_stop() above may have (re-)armed the RS485 trigger
+	 * hrtimers.  A callback may also re-enable the TX interrupt, so
+	 * cancel them and mask interrupts again before freeing the IRQ.
+	 * pl011_remove() cancels them again before freeing the owner.
+	 */
+	hrtimer_cancel(&uap->trigger_start_tx);
+	hrtimer_cancel(&uap->trigger_stop_tx);
+	pl011_disable_interrupts(uap);
+
 	free_irq(uap->port.irq, uap);
 
 	pl011_disable_uart(uap);
@@ -3063,6 +3073,8 @@ static void pl011_remove(struct amba_device *dev)
 	struct uart_amba_port *uap = amba_get_drvdata(dev);
 
 	uart_remove_one_port(&amba_reg, &uap->port);
+	hrtimer_cancel(&uap->trigger_start_tx);
+	hrtimer_cancel(&uap->trigger_stop_tx);
 	pl011_unregister_port(uap);
 }
 
-- 
2.34.1

Re: [PATCH] tty: serial: amba-pl011: cancel RS485 trigger hrtimers before removal

[Greg KH]

On Tue, Jul 21, 2026 at 04:28:58AM +0000, Fan Wu wrote:
> pl011_shutdown() calls pl011_rs485_tx_stop() when RS485 is enabled and
> the transmit state machine is still active.  That function may re-arm
> the trigger_stop_tx hrtimer via hrtimer_start() when the TX FIFO is not
> yet empty, or when delay_rts_after_send is configured, and the trigger
> timers can equally be left armed from a prior pl011_rs485_tx_start().
> 
> On removal, uart_remove_one_port() can finish without draining the RS485
> hrtimers, and devm frees the uart_amba_port once pl011_remove() returns.
> The existing teardown syncs do not help here: pl011_disable_interrupts()
> only masks the UART IMSC register, pl011_dma_shutdown() cancels a
> different (DMA rx) timer_list, and free_irq() plus serial_core's
> synchronize_irq() only drain the UART interrupt handler, not the hrtimer
> softirq callbacks.  Either trigger callback can therefore recover uap
> from its embedded hrtimer and access uap->port after the free.
> 
> Cancel both RS485 trigger hrtimers in pl011_shutdown(), after the
> re-arming pl011_rs485_tx_stop() call and before free_irq(), and again
> in pl011_remove() after uart_remove_one_port().  The remove-side
> calls cover paths such as suspend followed by unbind, where serial
> core need not invoke the driver's shutdown callback.  The
> callbacks pl011_trigger_start_tx and pl011_trigger_stop_tx return
> HRTIMER_NORESTART, so hrtimer_cancel() fully drains any pending and
> concurrently running callback without a self-restart loop.  As
> pl011_trigger_start_tx() may re-enable the TX interrupt via
> pl011_start_tx(), call pl011_disable_interrupts() again after the
> cancels to re-mask IMSC before free_irq().  The cancel site does not
> hold uart_port_lock across the cancel
> (pl011_disable_interrupts() already released it) and the callbacks take
> that lock internally, so there is no self-deadlock.  This mirrors the
> RS485 teardown pattern in 8250_port.c (serial8250_em485_destroy),
> which cancels both hrtimers before the embedding object is freed.
> 
> This issue was found by an in-house static analysis tool.

Yes, but a LLM obviously wrote the text here.  Please shorten it up a
LOT and make it more concise, as-is, it's impossible to read this
wall-of-text...

thanks,

greg k-h

Re: [PATCH] tty: serial: amba-pl011: cancel RS485 trigger hrtimers before removal

[Greg KH]

On Tue, Jul 21, 2026 at 04:28:58AM +0000, Fan Wu wrote:
> pl011_shutdown() calls pl011_rs485_tx_stop() when RS485 is enabled and
> the transmit state machine is still active.  That function may re-arm
> the trigger_stop_tx hrtimer via hrtimer_start() when the TX FIFO is not
> yet empty, or when delay_rts_after_send is configured, and the trigger
> timers can equally be left armed from a prior pl011_rs485_tx_start().
> 
> On removal, uart_remove_one_port() can finish without draining the RS485
> hrtimers, and devm frees the uart_amba_port once pl011_remove() returns.
> The existing teardown syncs do not help here: pl011_disable_interrupts()
> only masks the UART IMSC register, pl011_dma_shutdown() cancels a
> different (DMA rx) timer_list, and free_irq() plus serial_core's
> synchronize_irq() only drain the UART interrupt handler, not the hrtimer
> softirq callbacks.  Either trigger callback can therefore recover uap
> from its embedded hrtimer and access uap->port after the free.
> 
> Cancel both RS485 trigger hrtimers in pl011_shutdown(), after the
> re-arming pl011_rs485_tx_stop() call and before free_irq(), and again
> in pl011_remove() after uart_remove_one_port().  The remove-side
> calls cover paths such as suspend followed by unbind, where serial
> core need not invoke the driver's shutdown callback.  The
> callbacks pl011_trigger_start_tx and pl011_trigger_stop_tx return
> HRTIMER_NORESTART, so hrtimer_cancel() fully drains any pending and
> concurrently running callback without a self-restart loop.  As
> pl011_trigger_start_tx() may re-enable the TX interrupt via
> pl011_start_tx(), call pl011_disable_interrupts() again after the
> cancels to re-mask IMSC before free_irq().  The cancel site does not
> hold uart_port_lock across the cancel
> (pl011_disable_interrupts() already released it) and the callbacks take
> that lock internally, so there is no self-deadlock.  This mirrors the
> RS485 teardown pattern in 8250_port.c (serial8250_em485_destroy),
> which cancels both hrtimers before the embedding object is freed.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 2c1fd53af21b ("serial: amba-pl011: Fix RTS handling in RS485 mode")
> Cc: stable@vger.kernel.org   # v6.14+
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  drivers/tty/serial/amba-pl011.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 8ed91e1da22b..021b542b3d99 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2083,6 +2083,16 @@ static void pl011_shutdown(struct uart_port *port)
>  	if ((port->rs485.flags & SER_RS485_ENABLED && uap->rs485_tx_state != OFF))
>  		pl011_rs485_tx_stop(uap);
>  
> +	/*
> +	 * pl011_rs485_tx_stop() above may have (re-)armed the RS485 trigger
> +	 * hrtimers.  A callback may also re-enable the TX interrupt, so
> +	 * cancel them and mask interrupts again before freeing the IRQ.
> +	 * pl011_remove() cancels them again before freeing the owner.
> +	 */
> +	hrtimer_cancel(&uap->trigger_start_tx);
> +	hrtimer_cancel(&uap->trigger_stop_tx);
> +	pl011_disable_interrupts(uap);

Please read the review:
	https://sashiko.dev/#/patchset/20260721042858.3187311-1-fanwu01@zju.edu.cn
and determine if what it reports about this change is correct or not
when you submit your second version.

thanks,

greg k-h

[PATCH v2 0/3] serial: amba-pl011: fix RS485 and DMA teardown

[Fan Wu]

Hi Greg,

v2 shortens the changelogs and reworks the RS485 fix as you asked.

  1: fix an indefinite RS485 post-send-delay hrtimer rearm
  2: fix a use-after-free of the RS485 trigger hrtimers on teardown
  3: fix a use-after-free of the DMA buffers on teardown

The hrtimers are embedded in the devm-managed port. The IRQ handler can
arm a timer, so patch 2 frees the IRQ before cancelling them. Patch 1
goes first because the rearm it fixes is what keeps the timer armed.

I read the sashiko.dev review [1]. All six points it raises are valid;
this series addresses each:

  - the lockless pl011_rs485_tx_stop() in shutdown racing the hrtimer
    callback, and a TX interrupt re-arming a timer after it was cancelled:
    patch 2 stops under the port lock and frees the IRQ first.
  - cancelling the timer before it fires left rs485_tx_state stuck at
    WAIT_AFTER_SEND, so the next open skipped the RS485 TX setup: patch 2
    finishes the stop with pl011_rs485_tx_stop_now(), resetting the state.
  - the post-send-delay timer rearming indefinitely: patch 1.
  - the RX poll timer and the TX DMA callback touching freed buffers:
    patch 3.

Changes since v1:
  - split into three patches and shortened the changelogs
  - free the IRQ before cancelling the hrtimers (v1 cancelled first)
  - add the post-send-delay (1) and DMA (3) fixes

v1: https://lore.kernel.org/all/20260721042858.3187311-1-fanwu01@zju.edu.cn/

[1] https://sashiko.dev/#/patchset/20260721042858.3187311-1-fanwu01@zju.edu.cn

---

 drivers/tty/serial/amba-pl011.c | 45 +++++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 23 deletions(-)

--
2.34.1

[PATCH v2 1/3] serial: amba-pl011: fix indefinite RS485 post-send delay

[Fan Wu]

The RS485 stop hrtimer is used both to drain the transmitter and to wait
out delay_rts_after_send. The callback cannot tell the two apart, so it
restarts the post-send delay on every expiry and the timer never stops.

Add a WAIT_AFTER_SEND_DELAY state so its expiry ends the stop sequence
instead of restarting the delay.

Fixes: 2c1fd53af21b ("serial: amba-pl011: Fix RTS handling in RS485 mode")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/tty/serial/amba-pl011.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 7f17d288c807..b212e2bcd41a 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -253,6 +253,7 @@ enum pl011_rs485_tx_state {
 	WAIT_AFTER_RTS,
 	SEND,
 	WAIT_AFTER_SEND,
+	WAIT_AFTER_SEND_DELAY,
 };
 
 /*
@@ -1285,6 +1286,7 @@ static void pl011_rs485_tx_stop(struct uart_amba_port *uap)
 			return;
 		}
 		if (port->rs485.delay_rts_after_send > 0) {
+			uap->rs485_tx_state = WAIT_AFTER_SEND_DELAY;
 			hrtimer_start(&uap->trigger_stop_tx,
 				      ms_to_ktime(port->rs485.delay_rts_after_send),
 				      HRTIMER_MODE_REL);
@@ -1350,7 +1352,8 @@ static void pl011_rs485_tx_start(struct uart_amba_port *uap)
 		uap->rs485_tx_state = SEND;
 		return;
 	}
-	if (uap->rs485_tx_state == WAIT_AFTER_SEND) {
+	if (uap->rs485_tx_state == WAIT_AFTER_SEND ||
+	    uap->rs485_tx_state == WAIT_AFTER_SEND_DELAY) {
 		hrtimer_try_to_cancel(&uap->trigger_stop_tx);
 		uap->rs485_tx_state = SEND;
 		return;
@@ -1417,7 +1420,8 @@ static enum hrtimer_restart pl011_trigger_stop_tx(struct hrtimer *t)
 	unsigned long flags;
 
 	uart_port_lock_irqsave(&uap->port, &flags);
-	if (uap->rs485_tx_state == WAIT_AFTER_SEND)
+	if (uap->rs485_tx_state == WAIT_AFTER_SEND ||
+	    uap->rs485_tx_state == WAIT_AFTER_SEND_DELAY)
 		pl011_rs485_tx_stop(uap);
 	uart_port_unlock_irqrestore(&uap->port, flags);
 
-- 
2.34.1

[PATCH v2 2/3] serial: amba-pl011: cancel RS485 hrtimers after freeing IRQ

[Fan Wu]

The RS485 trigger hrtimers are embedded in the devm-managed port and can
fire after it is freed. The IRQ handler can arm a timer, so free the IRQ
first and then cancel both timers.

Complete the RS485 stop without arming a timer, and cancel the timers
in remove() for the suspend-then-unbind path, where shutdown is not
called.

This issue was found by an in-house static analysis tool.

Fixes: 2c1fd53af21b ("serial: amba-pl011: Fix RTS handling in RS485 mode")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/tty/serial/amba-pl011.c | 52 ++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index b212e2bcd41a..e686835d5150 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1269,11 +1269,30 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
 #define pl011_dma_flush_buffer	NULL
 #endif
 
-static void pl011_rs485_tx_stop(struct uart_amba_port *uap)
+static void pl011_rs485_tx_stop_now(struct uart_amba_port *uap)
 {
 	struct uart_port *port = &uap->port;
 	u32 cr;
 
+	cr = pl011_read(uap, REG_CR);
+
+	if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+		cr &= ~UART011_CR_RTS;
+	else
+		cr |= UART011_CR_RTS;
+
+	/* Disable the transmitter and reenable the transceiver */
+	cr &= ~UART011_CR_TXE;
+	cr |= UART011_CR_RXE;
+	pl011_write(cr, uap, REG_CR);
+
+	uap->rs485_tx_state = OFF;
+}
+
+static void pl011_rs485_tx_stop(struct uart_amba_port *uap)
+{
+	struct uart_port *port = &uap->port;
+
 	if (uap->rs485_tx_state == SEND)
 		uap->rs485_tx_state = WAIT_AFTER_SEND;
 
@@ -1297,19 +1316,7 @@ static void pl011_rs485_tx_stop(struct uart_amba_port *uap)
 		hrtimer_try_to_cancel(&uap->trigger_start_tx);
 	}
 
-	cr = pl011_read(uap, REG_CR);
-
-	if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
-		cr &= ~UART011_CR_RTS;
-	else
-		cr |= UART011_CR_RTS;
-
-	/* Disable the transmitter and reenable the transceiver */
-	cr &= ~UART011_CR_TXE;
-	cr |= UART011_CR_RXE;
-	pl011_write(cr, uap, REG_CR);
-
-	uap->rs485_tx_state = OFF;
+	pl011_rs485_tx_stop_now(uap);
 }
 
 static void pl011_stop_tx(struct uart_port *port)
@@ -2019,11 +2026,20 @@ static void pl011_shutdown(struct uart_port *port)
 
 	pl011_dma_shutdown(uap);
 
-	if ((port->rs485.flags & SER_RS485_ENABLED && uap->rs485_tx_state != OFF))
-		pl011_rs485_tx_stop(uap);
-
 	free_irq(uap->port.irq, uap);
 
+	/*
+	 * free_irq() drains the UART interrupt handler, which can arm either
+	 * timer.  Cancel the timers afterwards to drain their callbacks too.
+	 */
+	hrtimer_cancel(&uap->trigger_start_tx);
+	hrtimer_cancel(&uap->trigger_stop_tx);
+
+	uart_port_lock_irq(port);
+	if (uap->rs485_tx_state != OFF)
+		pl011_rs485_tx_stop_now(uap);
+	uart_port_unlock_irq(port);
+
 	pl011_disable_uart(uap);
 
 	/*
@@ -2941,6 +2957,8 @@ static void pl011_remove(struct amba_device *dev)
 	struct uart_amba_port *uap = amba_get_drvdata(dev);
 
 	uart_remove_one_port(&amba_reg, &uap->port);
+	hrtimer_cancel(&uap->trigger_start_tx);
+	hrtimer_cancel(&uap->trigger_stop_tx);
 	pl011_unregister_port(uap);
 }
 
-- 
2.34.1

[PATCH v2 3/3] serial: amba-pl011: synchronize DMA teardown

[Fan Wu]

dmaengine_terminate_all() does not wait for a running callback, so the TX
callback can still touch the TX buffer after it is freed. The RX poll
timer reads the RX buffers without the port lock.

Switch to dmaengine_terminate_sync() and delete the RX timer before
freeing the buffers.

Fixes: ead76f329f77 ("ARM: 6763/1: pl011: add optional RX DMA to PL011 v2")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/tty/serial/amba-pl011.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e686835d5150..9bfccb021383 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1182,7 +1182,7 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)
 
 	if (uap->using_tx_dma) {
 		/* In theory, this should already be done by pl011_dma_flush_buffer */
-		dmaengine_terminate_all(uap->dmatx.chan);
+		dmaengine_terminate_sync(uap->dmatx.chan);
 		if (uap->dmatx.queued) {
 			dma_unmap_single(uap->dmatx.chan->device->dev,
 					 uap->dmatx.dma, uap->dmatx.len,
@@ -1195,12 +1195,12 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)
 	}
 
 	if (uap->using_rx_dma) {
-		dmaengine_terminate_all(uap->dmarx.chan);
+		if (uap->dmarx.poll_rate)
+			timer_delete_sync(&uap->dmarx.timer);
+		dmaengine_terminate_sync(uap->dmarx.chan);
 		/* Clean up the RX DMA */
 		pl011_dmabuf_free(uap->dmarx.chan, &uap->dmarx.dbuf_a, DMA_FROM_DEVICE);
 		pl011_dmabuf_free(uap->dmarx.chan, &uap->dmarx.dbuf_b, DMA_FROM_DEVICE);
-		if (uap->dmarx.poll_rate)
-			timer_delete_sync(&uap->dmarx.timer);
 		uap->using_rx_dma = false;
 	}
 }
-- 
2.34.1