[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

[sashiko-bot]

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