Re: [PATCH net-next 06/37] can: j1939: cancel pending address claim timers from j1939_ecu_unmap_all()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] can: j1939: list corruption in __j1939_ecu_release due to unlocked list_del
- [High] can: j1939: incomplete fix allows timer refcount leak via concurrent RX packets
--

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

can: j1939: cancel pending address claim timers from j1939_ecu_unmap_all()

This commit cancels pending address claim timers from j1939_ecu_unmap_all()
to fix a struct j1939_ecu refcount leak reported by syzbot. It ensures timers
are cleanly stopped when bringing down the interface to prevent an unmapped
ECU from persisting.

Link: https://patch.msgid.link/9ba38419-967a-4d47-bcdf-bbb2fa7d3e9f@I-love.SAKURA.ne.jp

> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -99,8 +99,38 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
>  void j1939_ecu_unmap_all(struct j1939_priv *priv)
>  {
>  	int i;
> +	struct j1939_ecu *ecu;
>  
>  	write_lock_bh(&priv->lock);

[Severity: High]
Does this completely prevent the refcount leak if a concurrent RX packet is
being processed?

In __j1939_rx_release(), j1939_can_rx_unregister() uses call_rcu() and does
not block before calling j1939_ecu_unmap_all():

net/can/j1939/main.c:__j1939_rx_release()
    j1939_can_rx_unregister(priv);
    j1939_ecu_unmap_all(priv);

Since the eventual synchronize_rcu() in j1939_sk_release() happens later,
a concurrent can_receive() could process a J1939_PGN_ADDRESS_CLAIMED frame,
acquire priv->lock after j1939_ecu_unmap_all() drops it, and invoke
j1939_ecu_timer_start() in j1939_ac_process().

Would this spawn a new active timer and map the ECU again after all cleanup
has completed, defeating the fix and hanging the netdev unregister?

> +	/* Cancel all pending address claim timers before unmapping.
> +	 * This prevents an orphaned late-firing timer from re-mapping
> +	 * an ECU after the spaces are cleared.
> +	 */
> +rescan_timers:
> +	list_for_each_entry(ecu, &priv->ecus, list) {
> +		int ret = hrtimer_try_to_cancel(&ecu->ac_timer);
> +
> +		if (ret == 0) { /* Not active or already finished. */
> +			continue;
> +		} else if (ret == 1) { /* Successfully canceled. */
> +			/* Drop a ref from j1939_ecu_timer_start(). */
> +			j1939_ecu_put(ecu);

[Severity: High]
By dropping the timer's reference early here, could we shift the final
reference drop to the network receive paths like j1939_ac_recv() or
j1939_ac_process()?

When an ECU's final reference drops, j1939_ecu_put() triggers
__j1939_ecu_release() which removes the ECU from priv->ecus:

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

Since paths like j1939_ac_recv() and j1939_ac_process() call j1939_ecu_put()
completely unlocked, could this list_del() execute concurrently with list
traversals under read_lock_bh(&priv->lock), leading to list corruption?

> +		} else { /* Executing on another CPU. */
> +			/* Let j1939_ecu_timer_cancel() determine whether to
> +			 * drop a ref from j1939_ecu_timer_start(). But guard
> +			 * with a local ref in order to make sure that unlocked
> +			 * j1939_ecu_put() from j1939_ecu_timer_cancel() cannot
> +			 * be the final reference.
> +			 */
> +			j1939_ecu_get(ecu);
> +			write_unlock_bh(&priv->lock);
> +			j1939_ecu_timer_cancel(ecu);
> +			write_lock_bh(&priv->lock);
> +			j1939_ecu_put(ecu);
> +		}
> +		goto rescan_timers;
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009134323.64064-1-mkl@pengutronix.de?part=6