Re: [PATCH net 01/20] can: j1939: use netdevice_tracker for j1939_{priv,session,ecu} tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Teardown race in j1939_ecu_timer_handler causes j1939_priv and net_device leak.
- [High] __j1939_ecu_release() modifies the priv->ecus list via list_del(&ecu->list) without holding priv->lock, causing list corruption if the last reference is dropped concurrently.
--

--- Patch [1]: [PATCH net 01/20] can: j1939: use netdevice_tracker for j1939_{priv,session,ecu} tracking ---
commit 65a0f5e1134c318a0874d847f3b02716f80d26b7
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

can: j1939: use netdevice_tracker for j1939_{priv,session,ecu} tracking

To investigate a dev_hold()/dev_put() imbalance in j1939_priv management,
this commit adds netdevice_tracker support to j1939_{priv,session,ecu}.
This helps debug refcount leaks reported by syzbot when unregistering
vcan0 devices.

Link: https://patch.msgid.link/20260728055835.1151785-2-o.rempel@pengutronix.de

> This refcount leak in j1939_priv might be caused by a refcount leak in
> j1939_{session,ecu} because j1939_{session,ecu} holds a ref on j1939_priv.
> For further investigation using upstream kernels, enable netdevice_tracker
> in j1939_{priv,session,ecu} management.

[Severity: High]
This is a pre-existing issue, but regarding the syzbot report about the
unregister_netdevice hang and the refcount leak mentioned above:

Could this leak be caused by a teardown race involving
j1939_ecu_timer_handler()?

When the interface is brought down, __j1939_rx_release() unmaps existing
ECUs via j1939_ecu_unmap_all(), but does it cancel the pending ac_timer
hrtimers?

If the timer fires after teardown, j1939_ecu_timer_handler() unconditionally
maps the ECU and takes a reference:

	j1939_ecu_map_locked(ecu);

Since the unmap phase has already passed, the ECU permanently holds a
reference to priv, which holds a reference to the net_device, preventing
unregister_netdevice from completing.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index dc374286eeb6e..cdc3c0a719379 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -20,6 +20,7 @@ static void __j1939_ecu_release(struct kref *kref)
>  	struct j1939_priv *priv = ecu->priv;
>  
>  	list_del(&ecu->list);

[Severity: High]
This is a pre-existing issue, but does __j1939_ecu_release() safely modify
priv->ecus without holding priv->lock here?

If j1939_ecu_put() is called from lockless contexts like j1939_ac_recv() or
j1939_ac_fixup(), and another thread drops the penultimate reference while
holding priv->lock, could the final reference be dropped in the lockless
context?

This would cause list_del() to modify the list concurrently with other
threads iterating or modifying it.

> +	netdev_put(priv->ndev, &ecu->priv_dev_tracker);
>  	kfree(ecu);
>  	j1939_priv_put(priv);
>  }

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