Re: [PATCH v2] can: j1939: cancel pending address claim timers on rx release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: j1939: list corruption via lockless j1939_ecu_put()
--

commit 0dc29971aa78a56ec257a9c34f0e743a7e1b0390
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

can: j1939: cancel pending address claim timers on rx release

This commit attempts to fix a struct j1939_ecu refcount leak by cancelling
address claim timers before calling j1939_ecu_unmap_all() during device
teardown. It adds a rescan loop in __j1939_rx_release() to safely cancel
timers while avoiding deadlocks.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928193312.553632-1-mkl%40pengutronix.de

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index 5e5e6c228f225..3a4de839cf9a2 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c

[ ... ]

> @@ -212,8 +212,38 @@ static void __j1939_rx_release(struct kref *kref)
>  {
>  	struct j1939_priv *priv = container_of(kref, struct j1939_priv,
>  					       rx_kref);
> +	struct j1939_ecu *ecu;
>  
>  	j1939_can_rx_unregister(priv);
> +
> +	/* can_rx_unregister() uses call_rcu() internally and is asynchronous.
> +	 * We must wait for an RCU grace period to ensure that any in-flight
> +	 * j1939_can_recv() instances on other CPUs have fully completed.
> +	 */
> +	synchronize_rcu();
> +
> +	/* Cancel all pending address claim timers before unmapping the ECUs.
> +	 * This prevents an orphaned timer from re-mapping an ECU after the
> +	 * rx path has been completely torn down.
> +	 */
> +rescan_timers:
> +	write_lock_bh(&priv->lock);
> +	list_for_each_entry(ecu, &priv->ecus, list) {
> +		int ret = hrtimer_try_to_cancel(&ecu->ac_timer);
> +
> +		if (ret == 0) /* Was not running. */
> +			continue;
> +		kref_get(&ecu->kref);
> +		write_unlock_bh(&priv->lock);
> +		if (ret == -1) /* Was running. Wait without lock. */
> +			j1939_ecu_timer_cancel(ecu);
> +		else /* Was canceled. Drop ref from j1939_ecu_timer_start(). */
> +			j1939_ecu_put(ecu);
> +		j1939_ecu_put(ecu);

[Severity: High]
Could these lockless j1939_ecu_put() calls trigger list corruption?

When __j1939_rx_release() cancels the timer, it drops priv->lock before
dropping the references. If this ecu's reference count drops to zero here
(for example, if the timer was the last reference), __j1939_ecu_release() 
will run locklessly.

Looking at __j1939_ecu_release() in net/can/j1939/bus.c, it modifies the 
list without acquiring priv->lock:

net/can/j1939/bus.c:__j1939_ecu_release() {
    ...
    list_del(&ecu->list);
    ...
}

Does this deterministic path lead to a kernel panic when other threads traverse
or modify priv->ecus during device teardown?

As an aside, looking at j1939_ac_recv() in net/can/j1939/address-claim.c, it
has a pre-existing example of dropping a reference locklessly. Is it possible 
that the underlying list deletion vulnerability exists elsewhere as well?

> +		goto rescan_timers;
> +	}
> +	write_unlock_bh(&priv->lock);
> +
>  	j1939_ecu_unmap_all(priv);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/853e770e-4eee-4da8-8cf0-08731f9651ab@I-love.SAKURA.ne.jp?part=1