Re: [PATCH can v1 1/2] can: j1939: use netdevice_tracker for j1939_{priv,session,ecu} tracking

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

Pre-existing issues:
- [High] ECU list `priv->ecus` is modified locklessly during concurrent unmapping.
--

--- Patch [1]: [PATCH can v1 1/2] can: j1939: use netdevice_tracker for j1939_{priv,session,ecu} tracking ---
commit fa6ae711cbc8e468a294fc1a54de5efa19c0b5b3
Author: Tetsuo Handa <[email protected]>

can: j1939: use netdevice_tracker for j1939_{priv,session,ecu} tracking
    
This patch enables netdevice_tracker for j1939_priv, j1939_session, and
j1939_ecu structures. It aims to help identify the source of refcount
leaks reported by syzbot during unregister_netdevice().

> 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 is it safe to modify priv->ecus here without
holding priv->lock?

If a process unbinds a socket and drops its ECU reference concurrently with the
network receive path processing a packet:

CPU1 (Socket thread)
j1939_local_ecu_put()
  /* drops refcount to 1 */

CPU2 (SoftIRQ)
j1939_ac_recv()
  j1939_ecu_put(ecu); /* locklessly drops final reference */

This leads to __j1939_ecu_release() executing list_del(&ecu->list) and
kfree(ecu) without holding priv->lock. 

Can this happen concurrently with other threads iterating priv->ecus under
priv->lock, potentially resulting in data races, list corruption, and
use-after-free bugs?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1