Re: [PATCH v11 3/3] can: j1939: fix "struct j1939_ecu" refcount leak

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not mention the net/can/j1939/socket.c
  hunk at all.
- [Low] In j1939_ecu_unmap_all() (net/can/j1939/bus.c), the comment on
  the second j1939_ecu_put() reads "Drop the ref from default: above."…

--

can: j1939: fix "struct j1939_ecu" refcount leak

This patch fixes a j1939_ecu refcount leak. The leak happens when
j1939_ecu_timer_handler() remaps an ECU after j1939_ecu_unmap_all() has
already unmapped it. The patch cancels ecu->ac_timer in
j1939_ecu_unmap_all() and adds j1939_ecu_verify_get(), which stops
j1939_ecu_timer_start() and j1939_ecu_map_locked() from taking new refs
once the netdev is unregistered or priv->rx_kref has reached zero.

Closes: https://syzkaller.appspot.com/bug?extid=e2af46126e0644cbebdd

> We might want to add "(READ_ONCE(priv->ndev->flags) & IFF_UP) &&" to
> j1939_ecu_verify_get() for (2) and (3) if we want j1939_ecu_unmap_all()
>  from j1939_netdev_notify(NETDEV_DOWN) path to behave like
> j1939_ecu_unmap_all() from j1939_netdev_notify(NETDEV_UNREGISTER) path,
> but that should be a different patch. This patch focuses on fixing
> j1939_ecu_unmap_all() from j1939_netdev_notify(NETDEV_UNREGISTER) path
> which is reported as
> "unregister_netdevice: waiting for vcan0 to become free. Usage count = 3"
> problem by syzbot.

[Severity: Low]
Is this description of the call path accurate? In j1939_netdev_notify(),
only the NETDEV_DOWN case calls j1939_ecu_unmap_all() directly:

net/can/j1939/main.c:j1939_netdev_notify() {
    ...
	case NETDEV_DOWN:
		j1939_cancel_active_session(priv, NULL);
		j1939_sk_netdev_event_netdown(priv);
		j1939_ecu_unmap_all(priv);
		break;
	case NETDEV_UNREGISTER:
		j1939_cancel_active_session(priv, NULL);
		j1939_sk_netdev_event_netdown(priv);
		j1939_sk_netdev_event_unregister(priv);
		break;
    ...
}

The NETDEV_UNREGISTER path reaches j1939_ecu_unmap_all() only indirectly,
and only when the last rx_kref is dropped:

j1939_sk_netdev_event_unregister()
  j1939_netdev_stop()
    kref_put_mutex(&priv->rx_kref, __j1939_rx_release, ...)
      __j1939_rx_release()
        j1939_ecu_unmap_all()

The commit message also doesn't mention the net/can/j1939/socket.c hunk.
That hunk changes the reg_state check in j1939_sk_bind() to use
READ_ONCE(). It also adds a long comment explaining why a bind() racing
with NETDEV_UNREGISTER needs no recheck. The new reg_state test in
j1939_ecu_verify_get() relies on that reasoning.

Could the changelog describe the indirect UNREGISTER path and the
j1939_sk_bind() change?

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index c6278d7cb1b1f..f444a1f33582b 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c

[ ... ]

> @@ -130,8 +141,31 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
>  void j1939_ecu_unmap_all(struct j1939_priv *priv)
>  {
>  	int i;
> +	struct j1939_ecu *ecu;
>  
> +restart:
>  	write_lock_bh(&priv->lock);

[ ... ]

> +	list_for_each_entry(ecu, &priv->ecus, list) {
> +		if (!j1939_ecu_valid(ecu))
> +			continue;
> +		if (j1939_ecu_timer_cancel(ecu))
> +			continue;
> +		/* Get a ref, in order to call hrtimer_cancel() without priv->lock. */
> +		if (!j1939_ecu_get(ecu))
> +			continue;
> +		write_unlock_bh(&priv->lock);
> +		if (hrtimer_cancel(&ecu->ac_timer))
> +			/* Drop the ref from j1939_ecu_timer_start(). */
> +			j1939_ecu_put(ecu);
> +		/* Drop the ref from default: above. */
> +		j1939_ecu_put(ecu);

[Severity: Low]
This isn't a bug, but does "default: above" point to the right place?
j1939_ecu_unmap_all() has no default: label. The nearest one is the
"Currently executing" case in j1939_ecu_timer_cancel(), and that case
takes no reference:

	default: /* Currently executing. */
		/* Let the caller decide how to handle this situation. */
		return false;

The ref dropped here comes from the j1939_ecu_get(ecu) call a few lines
earlier in the same loop.

This file tracks get/put pairs through comments like this one. Could the
comment name that j1939_ecu_get() call instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/a1dd9760-7815-400a-a39c-a1a03cf8262e%40I-love.SAKURA.ne.jp