Re: [PATCH v12 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 2 potential
issues to consider:
- [High] can: j1939: ecu ref leak and uncanceled timer in
  j1939_ecu_timer_cancel()
- [Medium] can: j1939: stale ac_want_abort on abort path in
  j1939_ecu_timer_handler()
--

commit fa21532ae1ca00bb699863f7fb0ec168d518ddc4
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() in j1939_ecu_timer_cancel() with
hrtimer_try_to_cancel() and two boolean flags on struct j1939_ecu to
avoid waiting for j1939_ecu_timer_handler() while holding priv->lock.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index c49c96dea7960..8cbf8716d286e 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -147,15 +147,25 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  		return;
>  
>  	/* Schedule timer in 250 msec to commit address change. */
> +	ecu->ac_commit_done = false;
>  	hrtimer_start(&ecu->ac_timer, ms_to_ktime(250),
>  		      HRTIMER_MODE_REL_SOFT);
>  }
>  
>  void 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. */
> +		break;
> +	case 1:	/* Successfully canceled. */
>  		/* Drop the ref from j1939_ecu_timer_start(). */
>  		j1939_ecu_put(ecu);
> +		break;
> +	default: /* Currently executing. */
> +		/* Tell j1939_ecu_timer_handler() to abort unless committed. */
> +		if (!ecu->ac_commit_done)
> +			ecu->ac_want_abort = true;
> +	}
>  }

[Severity: High]
Can j1939_ecu_timer_cancel() fail to dequeue ecu->ac_timer and leak an
ecu->kref reference when two address-claim frames arrive while a previous
j1939_ecu_timer_handler() callback is still running?

In __run_hrtimer(), base->running remains set to &ecu->ac_timer from
before j1939_ecu_timer_handler() enters write_lock_bh(&priv->lock) until
after it releases priv->lock and returns.  Whenever
hrtimer_callback_running(&ecu->ac_timer) is true, hrtimer_try_to_cancel()
returns -1 without calling remove_hrtimer(), even if ecu->ac_timer was
re-enqueued in the meantime:

CPU0                              CPU1
__run_hrtimer()
  base->running = &ecu->ac_timer
  j1939_ecu_timer_handler()
                                  j1939_ac_process() [frame 1, SA1]
                                    write_lock_bh(&priv->lock)
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                    j1939_ecu_timer_start(ecu)
                                      j1939_ecu_get(ecu)
                                      hrtimer_start(&ecu->ac_timer, ...)
                                    write_unlock_bh(&priv->lock)

                                  j1939_ac_process() [frame 2, SA2]
                                    write_lock_bh(&priv->lock)
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                        <-- stays queued, no put
                                    j1939_ecu_timer_start(ecu)
                                      j1939_ecu_get(ecu)
                                      hrtimer_start(&ecu->ac_timer, ...)
                                    write_unlock_bh(&priv->lock)

Frame 2's j1939_ecu_timer_start() overwrites the already-queued timer in
place while taking another reference, so only one future callback runs and
frame 1's ecu->kref reference is leaked (along with j1939_priv and the
netdev reference on priv->ndev).

If frame 2 instead carries J1939_IDLE_ADDR or loses address contention and
does not call j1939_ecu_timer_start(), frame 1's timer is never dequeued;
once CPU0 acquires priv->lock and clears ecu->ac_want_abort, frame 1's
timer fires 250 ms later and maps the canceled address.  Conversely, if
CPU0 had already passed write_unlock_bh(&priv->lock) when frames 1 and 2
ran, j1939_ecu_timer_start() does not clear ecu->ac_want_abort, so frame
2's newly started timer aborts when it expires.

> @@ -165,14 +175,25 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
>  	struct j1939_priv *priv = ecu->priv;
>  
>  	write_lock_bh(&priv->lock);
> +
> +	/* Abort if j1939_ecu_timer_cancel() told me to abort. */
> +	if (ecu->ac_want_abort) {
> +		ecu->ac_want_abort = false;
> +		goto out;
> +	}
> +
>  	/* TODO: can we test if ecu->addr is unicast before starting
>  	 * the timer?
>  	 */
>  	j1939_ecu_map_locked(ecu);
>  
> +	/* Commit and tell j1939_ecu_timer_cancel() not to ask me to abort. */
> +	ecu->ac_commit_done = true;
> +out:
> +	write_unlock_bh(&priv->lock);
> +
>  	/* Drop the ref from j1939_ecu_timer_start(). */
>  	j1939_ecu_put(ecu);
> -	write_unlock_bh(&priv->lock);
>  
>  	return HRTIMER_NORESTART;
>  }

[Severity: Medium]
Does jumping to out leave ecu->ac_commit_done false after an aborting
handler drops priv->lock, causing the next timer to abort as well?

CPU0                              CPU1
j1939_ecu_timer_handler()
  write_lock_bh(&priv->lock)
  if (ecu->ac_want_abort) {
    ecu->ac_want_abort = false;
    goto out;
  }
  write_unlock_bh(&priv->lock)
                                  j1939_ac_process()
                                    write_lock_bh(&priv->lock)
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                      if (!ecu->ac_commit_done)
                                        ecu->ac_want_abort = true;
                                    j1939_ecu_timer_start(ecu)
                                      ecu->ac_commit_done = false;
                                      hrtimer_start(&ecu->ac_timer, ...)
                                        <-- ac_want_abort stays true
                                    write_unlock_bh(&priv->lock)
  j1939_ecu_put(ecu)
__run_hrtimer()
  base->running = NULL

When an earlier j1939_ecu_timer_cancel() aborts the running handler (for
example on J1939_IDLE_ADDR without restarting the timer), the handler
unlocks priv->lock with ecu->ac_commit_done still false.  If a new unicast
address claim arrives before __run_hrtimer() clears base->running,
j1939_ecu_timer_cancel() sees hrtimer_try_to_cancel() == -1 and
!ecu->ac_commit_done, setting ecu->ac_want_abort = true.  Because the
old handler has already passed write_unlock_bh(&priv->lock) and
j1939_ecu_timer_start() does not clear ecu->ac_want_abort, the new 250 ms
timer will see ecu->ac_want_abort == true and abort without mapping the
ECU.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/8de287d7-b4e1-4062-b40e-b193f5ec68ca@I-love.SAKURA.ne.jp?part=2