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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: j1939: j1939_ac_process() drops address claim when timer
  handler is running
--

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

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

This commit replaces hrtimer_cancel() with hrtimer_try_to_cancel() in
j1939_ecu_timer_cancel() and drops incoming address claim frames in
j1939_ac_process() when the address claim timer is currently executing.

> 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;
>  
>  	if (j1939_address_is_idle(skcb->addr.sa)) {
>  		j1939_ecu_unmap_locked(ecu);

[Severity: Medium]
Can jumping to out_ecu_put when j1939_ecu_timer_cancel() returns false
leave ecu->addr and priv->ents[] out of sync with the address claim
frame?

If a new address claim or J1939_IDLE_ADDR frame arrives for the same ECU
while j1939_ecu_timer_handler() is running on another CPU, the handler
can be spinning on write_lock_bh(&priv->lock):

CPU0                              CPU1
j1939_ac_process()
  write_lock_bh(&priv->lock)
                                  j1939_ecu_timer_handler()
                                    write_lock_bh(&priv->lock)
  j1939_ecu_timer_cancel(ecu)
    hrtimer_try_to_cancel() -> -1
  goto out_ecu_put;
  write_unlock_bh(&priv->lock)
                                    j1939_ecu_map_locked(ecu)
                                      <-- maps stale ecu->addr

Even if j1939_ecu_timer_handler() has already dropped priv->lock and is
returning, bailing out skips unmapping the old address, updating
ecu->addr, and starting the 250 ms timer for the new address.  For a
local ECU claiming a new address, j1939_ac_fixup() has already unmapped
the old address on TX, so dropping the echoed RX frame leaves the ECU
permanently unmapped and causes subsequent sendmsg() calls to fail with
-EADDRNOTAVAIL.

Since both j1939_ac_process() and j1939_ecu_timer_handler() serialize on
priv->lock, could timer cancellation be tracked under priv->lock so the
handler skips j1939_ecu_map_locked() when superseded instead of dropping
the frame?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/a1dd9760-7815-400a-a39c-a1a03cf8262e@I-love.SAKURA.ne.jp?part=2