Re: [PATCH v2] can: j1939: cancel all pending ECUs on device stop
From: sashiko-bot@kernel.org
Date: 2026-08-27 11:33:43
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 [off-list ref]
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
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+ write_unlock_bh(&priv->lock); +} +
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
+ } 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