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

[Felix Hoffmann <f3lix.dev@gmx.de>]

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