Re: [PATCH v12 3/3] can: j1939: fix "struct j1939_ecu" refcount leak

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] can: j1939: ecu refcount leak in j1939_ecu_timer_start() when
  handler runs
- [Medium] can: j1939: stale ac_want_abort in j1939_ecu_timer_handler()
  aborts claim
- [Medium] can: j1939: race in j1939_ecu_unmap_all() when ecu kref drops
  to zero
--

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

can: j1939: fix "struct j1939_ecu" refcount leak

This commit cancels pending ecu->ac_timer instances in
j1939_ecu_unmap_all() and adds j1939_ecu_verify_get() so that
j1939_ecu_map_locked() and j1939_ecu_timer_start() do not take new ECU
references after netdev unregistration or __j1939_rx_release().

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 8cbf8716d286e..a0120567bb8f5 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
[ ... ]
> @@ -130,8 +141,43 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
>  void j1939_ecu_unmap_all(struct j1939_priv *priv)
>  {
>  	int i;
> +	struct j1939_ecu *ecu;
>  
> +rescan:
>  	write_lock_bh(&priv->lock);
> +	/* Need to shutdown timers now, for j1939_module_exit() might be called
> +	 * relatively shortly after returning from j1939_ecu_unmap_all() from
> +	 * __j1939_rx_release() from j1939_netdev_stop() from j1939_sk_release().
> +	 */
> +	list_for_each_entry(ecu, &priv->ecus, list) {
> +		if (!j1939_ecu_valid(ecu))
> +			continue;
> +		switch (hrtimer_try_to_cancel(&ecu->ac_timer)) {
> +		case 0:	/* Not active or already finished. */
> +			continue;
> +		case 1:	/* Successfully canceled. */
> +			/* Drop the ref from j1939_ecu_timer_start(). */
> +			j1939_ecu_put(ecu);
> +			continue;
> +		default: /* Currently executing. */
> +			/* Get a ref, in order to call hrtimer_cancel() without priv->lock.
> +			 * If we can't get a ref, __j1939_ecu_release_work() will call
> +			 * hrtimer_cancel() without priv->lock, and j1939_module_exit()
> +			 * will wait for completion of __j1939_ecu_release_work().
> +			 */
> +			if (!j1939_ecu_get(ecu))
> +				continue;
> +			break;
> +		}

[Severity: Medium]
Can j1939_ecu_unmap_all() return before __j1939_ecu_release() queues
ecu->release_work on j1939_ecu_wq?

In j1939_ecu_timer_handler(), j1939_ecu_put(ecu) is called after
write_unlock_bh(&priv->lock).  When that drops the final reference,
kref_put() decrements ecu->kref to 0 before __j1939_ecu_release() calls
queue_work(j1939_ecu_wq, &ecu->release_work):

CPU0                              CPU1
j1939_ecu_timer_handler()
  write_unlock_bh(&priv->lock)
  j1939_ecu_put(ecu)
    refcount_dec_and_test() -> 0
                                  j1939_ecu_unmap_all()
                                    write_lock_bh(&priv->lock)
                                    !j1939_ecu_valid(ecu) -> skips ecu
                                    write_unlock_bh(&priv->lock)
                                  j1939_module_exit()
                                    destroy_workqueue(j1939_ecu_wq)
    __j1939_ecu_release()
      queue_work(j1939_ecu_wq, ...)  <-- use-after-free

Because j1939_ecu_valid(ecu) and j1939_ecu_get(ecu) both see ecu->kref == 0
before queue_work() runs, j1939_ecu_unmap_all() neither cancels
ecu->ac_timer nor waits for the running handler, and destroy_workqueue()
in j1939_module_exit() can complete while CPU0 is still in
__j1939_ecu_release() and j1939_ecu_timer_handler().

> +		write_unlock_bh(&priv->lock);
> +		if (hrtimer_cancel(&ecu->ac_timer))
> +			/* Drop the ref from j1939_ecu_timer_start(). */
> +			j1939_ecu_put(ecu);
> +		/* Drop the ref from default: above. */
> +		j1939_ecu_put(ecu);
> +		goto rescan;
> +	}
> +	/* Unmap any remaining mapped ECUs */
>  	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);
>  }
>  
>  void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  {
>  	/* The corresponding j1939_ecu_put() is in
> -	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel().
> +	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel() or j1939_ecu_unmap_all().
>  	 */
> -	if (!j1939_ecu_get(ecu))
> +	if (!j1939_ecu_verify_get(ecu))
>  		return;
>  
>  	/* Schedule timer in 250 msec to commit address change. */

[Severity: High]
Does j1939_ecu_timer_start() leak an ecu reference if two Address Claim
frames are processed while j1939_ecu_timer_handler() is still running on
another CPU?

In __run_hrtimer(), hrtimer_callback_running(&ecu->ac_timer) remains true
until after j1939_ecu_timer_handler() returns, and hrtimer_try_to_cancel()
always returns -1 while the callback is running even if the timer has
already been re-queued:

net/can/j1939/bus.c:j1939_ecu_timer_cancel() {
	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;
	}
}

