[PATCH net] can: j1939: avoid address-claim timer deadlock

[PATCH net] can: j1939: avoid address-claim timer deadlock

[Felix Hoffmann]

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)) {
+		hrtimer_forward_now(hrtimer, ms_to_ktime(1));
+		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);
 
 	return HRTIMER_NORESTART;
 }
-- 
2.43.0

Re: [PATCH net] can: j1939: avoid address-claim timer deadlock

[Oleksij Rempel]

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 |

[PATCH can v2] can: j1939: avoid address-claim timer deadlock

[Felix Hoffmann]

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

Re: [PATCH can v2] can: j1939: avoid address-claim timer deadlock

[Oleksij Rempel]

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 |