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

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 <[email protected]>

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: [email protected]

> 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/[email protected]?part=1