netdev - Re: [PATCH] can: mcp251x: fix deadlock if an interrupt occurs during mcp251x

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] [day] [month] [year] [list]

Message-ID: <0c45e6aa-3712-431a-8198-eb87d04a7cab@intel.com>
Date: Mon, 19 Aug 2024 14:00:29 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Simon Arlott <simon@...iron.net>
CC: <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-can@...r.kernel.org>, Marc Kleine-Budde <mkl@...gutronix.de>, "Vincent
 Mailhol" <mailhol.vincent@...adoo.fr>
Subject: Re: [PATCH] can: mcp251x: fix deadlock if an interrupt occurs during
 mcp251x_open

On 8/17/24 17:45, Simon Arlott wrote:
> The mcp251x_hw_wake() function is called with the mpc_lock mutex held and
> disables the interrupt handler so that no interrupts can be processed while
> waking the device. If an interrupt has already occurred then waiting for
> the interrupt handler to complete will deadlock because it will be trying
> to acquire the same mutex.
> 
> CPU0                           CPU1
> ----                           ----
> mcp251x_open()
>   mutex_lock(&priv->mcp_lock)
>    request_threaded_irq()
>                                 <interrupt>
>                                 mcp251x_can_ist()
>                                  mutex_lock(&priv->mcp_lock)
>    mcp251x_hw_wake()
>     disable_irq() <-- deadlock
> 
> Use disable_irq_nosync() instead because the interrupt handler does
> everything while holding the mutex so it doesn't matter if it's still
> running.
> 
> Signed-off-by: Simon Arlott <simon@...iron.net>

You have to provide a Fixes: tag for bugfixes [PATCH net]

otherwise the change looks fine for me,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>

> ---
>   drivers/net/can/spi/mcp251x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> index 3b8736ff0345..ec5c64006a16 100644
> --- a/drivers/net/can/spi/mcp251x.c
> +++ b/drivers/net/can/spi/mcp251x.c
> @@ -752,7 +752,7 @@ static int mcp251x_hw_wake(struct spi_device *spi)
>   	int ret;
>   
>   	/* Force wakeup interrupt to wake device, but don't execute IST */
> -	disable_irq(spi->irq);
> +	disable_irq_nosync(spi->irq);
>   	mcp251x_write_2regs(spi, CANINTE, CANINTE_WAKIE, CANINTF_WAKIF);
>   
>   	/* Wait for oscillator startup timer after wake up */