* Re: [PATCH net] can: j1939: avoid address-claim timer deadlock
2026-07-31 13:42 [PATCH net] can: j1939: avoid address-claim timer deadlock Felix Hoffmann
@ 2026-08-01 13:43 ` sashiko-bot
2026-08-03 6:18 ` Oleksij Rempel
2026-08-03 10:15 ` [PATCH can v2] " Felix Hoffmann
2 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-08-01 13:43 UTC (permalink / raw)
To: Felix Hoffmann
Cc: Marc Kleine-Budde, Vincent Mailhol, Oleksij Rempel, linux-can,
Oliver Hartkopp
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-after-free on `priv->lock` during ECU timer completion.
- [High] Data race and unprotected `list_del()` causing list corruption on ECU release.
- [Medium] Memory leak of unmapped ECUs and `j1939_priv` due to missing timer cancellation.
- [Medium] Logic Bug: Resurrecting lost address claim contention.
--
--- Patch [1]: [PATCH net] can: j1939: avoid address-claim timer deadlock ---
commit a5db6a778fcc4768a707910335bdf934adc29468
Author: Felix Hoffmann <f3lix.dev@gmx.de>
can: j1939: avoid address-claim timer deadlock
j1939_ac_process() holds priv->lock while synchronously canceling an
ECU's address-claim hrtimer. The timer callback takes the same lock. If the
callback starts on another CPU after the receive path takes the lock, the
callback waits for priv->lock while hrtimer_cancel() waits for the callback
to finish. This deadlocks both CPUs and makes the system unresponsive.
> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..ac654dc8872e6 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -131,7 +131,17 @@ 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);
> + /*
> + * j1939_ac_process() cancels this timer while holding priv->lock.
> + * Don't block here, otherwise the timer and receive paths can deadlock
[Severity: Medium]
This isn't a bug introduced by this patch, but does j1939_ac_process() always
cancel the timer?
If a new address claim takes precedence over an existing claimant (prev),
prev is evicted via j1939_ecu_unmap_locked(prev). However, it appears its
pending ac_timer is not cancelled.
If the winning ECU vacates the address before prev's timer expires, could the
address slot become free, allowing prev's timer to fire and blindly re-map
itself without broadcasting an Address Claim message?
> + * waiting for each other on different CPUs. Retry shortly if address
> + * claim processing currently owns the lock.
> + */
> + if (!write_trylock(&priv->lock)) {
> + hrtimer_forward_now(hrtimer, ms_to_ktime(1));
> + return HRTIMER_RESTART;
> + }
> +
> /* TODO: can we test if ecu->addr is unicast before starting
> * the timer?
> */
> j1939_ecu_map_locked(ecu);
[Severity: Medium]
This is a pre-existing issue, but can this lead to a memory leak when an
interface goes down?
When a network interface goes down, j1939_ecu_unmap_all() clears the ECU
entries from priv->ents. If it fails to cancel the pending ac_timer for the
ECUs, could this handler execute and call j1939_ecu_map_locked(ecu)?
Since the slot was cleared, the ECU would be successfully re-mapped and its
reference count incremented, potentially leaving it leaked along with the
j1939_priv object since the interface is already down.
>
> /* The corresponding j1939_ecu_get() is in
> * j1939_ecu_timer_start().
> */
> j1939_ecu_put(ecu);
> - write_unlock_bh(&priv->lock);
> + write_unlock(&priv->lock);
[Severity: High]
This isn't a bug introduced by this patch, but could this write_unlock()
trigger a use-after-free?
If the timer holds the last reference to the ECU, j1939_ecu_put(ecu) drops its
refcount to 0, invoking __j1939_ecu_release(). This in turn calls
j1939_priv_put(priv).
If this drops the final reference to the j1939_priv structure (e.g., after a
network interface teardown dropped the base reference), priv is synchronously
freed via kfree(). Immediately after j1939_ecu_put() returns, the handler
dereferences the freed memory by calling write_unlock(&priv->lock).
[Severity: High]
This is also a pre-existing issue, but is there a risk of list corruption
when j1939_ecu_put() drops the final reference without priv->lock being held
in other contexts?
While priv->lock is held here, j1939_ecu_put() is called from functions
like j1939_ac_recv() in address-claim.c without holding this lock.
If j1939_ecu_put() drops the final reference, __j1939_ecu_release() executes
list_del(&ecu->list), which assumes priv->lock is held for safe modification
of priv->ecus. Could concurrent operations trigger this and silently corrupt
the list?
>
> return HRTIMER_NORESTART;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731134255.1002902-1-f3lix.dev@gmx.de?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] can: j1939: avoid address-claim timer deadlock
2026-07-31 13:42 [PATCH net] can: j1939: avoid address-claim timer deadlock Felix Hoffmann
2026-08-01 13:43 ` sashiko-bot
@ 2026-08-03 6:18 ` Oleksij Rempel
2026-08-03 10:15 ` [PATCH can v2] " Felix Hoffmann
2 siblings, 0 replies; 6+ messages in thread
From: Oleksij Rempel @ 2026-08-03 6:18 UTC (permalink / raw)
To: Felix Hoffmann
Cc: linux-can, Robin van der Gracht, netdev, kernel, Oliver Hartkopp,
Marc Kleine-Budde, linux-kernel
Hi Felix,
Please do not send CAN related patches to the netdev list, and please
use [PATCH can] tag instead.
On Fri, Jul 31, 2026 at 03:42:55PM +0200, Felix Hoffmann wrote:
> j1939_ac_process() holds priv->lock while synchronously canceling an
> ECU's address-claim hrtimer. The timer callback takes the same lock. If the
> callback starts on another CPU after the receive path takes the lock, the
> callback waits for priv->lock while hrtimer_cancel() waits for the callback
> to finish. This deadlocks both CPUs and makes the system unresponsive.
>
> Do not wait for priv->lock from the soft hrtimer callback. If
> address-claim processing currently owns it, move the expiry forward by 1 ms
> and restart the timer. This lets a concurrent hrtimer_cancel() finish and
> remove the requeued timer. Without a cancellation, mapping is retried
> shortly.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:GPT5.6-Sol
> Signed-off-by: Felix Hoffmann <f3lix.dev@gmx.de>
> ---
>
> The deadlock was reproduced three times on a 2-vCPU kernel with KASAN
> and lockdep. Cross-CPU GDB stacks showed hrtimer_cancel() and the timer
> callback waiting on the same j1939_priv lock and hrtimer.
>
> With this change, the reproducer completed 420 stress rounds, and the
> patched CAN J1939 syzkaller campaign remained operational. The trigger
> drops to UID and GID 65534 before opening its CAN sockets. A minimal
> reproducer and the complete stack capture are available privately on
> request.
>
> net/can/j1939/bus.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a71937..ac654dc8872e 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -131,7 +131,17 @@ 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);
> + /*
> + * j1939_ac_process() cancels this timer while holding priv->lock.
> + * Don't block here, otherwise the timer and receive paths can deadlock
> + * waiting for each other on different CPUs. Retry shortly if address
> + * claim processing currently owns the lock.
> + */
> + if (!write_trylock(&priv->lock)) {
write_trylock() will not take a lock even if read lock is taken on the
receive path. Means, this operation will be delayed not only by the rare
address claim frames, but also by any frame on the bus.
Since, the problem is real, and this fix is good enough for stable, i'm
ok with it + some nit picks.
Update the comment and commit message to something like this:
/* j1939_ac_process() cancels this timer while holding priv->lock.
* Don't block here, otherwise the timer and receive paths can
* deadlock waiting for each other on different CPUs. Retry shortly
* if the lock is held, by the address claim writer or by a reader
* on the receive path.
*/
> + hrtimer_forward_now(hrtimer, ms_to_ktime(1));
Please add a define for 1 ms retry. No magic numbers.
If the bus load will really hit us so strong that we will retry for
too long, it will be good to have some rate limited warning after ~5
retries. Also comment on that 5 retires is not spec related, it is more
"seems to be reasonable" number.
> + return HRTIMER_RESTART;
> + }
> +
> /* TODO: can we test if ecu->addr is unicast before starting
> * the timer?
> */
> @@ -141,7 +151,7 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
> * j1939_ecu_timer_start().
> */
> j1939_ecu_put(ecu);
> - write_unlock_bh(&priv->lock);
> + write_unlock(&priv->lock);
Please explain in the commit message, why changing write_*_bh() to write_*()
is a safe option.
Best Regards,
Oleksij
--
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 |
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH can v2] can: j1939: avoid address-claim timer deadlock
2026-07-31 13:42 [PATCH net] can: j1939: avoid address-claim timer deadlock Felix Hoffmann
2026-08-01 13:43 ` sashiko-bot
2026-08-03 6:18 ` Oleksij Rempel
@ 2026-08-03 10:15 ` Felix Hoffmann
2026-08-03 10:43 ` sashiko-bot
2026-08-04 7:12 ` Oleksij Rempel
2 siblings, 2 replies; 6+ messages in thread
From: Felix Hoffmann @ 2026-08-03 10:15 UTC (permalink / raw)
To: linux-can, Robin van der Gracht, Oleksij Rempel
Cc: kernel, Oliver Hartkopp, Marc Kleine-Budde, linux-kernel
j1939_ac_process() holds priv->lock while synchronously canceling an
ECU's address-claim hrtimer. The timer callback takes the same lock. If the
callback starts on another CPU after the receive path takes the lock, the
callback waits for priv->lock while hrtimer_cancel() waits for the callback
to finish. This deadlocks both CPUs and makes the system unresponsive.
Replace the callback's blocking lock acquisition with write_trylock(). If
the lock is busy, move the expiry forward and restart the timer. This lets
a concurrent hrtimer_cancel() observe the callback finish and remove the
requeued timer. Without a cancellation, address mapping is retried shortly.
The failed write trylock does not necessarily mean that the address-claim
writer owns priv->lock. Receive processing also takes its read side for
other frames, and an active reader prevents write_trylock() from
succeeding. Document both the address-claim writer and receive-path readers
as possible sources of contention.
Name the 1 ms retry interval J1939_ECU_TIMER_RETRY_DELAY_MS instead of
using a literal in the callback. Track consecutive failures per ECU, reset
the counter when its timer starts, and emit a warning guarded by
net_ratelimit() on the fifth retry. Five is a pragmatic threshold for
detecting unusual delay; it is not specified by J1939.
The timer uses HRTIMER_MODE_REL_SOFT, so its callback runs from
HRTIMER_SOFTIRQ with bottom halves already disabled. write_trylock() does
not change bottom-half state and must therefore pair with write_unlock() on
success. The write_lock_bh()/write_unlock_bh() pair is unnecessary in this
context.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Assisted-by: Codex:GPT5.6-Sol
Signed-off-by: Felix Hoffmann <f3lix.dev@gmx.de>
---
Thanks to Oleksij Rempel for the review.
Changes in v2:
- use the CAN-specific patch prefix and omit the netdev list;
- clarify that both an address-claim writer and receive-path readers can
make write_trylock() fail;
- replace the literal 1 ms retry delay with a named constant;
- add a network-rate-limited warning on the fifth consecutive retry and
document that the threshold is pragmatic rather than protocol-defined;
- explain why write_trylock()/write_unlock() is safe in the soft hrtimer
callback without the _bh() variants.
The revised patch passed strict style checks, and the affected J1939 code
built successfully with additional compiler warnings enabled.
net/can/j1939/bus.c | 34 ++++++++++++++++++++++++++++++++--
net/can/j1939/j1939-priv.h | 1 +
2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..eb9a3fd847f4 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -10,10 +10,19 @@
* Since rtnetlink, no real bus is used.
*/
+#include <linux/net.h>
+
#include <net/sock.h>
#include "j1939-priv.h"
+#define J1939_ECU_TIMER_RETRY_DELAY_MS 1
+/*
+ * J1939 does not specify a lock-contention retry count. Five retries are a
+ * pragmatic threshold for warning about an unusual address-claim delay.
+ */
+#define J1939_ECU_TIMER_RETRY_WARN 5
+
static void __j1939_ecu_release(struct kref *kref)
{
struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
@@ -109,6 +118,8 @@ void j1939_ecu_unmap_all(struct j1939_priv *priv)
void j1939_ecu_timer_start(struct j1939_ecu *ecu)
{
+ ecu->ac_timer_retries = 0;
+
/* The ECU is held here and released in the
* j1939_ecu_timer_handler() or j1939_ecu_timer_cancel().
*/
@@ -131,7 +142,26 @@ 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);
+ /*
+ * j1939_ac_process() cancels this timer while holding priv->lock.
+ * Don't block here, otherwise the timer and receive paths can
+ * deadlock waiting for each other on different CPUs. Retry shortly
+ * if the lock is held by the address claim writer or by a reader on
+ * the receive path.
+ */
+ if (!write_trylock(&priv->lock)) {
+ ecu->ac_timer_retries++;
+ if (ecu->ac_timer_retries == J1939_ECU_TIMER_RETRY_WARN &&
+ net_ratelimit())
+ netdev_warn(priv->ndev,
+ "address claim timer retried %u times due to lock contention, name: 0x%016llx\n",
+ ecu->ac_timer_retries, ecu->name);
+
+ hrtimer_forward_now(hrtimer,
+ ms_to_ktime(J1939_ECU_TIMER_RETRY_DELAY_MS));
+ return HRTIMER_RESTART;
+ }
+
/* TODO: can we test if ecu->addr is unicast before starting
* the timer?
*/
@@ -141,7 +171,7 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
* j1939_ecu_timer_start().
*/
j1939_ecu_put(ecu);
- write_unlock_bh(&priv->lock);
+ write_unlock(&priv->lock);
return HRTIMER_NORESTART;
}
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..5ed4f4b592e3 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -36,6 +36,7 @@ struct j1939_ecu {
/* indicates that this ecu successfully claimed @sa as its address */
struct hrtimer ac_timer;
+ unsigned int ac_timer_retries;
struct kref kref;
struct j1939_priv *priv;
netdevice_tracker priv_dev_tracker;
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH can v2] can: j1939: avoid address-claim timer deadlock
2026-08-03 10:15 ` [PATCH can v2] " Felix Hoffmann
@ 2026-08-03 10:43 ` sashiko-bot
2026-08-04 7:12 ` Oleksij Rempel
1 sibling, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-08-03 10:43 UTC (permalink / raw)
To: Felix Hoffmann
Cc: Oliver Hartkopp, Vincent Mailhol, linux-can, Oleksij Rempel,
Marc Kleine-Budde
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Lockless calls to `j1939_ecu_put()` can drop the last reference to an ECU, triggering `__j1939_ecu_release()` and modifying `priv->ecus` locklessly.
- [High] Permanent memory leak of `j1939_ecu` and `j1939_priv` when the network interface is brought down while an address claim timer is pending.
--
--- Patch [1]: [PATCH can v2] can: j1939: avoid address-claim timer deadlock ---
commit 08d11140bccb342c8267ab26aee9e24877e324a3
Author: Felix Hoffmann <f3lix.dev@gmx.de>
can: j1939: avoid address-claim timer deadlock
j1939_ac_process() holds priv->lock while synchronously canceling an
ECU's address-claim hrtimer. The timer callback takes the same lock. If the
callback starts on another CPU after the receive path takes the lock, the
callback waits for priv->lock while hrtimer_cancel() waits for the callback
to finish. This deadlocks both CPUs and makes the system unresponsive.
Cc: stable@vger.kernel.org
Signed-off-by: Felix Hoffmann <f3lix.dev@gmx.de>
> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..eb9a3fd847f42 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -10,10 +10,19 @@
> * Since rtnetlink, no real bus is used.
> */
>
> +#include <linux/net.h>
> +
> #include <net/sock.h>
>
> #include "j1939-priv.h"
>
> +#define J1939_ECU_TIMER_RETRY_DELAY_MS 1
> +/*
> + * J1939 does not specify a lock-contention retry count. Five retries are a
> + * pragmatic threshold for warning about an unusual address-claim delay.
> + */
> +#define J1939_ECU_TIMER_RETRY_WARN 5
> +
> static void __j1939_ecu_release(struct kref *kref)
> {
> struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
> struct j1939_priv *priv = ecu->priv;
>
> list_del(&ecu->list);
[Severity: High]
This isn't a bug introduced by this patch, but could this lead to list
corruption?
If an ECU is concurrently unmapped (dropping its primary reference) while
another thread holds a temporary reference (e.g., in j1939_ac_recv() or
j1939_ac_fixup()), dropping the temporary reference locklessly via
j1939_ecu_put() will reduce the refcount to 0.
This triggers __j1939_ecu_release(), which performs list_del(&ecu->list) on
priv->ecus without holding priv->lock.
Can this be triggered remotely by sending precisely timed address claim and
release packets to cause a use-after-free or panic?
[ ... ]
> @@ -109,6 +118,8 @@ void j1939_ecu_unmap_all(struct j1939_priv *priv)
>
> void j1939_ecu_timer_start(struct j1939_ecu *ecu)
> {
[Severity: High]
This is a pre-existing issue, but does this leak j1939_ecu and j1939_priv when
the network interface is brought down?
When a J1939 interface is unregistered, j1939_ecu_unmap_all() clears mapped
ECUs from priv->ents, but it only iterates over mapped ECUs and ignores ECUs
in priv->ecus that have a pending timer:
net/can/j1939/bus.c:j1939_ecu_unmap_all() {
for (i = 0; i < ARRAY_SIZE(priv->ents); i++)
if (priv->ents[i].ecu)
j1939_ecu_unmap_locked(priv->ents[i].ecu);
}
It seems this fails to cancel pending ac_timer hrtimers for ECUs in the
middle of address claiming.
When the uncancelled timer subsequently fires, j1939_ecu_timer_handler()
executes and maps the ECU back into the now-abandoned j1939_priv.
Since interface teardown (j1939_netdev_notify()) won't run
j1939_ecu_unmap_all() again, does this cause a permanent leak blocking
unregister_netdevice()?
> + ecu->ac_timer_retries = 0;
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260803101503.21023-1-f3lix.dev@gmx.de?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH can v2] can: j1939: avoid address-claim timer deadlock
2026-08-03 10:15 ` [PATCH can v2] " Felix Hoffmann
2026-08-03 10:43 ` sashiko-bot
@ 2026-08-04 7:12 ` Oleksij Rempel
1 sibling, 0 replies; 6+ messages in thread
From: Oleksij Rempel @ 2026-08-04 7:12 UTC (permalink / raw)
To: Felix Hoffmann
Cc: linux-can, Robin van der Gracht, kernel, Oliver Hartkopp,
Marc Kleine-Budde, linux-kernel
Hi Felix,
Please send next patch stand alone, not in replay to the previous patch
or discussion.
On Mon, Aug 03, 2026 at 12:15:03PM +0200, Felix Hoffmann wrote:
> j1939_ac_process() holds priv->lock while synchronously canceling an
> ECU's address-claim hrtimer. The timer callback takes the same lock. If the
> callback starts on another CPU after the receive path takes the lock, the
> callback waits for priv->lock while hrtimer_cancel() waits for the callback
> to finish. This deadlocks both CPUs and makes the system unresponsive.
>
> Replace the callback's blocking lock acquisition with write_trylock(). If
> the lock is busy, move the expiry forward and restart the timer. This lets
> a concurrent hrtimer_cancel() observe the callback finish and remove the
> requeued timer. Without a cancellation, address mapping is retried shortly.
>
> The failed write trylock does not necessarily mean that the address-claim
> writer owns priv->lock. Receive processing also takes its read side for
> other frames, and an active reader prevents write_trylock() from
> succeeding. Document both the address-claim writer and receive-path readers
> as possible sources of contention.
>
> Name the 1 ms retry interval J1939_ECU_TIMER_RETRY_DELAY_MS instead of
> using a literal in the callback. Track consecutive failures per ECU, reset
> the counter when its timer starts, and emit a warning guarded by
> net_ratelimit() on the fifth retry. Five is a pragmatic threshold for
> detecting unusual delay; it is not specified by J1939.
>
> The timer uses HRTIMER_MODE_REL_SOFT, so its callback runs from
> HRTIMER_SOFTIRQ with bottom halves already disabled. write_trylock() does
> not change bottom-half state and must therefore pair with write_unlock() on
> success. The write_lock_bh()/write_unlock_bh() pair is unnecessary in this
> context.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:GPT5.6-Sol
> Signed-off-by: Felix Hoffmann <f3lix.dev@gmx.de>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Thank you!
Best Regards,
Oleksij
--
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 |
^ permalink raw reply [flat|nested] 6+ messages in thread