[PATCH] can: hi311x: drop hi3110_lock before free_irq() on open failure

[PATCH] can: hi311x: drop hi3110_lock before free_irq() on open failure

[Runyu Xiao]

hi3110_open() requests a threaded IRQ and then performs hardware
setup while holding priv->hi3110_lock. If reset, setup, or
normal-mode entry fails, the error path calls free_irq() while still
holding that mutex.

The threaded handler takes priv->hi3110_lock before checking
force_quit, while free_irq() waits for the threaded handler to finish.
That can deadlock the open() rollback path against a pending IRQ
thread.

Set force_quit, drop hi3110_lock before free_irq(), and take the
mutex again for the remaining hardware cleanup.

Fixes: 57e83fb9b746 ("can: hi311x: Add Holt HI-311x CAN driver")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/net/can/spi/hi311x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
index ae90e6716de5..2be851e8907d 100644
--- a/drivers/net/can/spi/hi311x.c
+++ b/drivers/net/can/spi/hi311x.c
@@ -787,7 +787,10 @@ static int hi3110_open(struct net_device *net)
 	return 0;
 
  out_free_irq:
+	priv->force_quit = 1;
+	mutex_unlock(&priv->hi3110_lock);
 	free_irq(spi->irq, priv);
+	mutex_lock(&priv->hi3110_lock);
 	hi3110_hw_sleep(spi);
  out_close:
 	hi3110_power_enable(priv->transceiver, 0);
-- 
2.34.1

Re: [PATCH] can: hi311x: drop hi3110_lock before free_irq() on open failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent execution of `hi3110_clean` due to a missing lock in `hi3110_do_set_mode` can lead to a double-free (use-after-free) of `priv->tx_skb`.
- [High] The driver fails to cancel pending works in `hi3110_stop` and `hi3110_can_remove`, allowing deferred I/O on stopped or powered-off hardware.
--

--- Patch [1]: [PATCH] can: hi311x: drop hi3110_lock before free_irq() on open failure ---
commit d2d3a5df6413395872456083be56586448f72c40
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

can: hi311x: drop hi3110_lock before free_irq() on open failure

hi3110_open() requests a threaded IRQ and then performs hardware setup
while holding priv->hi3110_lock. The patch fixes a deadlock by dropping
the lock before free_irq() in the open rollback path.

Cc: stable@vger.kernel.org

> diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
> index ae90e6716de5b..2be851e8907d0 100644
> --- a/drivers/net/can/spi/hi311x.c
> +++ b/drivers/net/can/spi/hi311x.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but can this lead to a double-free of
priv->tx_skb?

If hi3110_do_set_mode() is called (e.g. via netlink for manual bus-off
recovery), it unconditionally calls hi3110_clean() without acquiring
priv->hi3110_lock:

drivers/net/can/spi/hi311x.c:hi3110_do_set_mode() {
	...
	case CAN_MODE_START:
		hi3110_clean(net);
	...
}

Concurrently, hi3110_tx_work_handler() can access and free priv->tx_skb
under the lock:

drivers/net/can/spi/hi311x.c:hi3110_tx_work_handler() {
	...
	frame = (struct can_frame *)priv->tx_skb->data;
	...
}

Since hi3110_clean() frees priv->tx_skb unconditionally without the lock,
could both threads attempt to process and free the exact same sk_buff
simultaneously?

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of running
deferred I/O on stopped or powered-off hardware?

In hi3110_stop(), the device state is torn down without cancelling
pending works like priv->restart_work and priv->tx_work:

drivers/net/can/spi/hi311x.c:hi3110_stop() {
	struct hi3110_priv *priv = netdev_priv(net);
	...
}

Furthermore, during driver unbind, hi3110_can_remove() disables hardware
power before destroying the workqueue:

drivers/net/can/spi/hi311x.c:hi3110_can_remove() {
	...
	unregister_candev(net);

	hi3110_power_enable(priv->power, 0);

	destroy_workqueue(priv->wq);
	...
}

Since destroy_workqueue() flushes pending works, could this cause the works
to execute on powered-off hardware and unregistered netdevs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820020631.316418-1-runyu.xiao@seu.edu.cn?part=1