Re: [PATCH net v6 1/6] 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 3 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 2

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 patch is titled 'prevent bus-off recovery when the
  interface is not running', but the netif_running() checks it adds are…
- [Low] The patch rewords the bus-off recovery paragraph in
  Documentation/networking/can.rst so that the 'e.g.::' now follows the…
- [Low] The new user-facing text in Documentation/networking/can.rst
  states that recovery happens only while the interface is running, with…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch only
  narrows it.

--

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

This patch adds a netif_running() check to can_restart() before the echo
skbs are flushed and the driver restart callback is called. It also changes
can_bus_off() so the restart work is only scheduled when restart_ms is set
and the interface is running. Documentation/networking/can.rst is updated to
describe the running-interface requirement.

> diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
> index 536ff411da1d1..861397e4ce906 100644
> --- a/Documentation/networking/can.rst
> +++ b/Documentation/networking/can.rst
> @@ -1298,7 +1298,8 @@ More info to the above output:
>  	Automatic restart delay time. If set to a non-zero value, a
>  	restart of the CAN controller will be triggered automatically
>  	in case of a bus-off condition after the specified delay time
> -	in milliseconds. By default it's off.
> +	in milliseconds, provided the interface is running. By default
> +	it's off.

[Severity: Low]
Is this guarantee accurate as written? The bus-off paragraph below makes the
same claim with no qualification ("Recovery is only scheduled and started
while the interface is running").

Each new check is a single lockless netif_running() snapshot. Once the check
in can_restart() passes, the function still goes on to do:

	can_flush_echo_skb(dev);
	...
	netif_carrier_on(dev);
	err = priv->do_set_mode(dev, CAN_MODE_START);

Nothing orders this against __dev_close_many() clearing __LINK_STATE_START
and calling ndo_stop(). In the same way, can_bus_off() saves netif_running()
into restart and only queues the work later.

The only wait point is cancel_delayed_work_sync() in close_candev(). That
runs inside ndo_stop, after the interface has already stopped running.

The commit message says as much ("These checks do not wait for recovery
that has already passed the check, nor serialize the check with work
submission"), but the user documentation doesn't. Would softer wording, for
example that recovery is not scheduled and is skipped once the interface is
down, describe the behaviour more accurately?

> @@ -1372,7 +1373,8 @@ before you can start it to avoid error-prone default settings::
>  A device may enter the "bus-off" state if too many errors occurred on
>  the CAN bus. Then no more messages are received or sent. An automatic
>  bus-off recovery can be enabled by setting the "restart-ms" to a
> -non-zero value, e.g.::
> +non-zero value. Recovery is only scheduled and started while the
> +interface is running, e.g.::
>  
>      $ ip link set canX type can restart-ms 100

[Severity: Low]
Is the "e.g.::" now attached to the wrong sentence? Before this patch, the
example showed how to enable recovery by setting restart-ms. Now it follows
the "while the interface is running" sentence, so it reads as a command to
run on a running interface.

can_changelink() rejects that case:

drivers/net/can/dev/netlink.c:can_changelink() {
    ...
		/* Do not allow changing restart delay while running */
		if (dev->flags & IFF_UP)
			return -EBUSY;
    ...
}

Could the "e.g.::" stay on the enabling sentence, with the running-interface
note moved after the example? It might also be worth saying that restart-ms
must be set while the interface is down.

> diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
> index 769745e22a3c7..d5cb22db2a346 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]
Does this check actually close the window the subject line describes?
can_restart_work() runs on system_percpu_wq without RTNL. That means the
check can pass just before __dev_close_many() clears __LINK_STATE_START:

CPU0 (restart work)              CPU1 (RTNL held)
can_restart_work()
  can_restart()
    netif_running() == true
                                 __dev_close_many()
                                   clear_bit(__LINK_STATE_START)
                                   dev_deactivate_many()
                                   flexcan_close()
                                     free_irq()
                                     flexcan_chip_stop_disable_on_error()
                                     can_rx_offload_del()
                                     flexcan_transceiver_disable()
    netif_carrier_on()
    flexcan_set_mode(CAN_MODE_START)
      flexcan_chip_start()
      flexcan_chip_interrupts_enable()
      netif_wake_queue()
                                     close_candev()
                                       cancel_delayed_work_sync()

flexcan_close() calls close_candev() only after all of the hardware
teardown. So cancel_delayed_work_sync() just waits for the restart to
finish.

Could the controller then stay restarted and active on the bus after
ifdown, with its IRQ freed and the transceiver disabled?

can_bus_off() has a similar gap. restart comes from a snapshot, and
schedule_delayed_work() runs later. hi3110_stop() calls close_candev()
before free_irq(), so in drivers like hi3110 the work can be queued after
the only cancel. If the interface is reopened within restart_ms, that stale
work would run can_restart() on a live controller.

The commit message scopes this case out. A later patch in the series adds
close_candev_cancel_restart_sync(), but only rockchip_canfd uses it. At the
end of the series flexcan, hi3110 and the other in-tree drivers still have
this race.

Is real serialization needed here to stop recovery once the interface starts
closing? For example, the restart work could be cancelled at the start of
ndo_stop (in each driver, or through a core hook), or the work could use an
rtnl_trylock() and requeue on failure.

> @@ -259,13 +262,15 @@ 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);
>  
> -	if (priv->restart_ms)
> +	if (restart)
>  		netdev_info(dev, "bus-off, scheduling restart in %d ms\n",
>  			    priv->restart_ms);
>  	else
> @@ -273,7 +278,7 @@ void can_bus_off(struct net_device *dev)
>  
>  	netif_carrier_off(dev);
>  
> -	if (priv->restart_ms)
> +	if (restart)
>  		schedule_delayed_work(&priv->restart_work,
>  				      msecs_to_jiffies(priv->restart_ms));
>  }

[Severity: High]
This is a pre-existing issue, but this patch only narrows it and doesn't
close it. Can restart_work still be queued after close_candev() and then
outlive the net_device?

Here is the teardown path with hi3110:

hi3110_can_remove()
  unregister_candev()
    __dev_close_many()
      hi3110_stop()
        close_candev()
          cancel_delayed_work_sync()   <- nothing pending yet
        priv->force_quit = 1;
        free_irq()

Meanwhile the threaded IST can be on its way into can_bus_off(). It calls
can_bus_off() whatever the value of restart_ms:

hi3110_can_ist()
    ...
			if (new_state == CAN_STATE_BUS_OFF) {
				if (skb)
					netif_rx(skb);
				can_bus_off(net);

Suppose the IST computed restart as true before __LINK_STATE_START was
cleared, and was then delayed until after close_candev(). In that case
schedule_delayed_work() queues the work after the only cancel.
hi3110_can_remove() then calls free_candev(), which is just:

void free_candev(struct net_device *dev)
{
	free_netdev(dev);
}

When restart_ms expires, the delayed_work timer and can_restart_work() would
run on freed memory. That covers the container_of() to priv, the priv->dev
load, the new netif_running() read of dev->state, and the indirect call
through priv->do_set_mode. The new check in can_restart() can't help here,
because it reads the freed net_device itself.

Before this patch, any can_bus_off() after close_candev() queued the work,
so the window is now much smaller. The commit message leaves the remaining
case to drivers, and no later patch in the series changes hi3110 or the core
free path.

Would closing this need either serialization of the check and the queuing
against close_candev(), or a cancel/disable of restart_work in the
unregister or free path?

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