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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent execution of hi3110_clean() without mutual exclusion can result in a double-free of priv->tx_skb and a data race on priv->tx_busy.
- [High] hi3110_stop() does not cancel pending driver workqueues (priv->restart_work and priv->tx_work), leading to use-after-free, IRQ storms, or hardware access after power off.
--

commit 22aa36b3ba9420685fbf25a2a6f3ce41f336fdb3
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. If setup fails, dropping the lock before calling
free_irq() prevents a deadlock against a pending IRQ thread.

Link: https://patch.msgid.link/20260820020631.316418-1-runyu.xiao@seu.edu.cn

> 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
> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent execution of
hi3110_clean() without mutual exclusion result in a double-free of
priv->tx_skb and a data race on priv->tx_busy?

It appears hi3110_do_set_mode() can enter hi3110_clean() unlocked, while
hi3110_tx_work_handler() can enter it locked. If they run concurrently, could
this result in simultaneously invoking dev_kfree_skb() and corrupting the
echo skb state?

drivers/net/can/spi/hi311x.c:hi3110_clean() {
    ...
    dev_kfree_skb(priv->tx_skb);
    if (priv->tx_busy)
        can_free_echo_skb(priv->net, 0, NULL);
    ...
}

[Severity: High]
This is a pre-existing issue, but does hi3110_stop() need to cancel pending
driver workqueues like priv->restart_work and priv->tx_work?

Currently, there is no cancel_work_sync() in hi3110_stop(). Could this lead
to hardware access after power off if hi3110_can_remove() disables main VDD
power before the workqueue is drained?

For instance, if hi3110_restart_work_handler() runs after power off, would
it force execution against powered-off hardware and cause SPI bus faults?

drivers/net/can/spi/hi311x.c:hi3110_restart_work_handler() {
    ...
    if (priv->restart_tx) {
        priv->restart_tx = 0;
        hi3110_hw_reset(spi);
        hi3110_setup(net);
        hi3110_clean(net);
        hi3110_set_normal_mode(spi);
        netif_wake_queue(net);
    }
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=14