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

[PATCH v4] 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. Since hrtimer_cancel()
waits for timer callback, use hrtimer_try_to_cancel() to avoid
deadlock on 'priv->lock' between j1939_ecu_timer_handler() and
j1939_ecu_cancel_all().

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>
---
v4: go back to simpler approach with hrtimer_try_to_cancel()
v3: rebase on top of experimental lockless hrtimer handler
    and asynchronous ECU freeing
v2: add extra precaution to avoid ABBA deadlock between
    j1939_ecu_cancel_all() and timer callbacks (Sashiko)
---
 net/can/j1939/bus.c        | 11 +++++++++++
 net/can/j1939/j1939-priv.h |  1 +
 net/can/j1939/main.c       |  1 +
 3 files changed, 13 insertions(+)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..0b5232b53605 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -96,6 +96,17 @@ 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)
+		if (hrtimer_try_to_cancel(&ecu->ac_timer) != -1)
+			j1939_ecu_put(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 v4] 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:
- [Critical] can: j1939: refcount underflow of ECU on inactive timer cancel
- [High] can: j1939: unintended global disruption of active address claims
- [Medium] can: j1939: memory leak of ECU from ignored running timer
--

commit 8541ad61eedf789fa9a61f3ebd58354bc6c07318
Author: Dmitry Antipov <dmantipov@yandex.ru>

can: j1939: cancel all pending ECUs on device stop

Cancel all pending ECUs in j1939_netdev_stop() to prevent memory leaks when a
stopped device no longer processes ECUs. This introduces j1939_ecu_cancel_all()
using hrtimer_try_to_cancel() to avoid deadlocks.

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
> index cdc3c0a719379..0b5232b536051 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -96,6 +96,17 @@ 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)
> +		if (hrtimer_try_to_cancel(&ecu->ac_timer) != -1)
> +			j1939_ecu_put(ecu);

[Severity: Critical]
Does this code inadvertently drop a reference that the timer does not own?

If the timer is inactive, hrtimer_try_to_cancel() returns 0. Since 0 is != -1,
the condition evaluates to true, and j1939_ecu_put() is called unconditionally
for inactive timers. Could this lead to a refcount underflow and use-after-free
if a user binds and then closes a socket?

[Severity: Medium]
Will this leak the ECU and the associated j1939_priv if the timer callback is
currently running?

If hrtimer_try_to_cancel() returns -1, the -1 state is dropped without waiting
or retrying, abandoning the active timer during teardown.

If the abandoned callback j1939_ecu_timer_handler() begins executing and later
calls j1939_ecu_map_locked() after unmap_all has finished, might this map the
ECU back into the stopped device and prevent it from ever being freed?

> +	write_unlock_bh(&priv->lock);
> +}
> +

[ ... ]

> 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]
Can this unintentionally cancel all ECU timers interface-wide on a single
socket's close event before checking the interface reference count?

Because j1939_ecu_cancel_all(priv) is called unconditionally before the
priv->rx_kref reference check, any user creating and closing a socket could
unintentionally disrupt network interface state and abort in-progress address
claims for all other applications using the same interface.

>  	j1939_priv_put(priv);
>  }

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

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

[netdev-bot+sashiko]

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

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

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

- [Critical] Refcount underflow leading to use-after-free and double free
  in j1939_ecu_cancel_all() (net/can/j1939/bus.c:105).
- [High] Incomplete fix: the ECU/j1939_priv leak the patch is meant to
  close can still happen.
- [High] j1939_ecu_cancel_all() runs on every per-socket stop, not only
  when the device actually stops.
- [Low] Stale comment in j1939_ecu_timer_start()
  (net/can/j1939/bus.c:123-125).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  j1939_ac_process() has an ABBA deadlock.

--

can: j1939: cancel all pending ECUs on device stop

This adds j1939_ecu_cancel_all(). It walks priv->ecus under priv->lock,
cancels each ECU's address-claim hrtimer with hrtimer_try_to_cancel() and
drops the ECU reference held by the timer. The helper is called from
j1939_netdev_stop() before the rx_kref put, so that ECUs and their
j1939_priv are not leaked after the device stops.

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

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..0b5232b536051 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -96,6 +96,17 @@ 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)
> +		if (hrtimer_try_to_cancel(&ecu->ac_timer) != -1)
> +			j1939_ecu_put(ecu);

[Severity: Critical]
Can this drop a reference that the timer never took? hrtimer_try_to_cancel()
returns 0 when the timer is not active, 1 when it removed a queued timer,
and -1 when the callback is running. The != -1 test also calls
j1939_ecu_put() when the return value is 0.

The timer's reference is taken in j1939_ecu_timer_start() just before
hrtimer_start(). It is dropped either by the handler or by a cancel that
returns non-zero. The existing helper follows that rule:

void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
{
	if (hrtimer_cancel(&ecu->ac_timer))
		j1939_ecu_put(ecu);
}

An idle ac_timer is the normal state once the 250 ms claim window has
passed. So every j1939_netdev_stop() call drops a reference it does not own
from most ECUs on priv->ecus.

For example, take a socket bound with NAME N and SA S. It has sent an
Address Claimed frame, and the ECU was mapped after 250 ms. The ECU kref is
now 2 (one from bind, one from ents[S]). On close():

j1939_sk_release()
  j1939_local_ecu_put()              kref 2 -> 1
  j1939_netdev_stop()
    j1939_ecu_cancel_all()
      hrtimer_try_to_cancel() == 0
      j1939_ecu_put()                kref 1 -> 0, kfree(ecu)
    kref_put_mutex()
      __j1939_rx_release()
        j1939_ecu_unmap_all()
          j1939_ecu_unmap_locked(priv->ents[S].ecu)

