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

[PATCH v2] 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>
---
v2: add extra precaution to avoid ABBA deadlock between
    j1939_ecu_cancel_all() and timer callbacks (Sashiko)
---
 net/can/j1939/bus.c        | 39 ++++++++++++++++++++++++++------------
 net/can/j1939/j1939-priv.h |  6 ++++++
 net/can/j1939/main.c       |  2 ++
 3 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..e02454a4c64c 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;
@@ -131,18 +141,23 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
 		container_of(hrtimer, struct j1939_ecu, ac_timer);
 	struct j1939_priv *priv = ecu->priv;
 
-	write_lock_bh(&priv->lock);
-	/* TODO: can we test if ecu->addr is unicast before starting
-	 * the timer?
-	 */
-	j1939_ecu_map_locked(ecu);
-
-	/* The corresponding j1939_ecu_get() is in
-	 * j1939_ecu_timer_start().
-	 */
-	j1939_ecu_put(ecu);
-	write_unlock_bh(&priv->lock);
-
+	/* We're asked to stop from j1939_netdev_stop(). */
+	if (unlikely(atomic_read(&priv->stop))) {
+		lockdep_assert_held(&priv->lock);
+		j1939_ecu_put(ecu);
+	} else {
+		write_lock_bh(&priv->lock);
+		/* TODO: can we test if ecu->addr is unicast before starting
+		 * the timer?
+		 */
+		j1939_ecu_map_locked(ecu);
+
+		/* The corresponding j1939_ecu_get() is in
+		 * j1939_ecu_timer_start().
+		 */
+		j1939_ecu_put(ecu);
+		write_unlock_bh(&priv->lock);
+	}
 	return HRTIMER_NORESTART;
 }
 
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..1e8f018b7c8b 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -60,6 +60,11 @@ struct j1939_priv {
 	/* segments need a lock to protect the above list */
 	rwlock_t lock;
 
+	/* Used to avoid deadlock between j1939_ecu_cancel_all()
+	 * and timer callbacks.
+	 */
+	atomic_t stop;
+
 	struct net_device *ndev;
 	netdevice_tracker dev_tracker;
 
@@ -204,6 +209,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..22ea2b000185 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -306,6 +306,8 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
 
 void j1939_netdev_stop(struct j1939_priv *priv)
 {
+	atomic_set(&priv->stop, 1);
+	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 v2] can: j1939: cancel all pending ECUs on device stop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ABBA deadlock between `j1939_ecu_cancel_all()` and `j1939_ecu_timer_handler()` due to a TOCTOU race condition.
- [High] Use-After-Free in `hrtimer_cancel()` when the ECU is freed by the timer callback.
- [High] Lockless ECU reference drop in the fast-path causes concurrent list modification and corruption.
- [Medium] Erroneous `lockdep_assert_held()` check in the timer callback will cause false-positive warnings.
--

commit f501d51ab1fd28cee33a268e9dffc6800425d782
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
the stopped device from processing ECUs, avoiding scenarios where the
j1939_priv struct is never freed.
    
Closes: https://syzkaller.appspot.com/bug?extid=489e907b2a026a6f5fa0

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..9511134313a7d 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);

[Severity: High]
Does holding write_lock_bh(&priv->lock) during synchronous timer
cancellation here risk an ABBA deadlock?

j1939_ecu_timer_cancel() calls hrtimer_cancel(), which spins waiting for
the timer callback to finish. If j1939_ecu_timer_handler() runs
concurrently on another CPU, could it read priv->stop == 0 (due to a TOCTOU
race just before priv->stop is updated) and attempt to acquire the same
lock:

j1939_ecu_timer_handler() {
    ...
    } else {
        write_lock_bh(&priv->lock);
    ...
}

This sequence would leave hrtimer_cancel() spinning for the callback, while
the callback spins for priv->lock, resulting in a deadlock.

> +	write_unlock_bh(&priv->lock);
> +}
> +
[ ... ]
> @@ -131,18 +141,23 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
>  		container_of(hrtimer, struct j1939_ecu, ac_timer);
>  	struct j1939_priv *priv = ecu->priv;
>  
> +	/* We're asked to stop from j1939_netdev_stop(). */
> +	if (unlikely(atomic_read(&priv->stop))) {
> +		lockdep_assert_held(&priv->lock);

[Severity: Medium]
Is this lockdep assertion correct for an asynchronous timer callback?

Since hrtimer callbacks execute asynchronously in softirq context, the
callback does not inherently hold priv->lock, regardless of whether
j1939_netdev_stop() holds it on another CPU. Because lockdep verifies the
current execution context, won't this unconditionally trigger a warning?

> +		j1939_ecu_put(ecu);

[Severity: High]
Does dropping the ECU reference here without priv->lock risk list corruption
and a use-after-free?

If j1939_ecu_put() drops the final reference, __j1939_ecu_release() will be
called, which performs an unprotected list deletion:

__j1939_ecu_release() {
    ...
    list_del(&ecu->list);
    ...
}

Because this happens concurrently with j1939_ecu_cancel_all() traversing
the list using list_for_each_entry_safe() on the shutdown CPU, this
could corrupt the linked list.

Additionally, if the ECU is freed via kfree(ecu) in this path, what
happens to the CPU executing hrtimer_cancel(&ecu->ac_timer) inside
j1939_ecu_timer_cancel()? Will it dereference freed memory as it continues
to spin on the destroyed timer structure?

> +	} else {
> +		write_lock_bh(&priv->lock);
> +		/* TODO: can we test if ecu->addr is unicast before starting
> +		 * the timer?
> +		 */
> +		j1939_ecu_map_locked(ecu);
> +
> +		/* The corresponding j1939_ecu_get() is in
> +		 * j1939_ecu_timer_start().
> +		 */
> +		j1939_ecu_put(ecu);
> +		write_unlock_bh(&priv->lock);
> +	}
>  	return HRTIMER_NORESTART;
>  }

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