linux-kernel - Re: [PATCH] serial: 8250: 8250_omap: Fix race b/w dma completion and RX timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1497710243.22624.150.camel@linux.intel.com>
Date:   Sat, 17 Jun 2017 17:37:23 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Vignesh R <vigneshr@...com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Jiri Slaby <jslaby@...e.com>,
        Peter Hurley <peter@...leysoftware.com>,
        linux-serial@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-kernel@...r.kernel.org, Tony Lindgren <tony@...mide.com>
Subject: Re: [PATCH] serial: 8250: 8250_omap: Fix race b/w dma completion
 and RX timeout

On Sat, 2017-06-17 at 19:22 +0530, Vignesh R wrote:
> DMA RX completion handler for UART is called from a tasklet and hence
> may be delayed depending on the system load. In meanwhile, there may
> be
> RX timeout interrupt which can get serviced first before DMA RX
> completion handler is executed for the completed transfer.
> omap_8250_rx_dma_flush() which is called on RX timeout interrupt makes
> sure that the DMA RX buffer is pushed and then the FIFO is drained and
> also queues a new DMA request. But, when DMA RX completion handler
> executes, it will erroneously flush the currently queued DMA transfer
> which sometimes results in data corruption and double queueing of DMA
> RX
> requests.
> 
> Fix this by checking whether RX completion is for the currently queued
> transfer or not. And also hold port lock when in DMA completion to
> avoid
> race wrt RX timeout handler preempting it.


>  static void __dma_rx_complete(void *param)
>  {
> -	__dma_rx_do_complete(param);
> -	omap_8250_rx_dma(param);
> +	struct uart_8250_port *p = param;
> +	struct uart_8250_dma *dma = p->dma;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&p->port.lock, flags);
> +
> +	/*
> +	 * If the completion is for the current cookie then handle
> it,
> +	 * else a previous RX timeout flush would have already pushed
> +	 * data from DMA buffers, so exit.
> +	 */

> +	if (dma->rx_cookie != dma->rxchan->completed_cookie) {

Wouldn't be better to call DMAEngine API for that?
dmaengine_tx_status() I suppose

> +		spin_unlock_irqrestore(&p->port.lock, flags);
> +		return;
> +	}
> +	__dma_rx_do_complete(p);
> +	omap_8250_rx_dma(p);
> +
> +	spin_unlock_irqrestore(&p->port.lock, flags);
>  }
>  
>  static void omap_8250_rx_dma_flush(struct uart_8250_port *p)

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy