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

[sashiko-bot@kernel.org]

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