Re: [PATCH v11 2/3] can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] j1939_ac_process() (net/can/j1939/address-claim.c) now gives
  up on an Address Claimed frame whenever hrtimer_try_to_cancel()…
- [Low] On the new hrtimer_try_to_cancel() == -1 path, the in-code
  comment and Documentation/networking/j1939.rst no longer match the…

Pre-existing issues:
- [High] At the target commit, j1939_ecu_unmap_all()
  (net/can/j1939/bus.c) only walks priv->ents[] and calls…

--

can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()

The patch switches j1939_ecu_timer_cancel() from hrtimer_cancel() to
hrtimer_try_to_cancel(). This avoids waiting for a running ac_timer
callback while priv->lock is held for write. If the callback is running,
j1939_ecu_timer_cancel() now returns false and j1939_ac_process() ignores
the address claim frame.

> diff --git a/net/can/j1939/address-claim.c b/net/can/j1939/address-claim.c
> index ca4ad6cdd5cbf..89ba8c6fbe74d 100644
> --- a/net/can/j1939/address-claim.c
> +++ b/net/can/j1939/address-claim.c
> @@ -212,7 +212,8 @@ static void j1939_ac_process(struct j1939_priv *priv, struct sk_buff *skb)
>  		goto out_unlock_bh;
>  
>  	/* cancel pending (previous) address claim */
> -	j1939_ecu_timer_cancel(ecu);
> +	if (!j1939_ecu_timer_cancel(ecu))
> +		goto out_ecu_put;

[Severity: Medium]

If the previous ac_timer callback is running, this path skips several
steps: the idle/unmap handling, the ecu->addr update, the contention
check against prev, and j1939_ecu_timer_start(). Could the kernel then
end up committing the superseded claim instead of the new one?

Once j1939_ac_process() releases priv->lock, the running handler takes
it and maps the ECU at the old, unchanged ecu->addr:

net/can/j1939/bus.c:j1939_ecu_timer_handler() {
	write_lock_bh(&priv->lock);
	...
	j1939_ecu_map_locked(ecu);
	...
	write_unlock_bh(&priv->lock);
}

Say the new claim moved the ECU to a different SA, or was a Cannot-Claim
with SA 0xFE that should have unmapped it. That update seems to be lost.
The old NAME<->SA pairing would then stay in priv->ents[] until the NAME
sends another AC, and ACs are not periodic.

The frame is also not really dropped, as the commit message suggests,
because j1939_can_recv() still passes it on:

net/can/j1939/main.c:j1939_can_recv() {
	...
	j1939_ac_recv(priv, skb);

	if (j1939_tp_recv(priv, skb))
		goto done;

	j1939_simple_recv(priv, skb);
	j1939_sk_recv(priv, skb);
	...
}

So userspace sees the NAME at the new SA while the kernel keeps it at
the old SA. Would j1939_name_to_addr() then return the stale SA? If so,
j1939_ac_fixup() would fill in the wrong SA for sockets bound by NAME,
and the wrong DA for dst_name sends.

For local ECUs, j1939_ac_fixup() unmaps the ECU when the new claim is
transmitted, but the running handler would then map it back at the old
SA.

hrtimer_try_to_cancel() also returns -1 for as long as
hrtimer_callback_running() is true, not only while the handler is
spinning on priv->lock. That includes the window after the handler's
write_unlock_bh() but before __run_hrtimer() clears base->running. On
PREEMPT_RT this window can be wider, because the softirq thread can be
preempted.

In that window the old hrtimer_cancel() would have waited briefly
without deadlocking, then processed the claim correctly. Are claims that
used to be handled correctly now lost too?

The commit message says it is impossible to drop priv->lock, and
"Therefore" the frame is dropped. Is ignoring the claim the only option
that keeps the lock held?

One alternative: on -1, carry on with the update and re-arm the timer.
Calling hrtimer_start() on a timer whose callback is running is allowed.
j1939_ecu_timer_handler() could then skip the mapping if
hrtimer_is_queued() is true, or if a per-ECU generation counter has
changed.

This code is still the same at the end of the series.

[Severity: Low]

This isn't a bug, but the comment above the call still says "cancel
pending (previous) address claim". On the new false path, the previous
claim is not cancelled, because its running handler maps the ECU at the
old SA. The new claim is the one that gets discarded. Could a comment
here say what happens to the frame in that case?

j1939_ecu_timer_cancel() says "Let the caller decide how to handle this
situation". However, j1939_ac_process(), the only caller, does not
document what it chose.

Documentation/networking/j1939.rst says that after binding with a new
SA and sending a valid address claim:

  This restarts the state machine in the kernel (and any other
  participant on the bus) for this NAME.

It also says the NAME-SA pair becomes valid if nobody contests it within
250ms. When this race hits, the state does not change and no 250ms timer
is started for the new claim. Should the documentation be updated to
match?

>  
>  	if (j1939_address_is_idle(skcb->addr.sa)) {
>  		j1939_ecu_unmap_locked(ecu);
> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index c49c96dea7960..c6278d7cb1b1f 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -151,11 +151,19 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  		      HRTIMER_MODE_REL_SOFT);
>  }
>  
> -void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
> +bool j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
>  {
> -	if (hrtimer_cancel(&ecu->ac_timer))
> +	switch (hrtimer_try_to_cancel(&ecu->ac_timer)) {
> +	case 0:	/* Not active or already finished. */
> +		return true;
> +	case 1:	/* Successfully canceled. */
>  		/* Drop the ref from j1939_ecu_timer_start(). */
>  		j1939_ecu_put(ecu);
> +		return true;
> +	default: /* Currently executing. */
> +		/* Let the caller decide how to handle this situation. */
> +		return false;
> +	}
>  }

[Severity: High]

This is a pre-existing issue and was not introduced by this patch. At
this commit, j1939_ecu_unmap_all() does not cancel any pending
ecu->ac_timer:

net/can/j1939/bus.c:j1939_ecu_unmap_all() {
	write_lock_bh(&priv->lock);
	for (i = 0; i < ARRAY_SIZE(priv->ents); i++)
		if (priv->ents[i].ecu)
			j1939_ecu_unmap_locked(priv->ents[i].ecu);
	write_unlock_bh(&priv->lock);
}

Suppose an AC frame arms ac_timer just before the last socket closes:

j1939_sk_release()
  j1939_netdev_stop()
    __j1939_rx_release()
      j1939_can_rx_unregister(priv);
      j1939_ecu_unmap_all(priv);
      j1939_priv_set(priv->ndev, NULL);

The timer can still fire up to 250ms later. Can j1939_ecu_timer_handler()
then map the ECU back into the detached priv, taking an ECU reference
that is never dropped? That would leak the ECU, the priv and the netdev
tracker reference, and unregister_netdevice could wait forever.

Can the handler also run after j1939_module_exit() has destroyed
j1939_ecu_wq and the module has been unloaded?

The hrtimer_cancel() in __j1939_ecu_release_work() can't help here,
because the armed timer itself holds an ECU reference.

The later patch in this series, "can: j1939: fix "struct j1939_ecu"
refcount leak", appears to fix this. With it, j1939_ecu_unmap_all()
walks priv->ecus and tries to cancel each timer under priv->lock. If
that fails, it drops priv->lock, holds a temporary reference, and calls
hrtimer_cancel().

j1939_ecu_timer_start() and j1939_ecu_map_locked() also switch to
j1939_ecu_verify_get(). It refuses once rx_kref is 0 or the netdev is
unregistered.

[ ... ]

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