Re: [PATCH net v3 1/5] can: dev: prevent bus-off recovery when the interface is not running

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new `if (!netif_running(dev)) return -ENETDOWN;` in
  can_restart() (drivers/net/can/dev/dev.c:201) is a lockless test_bit()…
- [Low] The comment above can_bus_off() in drivers/net/can/dev/dev.c
  (line 265) still says "If enabled, a timer is started to trigger bus-…

--

can: dev: prevent bus-off recovery when the interface is not running

This patch adds a netif_running() check to can_restart() so that bus-off
recovery is skipped once __LINK_STATE_START has been cleared. It also
makes can_bus_off() schedule restart_work only for a running interface.
The commit message notes that the checks do not wait for recovery that
has already passed the check.

> diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
> index 769745e22a3c..92492c503446 100644
> --- a/drivers/net/can/dev/dev.c
> +++ b/drivers/net/can/dev/dev.c
> @@ -198,6 +198,9 @@ static int can_restart(struct net_device *dev)
>  	if (!priv->do_set_mode)
>  		return -EOPNOTSUPP;
>  
> +	if (!netif_running(dev))
> +		return -ENETDOWN;
> +

[Severity: Medium]
Is this check enough to prevent the failure described in the commit
message, "potentially re-enabling hardware while the driver is tearing it
down"?

netif_running() is a lockless test_bit(). can_restart_work() runs from
the system workqueue and holds neither RTNL nor the instance lock.
__dev_close_many() clears __LINK_STATE_START under RTNL and calls
ndo_stop() some time after that. A restart that tests the bit just before
the clear_bit() still goes on to:

can_restart_work()
  can_restart()
    can_flush_echo_skb()
    netif_rx()
    netif_carrier_on()
    priv->do_set_mode(dev, CAN_MODE_START)

and this can overlap with the driver's ndo_stop().

The only core cancel point is close_candev(), and most drivers call it
after they have already torn down the hardware. For example, in
drivers/net/can/sja1000/sja1000.c:

sja1000_close() {
	netif_stop_queue(dev);
	set_reset_mode(dev);

	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER))
		free_irq(dev->irq, (void *)dev);

	close_candev(dev);
	...
}

while the restart callback concurrently does:

sja1000_set_mode() {
	case CAN_MODE_START:
		sja1000_start(dev);
		if (netif_queue_stopped(dev))
			netif_wake_queue(dev);
	...
}

Could the controller end up out of reset mode, with interrupts enabled
and the queue woken, after its IRQ handler has been freed? By the time
cancel_delayed_work_sync() in close_candev() waits for the work, the
restart has already happened.

flexcan_close() follows the same order. It disables interrupts, frees
the IRQs, stops the chip, deletes rx-offload and disables the transceiver
before it calls close_candev(). Meanwhile flexcan_set_mode() can be
restarting the chip and re-enabling interrupts.

As far as I can tell, only rockchip_canfd gets
close_candev_cancel_restart_sync() at the start of ndo_stop by the end of
the series. flexcan, sja1000 and the other close_candev() users keep the
window.

Would it make sense to serialize this instead of relying on a lockless
pre-check? One option is to cancel restart_work before any driver
hardware teardown. Another is to recheck under RTNL via rtnl_trylock()
and requeue the work if the trylock fails.

>  	if (netif_carrier_ok(dev))
>  		netdev_err(dev, "Attempt to restart for bus-off recovery, but carrier is OK?\n");
>  
> @@ -264,8 +267,9 @@ int can_restart_now(struct net_device *dev)
>  void can_bus_off(struct net_device *dev)
>  {
>  	struct can_priv *priv = netdev_priv(dev);
> +	const bool restart = priv->restart_ms && netif_running(dev);

[Severity: Low]
This isn't a bug, but the comment above can_bus_off() still describes the
old condition:

/* CAN bus-off
 *
 * This functions should be called when the device goes bus-off to
 * tell the netif layer that no more packets can be sent or received.
 * If enabled, a timer is started to trigger bus-off recovery.
 */

Could it mention that recovery is now only scheduled when the interface
is running?

The word "timer" was already inaccurate before this patch, because
recovery is a delayed work item (priv->restart_work).

The restart-ms description in Documentation/networking/can.rst also says
nothing about a running interface. That matters less, since a down
interface has no user-visible bus-off condition.

>  
> -	if (priv->restart_ms)
> +	if (restart)
>  		netdev_info(dev, "bus-off, scheduling restart in %d ms\n",
>  			    priv->restart_ms);
>  	else

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-rockchip-canfd-restart-close-v3-0-f0eb41090242%40qq.com