If two Address Claim frames with changed unicast source addresses arrive on
CPU1 while j1939_ecu_timer_handler() is running on CPU0:

CPU0                              CPU1
j1939_ecu_timer_handler()
                                  j1939_ac_process() [frame 1]
                                    j1939_ecu_timer_cancel(ecu) -> -1
                                      ecu->ac_want_abort = true
                                    j1939_ecu_timer_start(ecu)
                                      j1939_ecu_verify_get(ecu)
                                      hrtimer_start(&ecu->ac_timer, ...)
                                  j1939_ac_process() [frame 2]
                                    j1939_ecu_timer_cancel(ecu) -> -1
                                      (does not put ref from frame 1)
                                    j1939_ecu_timer_start(ecu)
                                      j1939_ecu_verify_get(ecu)
                                      hrtimer_start(&ecu->ac_timer, ...)
                                        <-- overwrites queued timer,
                                            leaking frame 1's ecu ref

The leaked ecu->kref also pins priv->kref and ecu->priv_dev_tracker, which
prevents unregister_netdevice() from completing.  In addition, if frame 2
cancels the claim without restarting the timer (for example, with
J1939_IDLE_ADDR) while CPU0 is waiting on write_lock_bh(&priv->lock), CPU0
clears ecu->ac_want_abort = false and exits while the timer queued by
frame 1 stays armed and maps the canceled ECU 250 ms later.

[Severity: Medium]
Can a new address claim timer started by j1939_ecu_timer_start() leave a
stale ecu->ac_want_abort set and abort when it fires?

When j1939_ecu_timer_handler() aborts, it leaves ecu->ac_commit_done as
false and unlocks priv->lock before returning:

net/can/j1939/bus.c:j1939_ecu_timer_handler() {
...
	if (ecu->ac_want_abort) {
		ecu->ac_want_abort = false;
		goto out;
	}
...
	ecu->ac_commit_done = true;
out:
	write_unlock_bh(&priv->lock);

	/* Drop the ref from j1939_ecu_timer_start(). */
	j1939_ecu_put(ecu);
}

If j1939_ac_process() runs on another CPU after write_unlock_bh(&priv->lock)
but before __run_hrtimer() clears base->running:

CPU0                              CPU1
j1939_ecu_timer_handler()
  if (ecu->ac_want_abort)
    ecu->ac_want_abort = false
    goto out
  write_unlock_bh(&priv->lock)
                                  j1939_ac_process()
                                    j1939_ecu_timer_cancel(ecu) -> -1
                                      (!ecu->ac_commit_done is true)
                                      ecu->ac_want_abort = true
                                    j1939_ecu_timer_start(ecu)
                                      ecu->ac_commit_done = false
                                      hrtimer_start(&ecu->ac_timer, ...)
  j1939_ecu_put(ecu)
                                  (250 ms later)
                                  j1939_ecu_timer_handler()
                                    if (ecu->ac_want_abort)
                                      goto out  <-- aborts new claim

Because j1939_ecu_timer_start() does not clear ecu->ac_want_abort and CPU0
has already passed the check, the newly started timer aborts without
mapping the ECU.

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