Re: [PATCH v9 2/2] can: j1939: fix "struct j1939_ecu" refcount leak

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: j1939: race between j1939_local_ecu_get() and
  j1939_jsk_add() on unbind
--

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

can: j1939: fix "struct j1939_ecu" refcount leak

This commit cancels ECU address-claim timers during j1939_ecu_unmap_all() and
blocks taking new ECU references or binding local ECUs once the netdev is no
longer registered or rx_kref reaches zero.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..7346248a18020 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c

[ ... ]

> @@ -310,6 +360,15 @@ int j1939_local_ecu_get(struct j1939_priv *priv, name_t name, u8 sa)
>  
>  	write_lock_bh(&priv->lock);
>  
> +	/* Do nothing and fail if netdev is no longer registered or
> +	 * __j1939_rx_release() was called.
> +	 */
> +	if ((READ_ONCE(priv->ndev->reg_state) != NETREG_REGISTERED ||
> +	     !kref_read(&priv->rx_kref))) {
> +		err = -ENODEV;
> +		goto done;
> +	}
> +
>  	if (j1939_address_is_unicast(sa))
>  		priv->ents[sa].nusers++;
>  

[Severity: Medium]
Can j1939_sk_bind() still race with NETDEV_UNREGISTER between
j1939_local_ecu_get() and j1939_jsk_add(), leaving the socket bound and
holding references to struct j1939_ecu, struct j1939_priv, and priv->ndev
after j1939_sk_netdev_event_unregister() has finished?

In j1939_sk_bind(), the socket is not on priv->j1939_socks (or has just been
removed by j1939_jsk_del() on re-bind) when j1939_local_ecu_get() runs, and
j1939_jsk_add() is only called after j1939_local_ecu_get() drops priv->lock:

net/can/j1939/socket.c:j1939_sk_bind() {
    ...
	/* get new references */
	ret = j1939_local_ecu_get(priv, jsk->addr.src_name, jsk->addr.sa);
	if (ret) {
    ...
	}

	j1939_jsk_add(priv, jsk);
    ...
}

Because unregister_netdevice_many_notify() sets NETREG_UNREGISTERING without
holding priv->lock or the socket lock, the two paths can interleave:

CPU0                                  CPU1
j1939_sk_bind()
  j1939_local_ecu_get()
    write_lock_bh(&priv->lock)
    reg_state == NETREG_REGISTERED
    j1939_ecu_create_locked()
    write_unlock_bh(&priv->lock)
                                      unregister_netdevice_many_notify()
                                        dev->reg_state = NETREG_UNREGISTERING
                                        j1939_netdev_notify()
                                          j1939_sk_netdev_event_unregister()
                                            /* jsk not in priv->j1939_socks */
  j1939_jsk_add(priv, jsk)
    /* jsk left bound after unregister */

j1939_sk_netdev_event_unregister() misses jsk because it only scans
priv->j1939_socks, so jsk remains bound with its ECU, priv, and netdev
references held until the socket is closed, blocking unregister_netdevice().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/396b5de7-3fe0-474f-9194-b4816fde7182@I-love.SAKURA.ne.jp?part=2