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

[Marc Kleine-Budde]

> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Runyu Xiao <runyu.xiao@xxxxxxxxxx>

The netdev LLM bot has found some new issues with this patch, can you
take care of them?

regards,
Marc

--- Patch [30]: [PATCH net-next 30/37] can: hi311x: drop hi3110_lock before free_irq() on open failure ---
commit 67dd4c372c8d870201d8692df57226dc8aab950d
Author: Runyu Xiao <runyu.xiao@xxxxxxxxxx>

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

This patch changes the hi3110_open() error path. It now sets force_quit
and drops hi3110_lock before calling free_irq(), then takes the mutex
again for the remaining hardware cleanup. This avoids a deadlock against
the threaded IRQ handler, which takes hi3110_lock before it checks
force_quit.

Link: https://patch.msgid.link/20260820020631.316418-1-runyu.xiao@xxxxxxxxxx

> diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
> index 076a3efa1bfff..927555b5d581e 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);

Discovered by: gpt-6-astra · Missed by: opus-5-5, sashiko-gemini · Confirmed by: opus-5-5 · Ranked High by: gpt-6-astra
Can this rollback turn the old deadlock into an IRQ storm when
hi3110_set_normal_mode() fails?

The IRQ is requested with IRQF_ONESHOT | IRQF_TRIGGER_HIGH. Before
hi3110_set_normal_mode() returns -EBUSY, it has already armed the chip:

drivers/net/can/spi/hi311x.c:hi3110_set_normal_mode() {
    hi3110_write(spi, HI3110_WRITE_INTE, HI3110_INT_BUSERR |
                 HI3110_INT_RXFIFO | HI3110_INT_TXCPLT);
    hi3110_write(spi, HI3110_WRITE_CTRL1, HI3110_CTRL1_TXEN);
    ...
    hi3110_write(spi, HI3110_WRITE_CTRL0, reg);
    mdelay(HI3110_OST_DELAY_MS);
    reg = hi3110_read(spi, HI3110_READ_CTRL0);
    if ((reg & HI3110_CTRL0_MODE_MASK) != reg)
        return -EBUSY;
    ...
}

A received frame or a bus error during the mdelay() window can assert
INT. Once force_quit is set, hi3110_can_ist() skips the whole
while (!priv->force_quit) loop. It never reads STATF, the FIFO or INTF,
but it still returns IRQ_HANDLED.

irq_finalize_oneshot() then unmasks the line, which is still asserted.
The interrupt fires again right away, so the SCHED_FIFO IRQ thread spins
until free_irq() finally shuts the line down.

If the IRQ thread was blocked on hi3110_lock, the new mutex_unlock()
wakes it. On the same CPU (always the case on UP), it can preempt the
opener, which holds RTNL, before the opener reaches free_irq().

The opener then only runs again through RT throttling or the fair
dl_server. That bounds the stall to about one RT period by default, but
the stall can be indefinite if those safeguards are disabled.

Would it be better to quiet the chip while still holding the mutex,
before force_quit is set and the lock is dropped? That could mirror
what hi3110_stop() does:

    hi3110_write(spi, HI3110_WRITE_CTRL1, 0x0);
    hi3110_write(spi, HI3110_WRITE_INTE, 0x0);
    hi3110_read(spi, HI3110_READ_INTF);

Another option is calling disable_irq_nosync() before mutex_unlock().

hi3110_stop() already uses the same force_quit then free_irq() ordering
while INTE is still enabled, so the same hazard exists there. In the
open() rollback path, though, it is new with this patch.

> +	mutex_lock(&priv->hi3110_lock);
>  	hi3110_hw_sleep(spi);
>   out_close:
>  	hi3110_power_enable(priv->transceiver, 0);

Discovered by: gpt-6-astra · Missed by: opus-5-5, sashiko-gemini · Confirmed by: opus-5-5 · Called a false positive by: gpt-6-astra
This is a pre-existing issue that this patch did not introduce, but is
priv->restart_work ever cancelled or flushed on shutdown?

hi3110_do_set_mode() queues it for CAN_MODE_START:

drivers/net/can/spi/hi311x.c:hi3110_do_set_mode() {
    ...
    priv->restart_tx = 1;
    if (priv->can.restart_ms == 0)
        priv->after_suspend = HI3110_AFTER_SUSPEND_RESTART;
    queue_work(priv->wq, &priv->restart_work);
    ...
}

hi3110_stop() calls close_candev(), which only cancels the CAN core's
delayed restart_work. It never calls
cancel_work_sync(&priv->restart_work), and neither does the hi3110_open()
rollback.

If the work is still pending at ifdown or during a failed open(),
hi3110_restart_work_handler() can run after cleanup. It takes
hi3110_lock and calls hi3110_hw_reset(), hi3110_setup() and
hi3110_set_normal_mode(), which re-arms INTE, TXEN and normal mode. It
then clears force_quit and calls netif_wake_queue() on the downed
interface.

The controller would then be back on the bus while the interface is
administratively down.

The new unlocked window around free_irq() in hi3110_open() gives the
worker one more place to run. It does not seem to make things worse than
the existing post-stop case, because free_irq() still completes and the
rollback then calls hi3110_hw_sleep().

There is no use-after-free here, because hi3110_can_remove() runs
destroy_workqueue() before free_candev().

-- 
Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>