__j1939_ecu_release() does list_del() and kfree() but leaves
priv->ents[S].ecu pointing at the freed ECU. j1939_ecu_unmap_locked() then
reads ecu->priv and ecu->addr, writes ent->nusers and calls j1939_ecu_put()
on the freed object a second time.

If this is not the last rx_kref, the stale ents[] entry can still be reached
from the RX softirq. For example, j1939_ac_recv()->j1939_ecu_get_by_addr()
would call kref_get() on freed memory. Remote ECUs that are held only by
ents[] (kref 1) would also be freed this way whenever any socket is closed.

Should this only call j1939_ecu_put() when hrtimer_try_to_cancel() returns
1, as j1939_ecu_timer_cancel() does?

[Severity: Low]
This isn't a bug, but the comment in j1939_ecu_timer_start() still lists
only two places that release the timer's reference:

	/* The ECU is held here and released in the
	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel().
	 */

Should it also mention j1939_ecu_cancel_all()?

[Severity: High]
This isn't a bug introduced by this patch, but the commit message gives the
deadlock on priv->lock between j1939_ecu_timer_handler() and a synchronous
cancel as the reason for using hrtimer_try_to_cancel() here. The same
pattern is already present in j1939_ac_process():

net/can/j1939/address-claim.c:j1939_ac_process() {
    ...
	write_lock_bh(&priv->lock);
    ...
	/* cancel pending (previous) address claim */
	j1939_ecu_timer_cancel(ecu);
    ...
}

j1939_ecu_timer_cancel() calls hrtimer_cancel(), which waits for a running
callback. The callback starts with:

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

CPU0 (NET_RX softirq)
j1939_can_recv()->j1939_ac_recv()->j1939_ac_process()
  write_lock_bh(&priv->lock)
  j1939_ecu_timer_cancel()
    hrtimer_cancel()     waits while try_to_cancel returns -1

CPU1 (hrtimer softirq, the ECU's ac_timer just expired)
j1939_ecu_timer_handler()
  write_lock_bh(&priv->lock)   spins, held by CPU0

Can these two CPUs lock up waiting on each other? The path is reached when
a claim arrives for a known NAME with a different SA while that ECU's
previous 250 ms timer is expiring. On PREEMPT_RT, the wait is on
softirq_expiry_lock, which is held by the timer thread that is blocked on
priv->lock, so it looks like it still deadlocks there.

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

[ ... ]

> 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]
Is this the right place for a device-wide cancel? j1939_netdev_stop() runs
once per socket: from j1939_sk_release(), from the j1939_sk_bind() error
path and from j1939_sk_netdev_event_unregister(). Only the final rx_kref
drop reaches __j1939_rx_release().

As written, closing any single J1939 socket cancels the pending address
claim timers of every ECU on the interface. That includes local ECUs of
other sockets that are still bound, and remote nodes. RX stays active the
whole time.

A cancelled claim never reaches j1939_ecu_map_locked(). j1939_ac_process()
has already stored ecu->addr = sa before arming the timer, so a repeated
claim with the same NAME and SA takes the early exit and never re-arms the
timer:

	if (ecu && ecu->addr == skcb->addr.sa) {
		...
		goto out_ecu_put;
	}

The surviving socket's ECU then stays unmapped. j1939_name_to_addr()
returns J1939_IDLE_ADDR, and j1939_ac_fixup() fails that socket's
NAME-based sends with -EADDRNOTAVAIL. A local user who opens and closes
J1939 sockets faster than every 250 ms could keep ECUs on the bus from ever
completing their address claim.

The subject says "on device stop". Would it work better to cancel from
__j1939_rx_release(), next to the existing j1939_ecu_unmap_all()?

[Severity: High]
Can an ECU still be mapped after the final j1939_ecu_unmap_all(), leaving
the same j1939_priv and netdev reference leak this patch is meant to fix?
There appear to be two ways.

First, when hrtimer_try_to_cancel() returns -1, j1939_ecu_cancel_all()
skips that ECU and nothing waits for the callback later. If the running
j1939_ecu_timer_handler() gets priv->lock only after this sequence:

__j1939_rx_release()
  j1939_can_rx_unregister(priv);
  j1939_ecu_unmap_all(priv);
  j1939_priv_set(priv->ndev, NULL);

it calls j1939_ecu_map_locked() and takes a new ents[] reference that
nothing releases. This window is wider on PREEMPT_RT, where soft hrtimers
run in a preemptible thread.

Second, the CAN receiver stays registered between j1939_ecu_cancel_all()
and j1939_can_rx_unregister(). can_rx_unregister() also only does
hlist_del_rcu() and call_rcu(), so it does not wait for in-flight
j1939_can_recv() calls. An Address Claimed frame processed in that window
re-arms a timer that holds a reference:

j1939_can_recv()->j1939_ac_recv()->j1939_ac_process()
  j1939_ecu_timer_start(ecu);

250 ms later, the handler maps the ECU after j1939_ecu_unmap_all() has
already run. j1939_priv_set(ndev, NULL) has made priv unreachable from the
notifiers, so that mapping is never undone.

In both cases the ECU keeps its j1939_priv reference and netdev tracker,
and unregister_netdevice() hangs. Would it be better to cancel only after
the RX path is quiesced (or to check a stopping flag under priv->lock), and
to wait for or undo running callbacks in the final rx_kref release?

>  	j1939_priv_put(priv);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002091552.737529-1-dmantipov%40yandex.ru