[PATCH] 3.19-rc4: Fix race condition in i.MX serial port driver

[PATCH] 3.19-rc4: Fix race condition in i.MX serial port driver

[Michael Doswald]

Under pressure, the imx.c device driver may transfer the same data multiple
times. 

Reason: 
Function imx_dma_tx checks for an active DMA transfer by calling
dmaengine_tx_status. The return value may indicate that no transfer is 
running while the dma_tx_callback function is still being executed. In 
this case the xmit->tail pointer may not be updated to the correct value 
when imx_dma_tx initiates a new DMA transfer, which will result in a DMA
transfer with the same base address as the previous one.

Patch:
Use existing dma_is_txing variable to guard DMA transfer initiation instead
of dmaengine_tx_status. Clear dma_is_txing at the end of the DMA callback
function therefore extending the critical section where the port lock is
held. Critical section extension is in line with the callback function 
of other drivers, e.g. serial-tegra.c (tegra_uart_tx_dma_complete).
 
 
Signed-off-by: Michael Doswald <michael.doswald@gmx.net>
---
 drivers/tty/serial/imx.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 4c5e909..29e31b1 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -503,13 +503,10 @@ static void dma_tx_callback(void *data)
 
 	dma_unmap_sg(sport->port.dev, sgl, sport->dma_tx_nents, DMA_TO_DEVICE);
 
-	sport->dma_is_txing = 0;
-
 	/* update the stat */
 	spin_lock_irqsave(&sport->port.lock, flags);
 	xmit->tail = (xmit->tail + sport->tx_bytes) & (UART_XMIT_SIZE - 1);
 	sport->port.icount.tx += sport->tx_bytes;
-	spin_unlock_irqrestore(&sport->port.lock, flags);
 
 	dev_dbg(sport->port.dev, "we finish the TX DMA.\n");
 
@@ -518,8 +515,10 @@ static void dma_tx_callback(void *data)
 	if (waitqueue_active(&sport->dma_wait)) {
 		wake_up(&sport->dma_wait);
 		dev_dbg(sport->port.dev, "exit in %s.\n", __func__);
-		return;
 	}
+
+	sport->dma_is_txing = 0;
+	spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
 static void imx_dma_tx(struct imx_port *sport)
@@ -529,11 +528,9 @@ static void imx_dma_tx(struct imx_port *sport)
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan	*chan = sport->dma_chan_tx;
 	struct device *dev = sport->port.dev;
-	enum dma_status status;
 	int ret;
 
-	status = dmaengine_tx_status(chan, (dma_cookie_t)0, NULL);
-	if (DMA_IN_PROGRESS == status)
+	if (sport->dma_is_txing)
 		return;
 
 	sport->tx_bytes = uart_circ_chars_pending(xmit);
-- 
1.9.1


 
 

Re: [PATCH] 3.19-rc4: Fix race condition in i.MX serial port driver

[Greg KH]

On Tue, Jan 13, 2015 at 05:07:08PM +0100, Michael Doswald wrote:
> Under pressure, the imx.c device driver may transfer the same data multiple
> times. 
> 
> Reason: 
> Function imx_dma_tx checks for an active DMA transfer by calling
> dmaengine_tx_status. The return value may indicate that no transfer is 
> running while the dma_tx_callback function is still being executed. In 
> this case the xmit->tail pointer may not be updated to the correct value 
> when imx_dma_tx initiates a new DMA transfer, which will result in a DMA
> transfer with the same base address as the previous one.
> 
> Patch:
> Use existing dma_is_txing variable to guard DMA transfer initiation instead
> of dmaengine_tx_status. Clear dma_is_txing at the end of the DMA callback
> function therefore extending the critical section where the port lock is
> held. Critical section extension is in line with the callback function 
> of other drivers, e.g. serial-tegra.c (tegra_uart_tx_dma_complete).
>  
>  
> Signed-off-by: Michael Doswald <michael.doswald@gmx.net>
> ---
>  drivers/tty/serial/imx.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Fails to apply to my tree :(

Re: [PATCH] 3.19-rc4: Fix race condition in i.MX serial port driver

[Michael Doswald]

On 31.01.2015 00:25, Greg KH wrote:
>> Under pressure, the imx.c device driver may transfer the same data multiple
>> times. 
>>
>> Reason: 
>> Function imx_dma_tx checks for an active DMA transfer by calling
>> dmaengine_tx_status. The return value may indicate that no transfer is 
>> running while the dma_tx_callback function is still being executed. In 
>> this case the xmit->tail pointer may not be updated to the correct value 
>> when imx_dma_tx initiates a new DMA transfer, which will result in a DMA
>> transfer with the same base address as the previous one.
>>
>> Patch:
>> Use existing dma_is_txing variable to guard DMA transfer initiation instead
>> of dmaengine_tx_status. Clear dma_is_txing at the end of the DMA callback
>> function therefore extending the critical section where the port lock is
>> held. Critical section extension is in line with the callback function 
>> of other drivers, e.g. serial-tegra.c (tegra_uart_tx_dma_complete).
>>  
>>  
>> Signed-off-by: Michael Doswald <michael.doswald@gmx.net>
>> ---
>>  drivers/tty/serial/imx.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> Fails to apply to my tree :(
>

Hi,

I'm sorry, it seems I made the mistake to create the patch against 3.19-rc3 instead of rc4. I just saw that the rc4 tree already contains a patch which fixes the exact same problem (commit 42f752b3fbcfee9c27e4f6f6216e60e130ba98c8 by Dirk Behme).

Thank you for notifying me.

Regards,
Michael