Re: [PATCH v13 6/6] 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:
- [Medium] can: j1939: j1939_ac_process() drops new claim and leaves
  stale addr on cancel
- [Medium] can: j1939: j1939_ecu_timer_start() races with finishing
  __run_hrtimer()
--

commit d92351ad8ae3e165027e4084ec974a0caa02eabc
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 to avoid waiting for the
timer callback while holding priv->lock.

> diff --git a/net/can/j1939/address-claim.c b/net/can/j1939/address-claim.c
> index ca4ad6cdd5cbf..eb5916ac97bb4 100644
> --- a/net/can/j1939/address-claim.c
> +++ b/net/can/j1939/address-claim.c
> @@ -211,8 +211,11 @@ static void j1939_ac_process(struct j1939_priv *priv, struct sk_buff *skb)
>  	if (IS_ERR_OR_NULL(ecu))
>  		goto out_unlock_bh;
>  
> -	/* cancel pending (previous) address claim */
> -	j1939_ecu_timer_cancel(ecu);
> +	/* Cancel pending (previous) address claim. If we can't cancel,
> +	 * do nothing until pending (previous) address claim commits or aborts.
> +	 */
> +	if (!j1939_ecu_timer_cancel(ecu))
> +		goto out_ecu_put;

[Severity: Medium]
Does returning early here while setting ecu->abort_ac_handler = true drop
both the expiring address claim and the newly received replacement claim?

CPU0                              CPU1
j1939_ecu_timer_handler()
  /* waiting on priv->lock */
                                  j1939_ac_process() /* new claim for SA2 */
                                    write_lock_bh(&priv->lock)
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                      ecu->abort_ac_handler = true
                                      return false
                                    goto out_ecu_put  <-- drops SA2 claim
                                    write_unlock_bh(&priv->lock)
  write_lock_bh(&priv->lock)
  if (ecu->abort_ac_handler)
    goto out                      <-- aborts SA1 claim without mapping
  j1939_ecu_put(ecu)

Because SAE J1939 address claim messages are one-shot announcements,
neither SA1 nor SA2 is claimed.  For a remote ECU (ecu->nusers == 0),
j1939_ecu_put() in the timer handler drops the last reference and frees
the struct j1939_ecu.  For a surviving ECU (such as a local ECU with
ecu->nusers > 0), ecu remains unmapped with no running timer while
ecu->addr still holds SA1, so any later claim for SA1 hits the early check
in j1939_ac_process() and is ignored:

net/can/j1939/address-claim.c:j1939_ac_process() {
	...
	if (ecu && ecu->addr == skcb->addr.sa) {
		...
		goto out_ecu_put;
	}
	...
}

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 9b40f37546233..6520c91a6e1ec 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -156,15 +156,35 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  		return;
>  
>  	/* Schedule timer in 250 msec to commit address change. */
> +	ecu->abort_ac_handler = false;
> +	ecu->done_ac_handler = false;
>  	hrtimer_start(&ecu->ac_timer, ms_to_ktime(250),
>  		      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. */
> +		break;
> +	case 1:	/* Successfully canceled. */
>  		/* Drop the ref from j1939_ecu_timer_start(). */
>  		j1939_ecu_put(ecu);
> +		break;
> +	default: /* Currently executing. */
> +		/* OK to call j1939_ecu_timer_start() again if
> +		 * j1939_ecu_timer_handler() already committed or aborted.
> +		 */
> +		if (ecu->done_ac_handler)
> +			break;
> +		/* Tell j1939_ecu_timer_handler() to abort. */
> +		ecu->abort_ac_handler = true;
> +		/* Don't call j1939_ecu_timer_start() until
> +		 * j1939_ecu_timer_handler() commits or aborts.
> +		 */
> +		return false;
> +	}
> +	return true;
>  }
>  
>  static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
> @@ -174,14 +194,28 @@ 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->abort_ac_handler) {
> +		ecu->abort_ac_handler = false;
> +		goto out;
> +	}
> +
>  	/* TODO: can we test if ecu->addr is unicast before starting
>  	 * the timer?
>  	 */
>  	j1939_ecu_map_locked(ecu);
>  
> +out:
> +	/* Tell j1939_ecu_timer_cancel() that this handler has finished
> +	 * its critical section (either committed or aborted), so no need
> +	 * to set abort_ac_handler again while __run_hrtimer() is finishing.
> +	 */
> +	ecu->done_ac_handler = true;
> +	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]
Can allowing j1939_ecu_timer_start() while __run_hrtimer() is still
finishing cause a subsequent j1939_ecu_timer_cancel() to fail to cancel
the newly started timer and Poison it with ecu->abort_ac_handler = true?

__run_hrtimer() keeps base->running set to &ecu->ac_timer until after
j1939_ecu_timer_handler() returns and cpu_base->lock is re-acquired.  If
two address claim frames arrive after write_unlock_bh(&priv->lock) but
before __run_hrtimer() clears base->running:

CPU0                              CPU1
__run_hrtimer()
  j1939_ecu_timer_handler()
    ecu->done_ac_handler = true
    write_unlock_bh(&priv->lock)
                                  j1939_ac_process() /* frame 1 */
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                      /* done_ac_handler is true */
                                      return true
                                    j1939_ecu_timer_start(ecu)
                                      ecu->done_ac_handler = false
                                      hrtimer_start(&ecu->ac_timer, ...)
                                  j1939_ac_process() /* frame 2 */
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                      /* done_ac_handler is now false */
                                      ecu->abort_ac_handler = true
                                      return false  <-- drops frame 2
    j1939_ecu_put(ecu)
  base->running = NULL

Because hrtimer_callback_running(&ecu->ac_timer) is still true on CPU0,
hrtimer_try_to_cancel() on frame 2 returns -1 without dequeuing frame 1's
newly started timer.  With ecu->done_ac_handler cleared by frame 1,
j1939_ecu_timer_cancel() sets ecu->abort_ac_handler = true and drops frame
2, while frame 1's timer remains queued for 250 ms only to abort when it
fires.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/7cc08b0f-290e-464b-ab13-0a5ce3c57f4f@I-love.SAKURA.ne.jp?part=6