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

[Cunhao Lu]

Hi,

> [Severity: Medium]
> Is this check enough to deliver what the subject line promises? It reads
> the running bit once without any lock. can_restart_work() runs on the
> system workqueue without RTNL or any CAN-private lock, so nothing orders
> it against __dev_close_many() and ndo_stop().

The check alone does not serialize restart with ndo_stop(), as noted in
the commit message. For Rockchip, this series also calls
close_candev_cancel_restart_sync() at the beginning of rkcanfd_stop(),
while the resources are still available, to wait for any restart that
has already passed the check before tearing down the hardware and IRQ.
The pre-existing teardown races in other drivers are outside the scope
of this series and should be addressed in separate patches.

> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch. The
> patch makes the window smaller, but can restart_work still be queued
> after close_candev() has cancelled it?

Yes, if a bus-off producer is still running, the check does not prevent
it from queuing work after cancellation. For Rockchip, the series keeps
the final close_candev() after free_irq() has drained the bus-off
producer. This final cancellation catches work queued after the early
cancellation, and the running check prevents such work from restarting
the hardware while the interface is down.

The late-enqueue and lifetime issues in other drivers should also be
handled in separate patches. This series is limited to the Rockchip
CAN-FD restart/close races and does not add a generic free/unregister
safeguard.

Regards,
Cunhao Lu