Re: [PATCH v3 2/2] can: j1939: cancel all pending ECUs on device stop

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional cancellation of ECU timers in j1939_netdev_stop() breaks address claiming for all sockets.
- [High] Race condition allows ECU and priv memory leak because timers are cancelled before the RX path is disabled.

Pre-existing issues:
- [High] Deadlock in __j1939_ecu_work due to acquiring write_lock(&priv->lock) without disabling softirqs.
- [High] Module unload while asynchronous ECU workqueue items are still pending causes an Execution-After-Free panic.
--

commit 5eb4b9f58ccbd7c9b7e110c01dfe98b7a95acee8
Author: Dmitry Antipov <dmantipov@yandex.ru>

can: j1939: cancel all pending ECUs on device stop

This commit cancels all pending ECUs in j1939_netdev_stop() to prevent
a situation where a stopped device no longer processes ECUs. This
ensures the ECU and its reference to the j1939_priv structure are
properly freed.

Link: https://syzkaller.appspot.com/bug?extid=489e907b2a026a6f5fa0

[ ... ]

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index 5e5e6c228f225..1f68ba8037004 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
> @@ -306,6 +306,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
>  
>  void j1939_netdev_stop(struct j1939_priv *priv)
>  {
> +	j1939_ecu_cancel_all(priv);
>  	kref_put_mutex(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock);

[Severity: High]
Does unconditionally cancelling all ECU timers here break address claiming
for all sockets?

When any single socket closes, it invokes j1939_netdev_stop(), which
unconditionally cancels all ECU timers for the shared priv structure.

Does this forcefully abort pending address claim timers for every other
active socket on the interface, leaving those ECUs unmapped permanently?

Should the timer cancellation be properly scoped to the destruction of the
last socket in the __j1939_rx_release callback instead?


[Severity: High]
Can this sequence race and cause an ECU memory leak?

If a socket is closed, initiating j1939_netdev_stop(), j1939_ecu_cancel_all()
runs. If a new AC frame is received on the bus before __j1939_rx_release()
disables CAN RX, it triggers j1939_ac_process() which starts a new ECU timer.

Since j1939_ecu_unmap_all() already ran synchronously during teardown, the
new ECU timer will fire later, taking a reference and leaving the new ECU
permanently unmapped. 

Does this permanently leak both the ECU and its referenced j1939_priv?

>  	j1939_priv_put(priv);
>  }

[ ... ]


This is a pre-existing issue, but I noticed a potential deadlock here:

    net/can/j1939/bus.c:__j1939_ecu_work() {
        ...
        write_lock(&priv->lock);
        list_del(&ecu->list);
        ...
    }

[Severity: High]
Does acquiring write_lock(&priv->lock) without disabling softirqs cause a
deadlock?

Because write_lock() disables preemption but not softirqs, the workqueue
thread can be preempted by the CAN RX softirq on the same CPU. 

If a CAN RX interrupt arrives and triggers j1939_ac_process() in softirq
context, it will spin on write_lock_bh(&priv->lock), resulting in a lock
inversion deadlock. 

Should this use write_lock_bh() instead?


This is a pre-existing issue, but I also noticed a missing synchronization
here:

    net/can/j1939/main.c:j1939_module_exit() {
        can_proto_unregister(&j1939_can_proto);
        unregister_netdevice_notifier(&j1939_netdev_notifier);
    }

[Severity: High]
Can unloading the module while asynchronous ECU workqueue items are still
pending cause an Execution-After-Free panic?

When a socket closes, __j1939_ecu_release defers freeing the ECU via
schedule_work(&ecu->work). 

Because j1939_module_exit() lacks a flush_scheduled_work() or similar
synchronization call, the module memory can be freed before the workqueue
executes __j1939_ecu_work. 

Does this crash the kernel when the worker thread branches to the unmapped
function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828064621.91364-1-dmantipov@yandex.ru?part=2