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

[sashiko-bot@kernel.org]

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

New issues:
- [High] AB-BA deadlock between `priv->lock` and `hrtimer_cancel()` when stopping the device.
- [High] Unconditional cancellation of all ECUs on any socket closure breaks address claiming for the entire CAN interface.

Pre-existing issues:
- [Critical] Unlocked list deletion in `__j1939_ecu_release()` leads to list corruption.
--

--- Patch [1]: [PATCH] can: j1939: cancel all pending ECUs on device stop ---
commit f80db37c4e58ab7d9daa72ce8c93a7552521ee48
Author: Dmitry Antipov <dmantipov@yandex.ru>

can: j1939: cancel all pending ECUs on device stop
    
This patch modifies the device stop routine to cancel all pending ECU
timers to prevent a situation where an ECU continues holding a reference
to the private data structure after the device stops processing.
    
Reported-by: syzbot+489e907b2a026a6f5fa0@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=489e907b2a026a6f5fa0
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -96,6 +96,16 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
>  	write_unlock_bh(&ecu->priv->lock);
>  }
>  
> +void j1939_ecu_cancel_all(struct j1939_priv *priv)
> +{
> +	struct j1939_ecu *ecu, *tmp;
> +
> +	write_lock_bh(&priv->lock);
> +	list_for_each_entry_safe(ecu, tmp, &priv->ecus, list)
> +		j1939_ecu_timer_cancel(ecu);

[Severity: High]
Could this introduce an AB-BA deadlock with the timer callback?

The loop holds the lock while cancelling the timer:

write_lock_bh(&priv->lock);
list_for_each_entry_safe(ecu, tmp, &priv->ecus, list)
        j1939_ecu_timer_cancel(ecu);

If j1939_ecu_timer_handler() is currently executing on another CPU, it
will unconditionally attempt to acquire the exact same lock:

net/can/j1939/bus.c:j1939_ecu_timer_handler() {
    ...
    write_lock_bh(&priv->lock);
    ...
}

Would this cause hrtimer_cancel() to spin waiting for the callback to
finish, while the callback waits for the lock held by this thread?

[Severity: Critical]
This isn't a bug introduced by this patch, but iterating this list might
expose a pre-existing list corruption issue.

When j1939_ecu_timer_cancel() drops the reference via j1939_ecu_put(), or
when a temporary reference is dropped in j1939_ac_recv(), it can trigger
the final release:

net/can/j1939/bus.c:__j1939_ecu_release() {
    ...
    list_del(&ecu->list);
    ...
}

The list_del() executes without holding priv->lock. If this happens
concurrently while another thread is iterating priv->ecus (like the loop
introduced here), could it corrupt the list and cause a use-after-free or
kernel panic?

> +	write_unlock_bh(&priv->lock);
> +}
> +
>  void j1939_ecu_unmap_all(struct j1939_priv *priv)

[ ... ]

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> --- 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);

[Severity: High]
Does unconditionally cancelling all ECUs here break address claiming for
the shared interface?

It appears j1939_netdev_stop() is called whenever any local user closes
a J1939 socket, but the CAN interface itself might still be in use by
other sockets. Calling j1939_ecu_cancel_all() before checking the shared
rx_kref count could disrupt network management for those remaining users.

>  	kref_put_mutex(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock);
>  	j1939_priv_put(priv);
>  }

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