Fix two races in DMA Rx completion and rearm handling.
I know in advance that the first patch will not be backport friendly
because of commit 56dc5074cbec ("serial: 8250_dma: Rearm DMA Rx if more
data is pending") that is not even in 6.1 but I can create the custom
backport on request.
Cc: [email protected]
Ilpo Järvinen (2):
serial: 8250_dma: Fix DMA Rx completion race
serial: 8250_dma: Fix DMA Rx rearm race
drivers/tty/serial/8250/8250_dma.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
--
2.30.2
__dma_rx_complete() is called from two places:
- Through the DMA completion callback dma_rx_complete()
- From serial8250_rx_dma_flush() after IIR_RLSI or IIR_RX_TIMEOUT
The former does not hold port's lock during __dma_rx_complete() which
allows these two to race and potentially insert the same data twice.
Extend port's lock coverage in dma_rx_complete() to prevent the race
and check if the DMA Rx is still pending completion before calling
into __dma_rx_complete().
Reported-by: Gilles BULOZ <[email protected]>
Tested-by: Gilles BULOZ <[email protected]>
Fixes: 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
Cc: [email protected]
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250_dma.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 37d6af2ec427..5594883a96f8 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -62,9 +62,14 @@ static void dma_rx_complete(void *param)
struct uart_8250_dma *dma = p->dma;
unsigned long flags;
- __dma_rx_complete(p);
-
spin_lock_irqsave(&p->port.lock, flags);
+ if (dma->rx_running)
+ __dma_rx_complete(p);
+
+ /*
+ * Cannot be combined with the previous check because __dma_rx_complete()
+ * changes dma->rx_running.
+ */
if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
p->dma->rx_dma(p);
spin_unlock_irqrestore(&p->port.lock, flags);
--
2.30.2
As DMA Rx can be completed from two places, it is possible that DMA Rx
completes before DMA completion callback had a chance to complete it.
Once the previous DMA Rx has been completed, a new one can be started
on the next UART interrupt. The following race is possible
(uart_unlock_and_check_sysrq_irqrestore() replaced with
spin_unlock_irqrestore() for simplicity/clarity):
CPU0 CPU1
dma_rx_complete()
serial8250_handle_irq()
spin_lock_irqsave(&port->lock)
handle_rx_dma()
serial8250_rx_dma_flush()
__dma_rx_complete()
dma->rx_running = 0
// Complete DMA Rx
spin_unlock_irqrestore(&port->lock)
serial8250_handle_irq()
spin_lock_irqsave(&port->lock)
handle_rx_dma()
serial8250_rx_dma()
dma->rx_running = 1
// Setup a new DMA Rx
spin_unlock_irqrestore(&port->lock)
spin_lock_irqsave(&port->lock)
// sees dma->rx_running = 1
__dma_rx_complete()
dma->rx_running = 0
// Incorrectly complete
// running DMA Rx
This race seems somewhat theoretical to occur for real but handle it
correctly regardless. Check what is the DMA status before complething
anything in __dma_rx_complete().
Reported-by: Gilles BULOZ <[email protected]>
Tested-by: Gilles BULOZ <[email protected]>
Fixes: 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
Cc: [email protected]
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250_dma.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 5594883a96f8..7fa66501792d 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -43,15 +43,23 @@ static void __dma_rx_complete(struct uart_8250_port *p)
struct uart_8250_dma *dma = p->dma;
struct tty_port *tty_port = &p->port.state->port;
struct dma_tx_state state;
+ enum dma_status dma_status;
int count;
- dma->rx_running = 0;
- dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
+ /*
+ * New DMA Rx can be started during the completion handler before it
+ * could acquire port's lock and it might still be ongoing. Don't to
+ * anything in such case.
+ */
+ dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
+ if (dma_status == DMA_IN_PROGRESS)
+ return;
count = dma->rx_size - state.residue;
tty_insert_flip_string(tty_port, dma->rx_buf, count);
p->port.icount.rx += count;
+ dma->rx_running = 0;
tty_flip_buffer_push(tty_port);
}
--
2.30.2
On Mon, Jan 30, 2023 at 01:48:40PM +0200, Ilpo J?rvinen wrote:
> __dma_rx_complete() is called from two places:
> - Through the DMA completion callback dma_rx_complete()
> - From serial8250_rx_dma_flush() after IIR_RLSI or IIR_RX_TIMEOUT
> The former does not hold port's lock during __dma_rx_complete() which
> allows these two to race and potentially insert the same data twice.
>
> Extend port's lock coverage in dma_rx_complete() to prevent the race
> and check if the DMA Rx is still pending completion before calling
> into __dma_rx_complete().
>
> Reported-by: Gilles BULOZ <[email protected]>
> Tested-by: Gilles BULOZ <[email protected]>
> Fixes: 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
> Cc: [email protected]
> Signed-off-by: Ilpo J?rvinen <[email protected]>
FWIW:
Acked-by: Heikki Krogerus <[email protected]>
> ---
> drivers/tty/serial/8250/8250_dma.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 37d6af2ec427..5594883a96f8 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -62,9 +62,14 @@ static void dma_rx_complete(void *param)
> struct uart_8250_dma *dma = p->dma;
> unsigned long flags;
>
> - __dma_rx_complete(p);
> -
> spin_lock_irqsave(&p->port.lock, flags);
> + if (dma->rx_running)
> + __dma_rx_complete(p);
> +
> + /*
> + * Cannot be combined with the previous check because __dma_rx_complete()
> + * changes dma->rx_running.
> + */
> if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
> p->dma->rx_dma(p);
> spin_unlock_irqrestore(&p->port.lock, flags);
> --
> 2.30.2
--
heikki
On Mon, Jan 30, 2023 at 01:48:41PM +0200, Ilpo J?rvinen wrote:
> As DMA Rx can be completed from two places, it is possible that DMA Rx
> completes before DMA completion callback had a chance to complete it.
> Once the previous DMA Rx has been completed, a new one can be started
> on the next UART interrupt. The following race is possible
> (uart_unlock_and_check_sysrq_irqrestore() replaced with
> spin_unlock_irqrestore() for simplicity/clarity):
>
> CPU0 CPU1
> dma_rx_complete()
> serial8250_handle_irq()
> spin_lock_irqsave(&port->lock)
> handle_rx_dma()
> serial8250_rx_dma_flush()
> __dma_rx_complete()
> dma->rx_running = 0
> // Complete DMA Rx
> spin_unlock_irqrestore(&port->lock)
>
> serial8250_handle_irq()
> spin_lock_irqsave(&port->lock)
> handle_rx_dma()
> serial8250_rx_dma()
> dma->rx_running = 1
> // Setup a new DMA Rx
> spin_unlock_irqrestore(&port->lock)
>
> spin_lock_irqsave(&port->lock)
> // sees dma->rx_running = 1
> __dma_rx_complete()
> dma->rx_running = 0
> // Incorrectly complete
> // running DMA Rx
>
> This race seems somewhat theoretical to occur for real but handle it
> correctly regardless. Check what is the DMA status before complething
> anything in __dma_rx_complete().
>
> Reported-by: Gilles BULOZ <[email protected]>
> Tested-by: Gilles BULOZ <[email protected]>
> Fixes: 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
> Cc: [email protected]
> Signed-off-by: Ilpo J?rvinen <[email protected]>
Acked-by: Heikki Krogerus <[email protected]>
> ---
> drivers/tty/serial/8250/8250_dma.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 5594883a96f8..7fa66501792d 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -43,15 +43,23 @@ static void __dma_rx_complete(struct uart_8250_port *p)
> struct uart_8250_dma *dma = p->dma;
> struct tty_port *tty_port = &p->port.state->port;
> struct dma_tx_state state;
> + enum dma_status dma_status;
> int count;
>
> - dma->rx_running = 0;
> - dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> + /*
> + * New DMA Rx can be started during the completion handler before it
> + * could acquire port's lock and it might still be ongoing. Don't to
> + * anything in such case.
> + */
> + dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> + if (dma_status == DMA_IN_PROGRESS)
> + return;
>
> count = dma->rx_size - state.residue;
>
> tty_insert_flip_string(tty_port, dma->rx_buf, count);
> p->port.icount.rx += count;
> + dma->rx_running = 0;
>
> tty_flip_buffer_push(tty_port);
> }
> --
> 2.30.2
--
heikki