linux-kernel - Re: [PATCH] dmaengine: imx-sdma: fix deadlock in interrupt handler

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: <20230925090546.GW637806@pengutronix.de>
Date:   Mon, 25 Sep 2023 11:05:46 +0200
From:   Sascha Hauer <s.hauer@...gutronix.de>
To:     Tim van der Staaij | Zign <Tim.vanderstaaij@...ngroup.com>
Cc:     Shawn Guo <shawnguo@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Vinod Koul <vkoul@...nel.org>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        Fabio Estevam <festevam@...il.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] dmaengine: imx-sdma: fix deadlock in interrupt handler

On Fri, Sep 22, 2023 at 11:50:32AM +0200, Sascha Hauer wrote:
> Hi Tim,
> 
> On Thu, Sep 21, 2023 at 09:57:11AM +0000, Tim van der Staaij | Zign wrote:
> > dev_warn internally acquires the lock that is already held when
> > sdma_update_channel_loop is called. Therefore it is acquired twice and
> > this is detected as a deadlock. Temporarily release the lock while
> > logging to avoid this.
> > 
> > Signed-off-by: Tim van der Staaij <tim.vanderstaaij@...ngroup.com>
> > Link: https://lore.kernel.org/all/AM0PR08MB308979EC3A8A53AE6E2D3408802CA@AM0PR08MB3089.eurprd08.prod.outlook.com/
> > ---
> >  drivers/dma/imx-sdma.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index 51012bd39900..3a7cd783a567 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -904,7 +904,10 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
> >  	 * owned buffer is available (i.e. BD_DONE was set too late).
> >  	 */
> >  	if (sdmac->desc && !is_sdma_channel_enabled(sdmac->sdma, sdmac->channel)) {
> > +		spin_unlock(&sdmac->vc.lock);
> >  		dev_warn(sdmac->sdma->dev, "restart cyclic channel %d\n", sdmac->channel);
> > +		spin_lock(&sdmac->vc.lock);
> 
> This is strange. Why and how does dev_warn() call back into the SDMA
> driver?
> 
> We shouldn't merge this without having a clue what exactly goes wrong
> here. Please provide the corresponding lockdep output.

I have overlooked that you actually did provide the lockdep output in a
link.

I think this is a false positive. The i.MX UART driver makes sure that
the console UART never uses DMA, so it shouldn't happen that the DMA
driver issuing console messages calls back into the DMA driver.

Could you give the following patch a test?

Sascha


-------------------------------8<------------------------------------

>From 5ac9902683710c300a64a731bcda6b3b089b2706 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@...gutronix.de>
Date: Mon, 25 Sep 2023 10:39:44 +0200
Subject: [PATCH] serial: imx: Put DMA enabled UART in separate lock subclass

The i.MX UART driver never uses DMA on UARTs providing the console.
Put the UART port lock in a separate subclass to avoid lockdep
complaining about possible deadlocks when the DMA driver issues
console messages under its own spinlock held.

Reported-by: Tim van der Staaij <Tim.vanderstaaij@...ngroup.com>
Signed-off-by: Sascha Hauer <s.hauer@...gutronix.de>
---
 drivers/tty/serial/imx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 7341d060f85cb..c30113cf5db85 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1458,8 +1458,10 @@ static int imx_uart_startup(struct uart_port *port)
 	imx_uart_writel(sport, ucr4 & ~UCR4_DREN, UCR4);
 
 	/* Can we enable the DMA support? */
-	if (!uart_console(port) && imx_uart_dma_init(sport) == 0)
+	if (!uart_console(port) && imx_uart_dma_init(sport) == 0) {
+		lockdep_set_subclass(&port->lock, 1);
 		dma_is_inited = 1;
+	}
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
-- 
2.39.2

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |