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

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(&e=
cu->list) without holding priv->lock, causing list corruption if the last r=
eference is dropped concurrently.
--

--- Patch [1]: [PATCH net 01/20] can: j1939: use netdevice_tracker for j193=
9_{priv,session,ecu} tracking ---
commit 65a0f5e1134c318a0874d847f3b02716f80d26b7
Author: Tetsuo Handa <[email protected]>

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@pengutroni=
x.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 =3D ecu->priv;
> =20
>  	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);
>  }

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260729102802.5051=
[email protected]?part=3D1