Re: [PATCH v8 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[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] Undisclosed backport dependency.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a two-
  CPU deadlock.

--

can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

This commit makes j1939_ecu_map_locked() drop the ECU reference it just
took, and makes j1939_ecu_create_locked() fail with -ENODEV, when the
netdev is no longer registered or __j1939_rx_release() has already run.
This keeps j1939_ecu_timer_handler() from re-mapping an ECU after
j1939_ecu_unmap_all() has finished.

> For now we don't cancel ecu->ac_timer from j1939_ecu_unmap_all(), for
> we can't safely call hrtimer_cancel() with priv->lock held. However,
> since ecu->ac_timer expires in 250ms, for now let's tolerate delaying
> completion of NETDEV_UNREGISTER event for less than one second.

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Still,
the rule given here (hrtimer_cancel() is not safe with priv->lock held)
already seems to be broken in j1939_ac_process() in
net/can/j1939/address-claim.c.

j1939_ac_process() takes priv->lock for write. While still holding it, it
calls j1939_ecu_timer_cancel(), which calls hrtimer_cancel() on
ecu->ac_timer:

CPU0 (CAN RX softirq)
j1939_can_recv()->j1939_ac_recv()->j1939_ac_process()
    write_lock_bh(&priv->lock);
    ...
    /* cancel pending (previous) address claim */
    j1939_ecu_timer_cancel(ecu);
        hrtimer_cancel(&ecu->ac_timer);

The timer callback takes the same lock:

CPU1 (hrtimer softirq)
__run_hrtimer()->j1939_ecu_timer_handler()
    write_lock_bh(&priv->lock);

Say ac_timer expires on CPU1, and j1939_ecu_timer_handler() is already
spinning on priv->lock. At that moment CPU0 gets an ADDRESS_CLAIMED frame
with the same NAME but a different SA.

hrtimer_try_to_cancel() returns -1 while the callback is running, so
hrtimer_cancel() keeps retrying in hrtimer_cancel_wait_running(). On
!PREEMPT_RT that is cpu_relax(). On PREEMPT_RT it blocks on the softirq
expiry lock while the rwlock is held.

Can this leave both CPUs spinning forever with BH disabled? The trigger is
ADDRESS_CLAIMED frames, which any node on the bus or a local vcan sender can
send.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..77b6788060573 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -84,6 +84,15 @@ static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
>  	if (!j1939_ecu_get(ecu))
>  		return;
>  
> +	/* Drop the ref 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)) {
> +		j1939_ecu_put(ecu);
> +		return;
> +	}
> +

[Severity: Low]
This isn't a bug, but is there an undisclosed backport dependency here?

The context line "if (!j1939_ecu_get(ecu)) return;" comes from the earlier
patch "can: j1939: fix possibility of list corruption in
__j1939_ecu_release()". That patch turned j1939_ecu_get() into a
__must_check bool wrapper around kref_get_unless_zero(). It also moved the
call ahead of the ent->ecu check.

In the trees covered by the Fixes: tag, the code still looks like this:

static void j1939_ecu_get(struct j1939_ecu *ecu)
{
	kref_get(&ecu->kref);
}

and j1939_ecu_map_locked() calls it after the ent->ecu check. So this hunk
won't apply as-is.

The prerequisite has no Fixes: or Cc: stable tag, and this commit doesn't
mention it. Since this patch carries "Fixes: 9d71dd0c7009", stable tooling
will try to pick it up on its own.

Could the prerequisite get a Fixes:/Cc: stable tag, or could this commit
note the dependency? Either way, backporters would know whether to take
both patches or adapt this one by hand.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/3d7e48fa-9af5-43e2-84f7-884e61f9d179%40I-love.SAKURA.ne.jp