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

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

[Dmitry Antipov]

Cancel all pending ECUs in 'j1939_netdev_stop()'. This is needed
to prevent the case when stopped device no longer processes ECUs
and, since ECU holds the reference to 'struct j1939_priv', the
latter (and ECU itself) is never freed.

Reported-by: syzbot+489e907b2a026a6f5fa0@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=489e907b2a026a6f5fa0
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 net/can/j1939/bus.c        | 10 ++++++++++
 net/can/j1939/j1939-priv.h |  1 +
 net/can/j1939/main.c       |  1 +
 3 files changed, 12 insertions(+)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..a180bbdb3790 100644
--- 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);
+	write_unlock_bh(&priv->lock);
+}
+
 void j1939_ecu_unmap_all(struct j1939_priv *priv)
 {
 	int i;
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..37a245ef9b73 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -204,6 +204,7 @@ struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name);
 
 void j1939_ecu_timer_start(struct j1939_ecu *ecu);
 void j1939_ecu_timer_cancel(struct j1939_ecu *ecu);
+void j1939_ecu_cancel_all(struct j1939_priv *priv);
 void j1939_ecu_unmap_all(struct j1939_priv *priv);
 
 struct j1939_priv *j1939_netdev_start(struct net_device *ndev);
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 5e5e6c228f22..1f68ba803700 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);
 	j1939_priv_put(priv);
 }
-- 
2.55.0

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

[sashiko-bot]

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

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

[Oleksij Rempel]

Hi Dmitry,

On Mon, Aug 24, 2026 at 04:08:43PM +0000, sashiko-bot@kernel.org wrote:
> 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.

Can you please take a look to this issues.

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

No need to worry about pre-existing, except you wont to.

> --
> 
> --- 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
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Dmitry Antipov]

On 8/26/26 8:00 AM, Oleksij Rempel wrote:

>> - [High] AB-BA deadlock between `priv->lock` and `hrtimer_cancel()` when stopping the device.

Will likely resend with an extra quirk, see https://syzkaller.appspot.com/text?tag=Patch&x=14fe5625580000.

>> - [High] Unconditional cancellation of all ECUs on any socket closure breaks address claiming for the entire CAN interface.

Sure it breaks address claiming. But, and this is the question for real
(not like me) CAN experts: if the device is forced to stop, what else we can do?

Dmitry