Re: [PATCH v6] can: j1939: fix j1939_ecu refcount leak and related race conditions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] can: j1939: double j1939_ecu_put() when j1939_ecu_timer_cancel() cancels tasklet
- [High] can: j1939: deadlock between priv->lock and softirq_expiry_lock on PREEMPT_RT
--

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

can: j1939: fix j1939_ecu refcount leak and related race conditions

This commit fixes a struct j1939_ecu reference count leak and related race
conditions by deferring ECU release to a workqueue, handing off address claim
timer completion to a tasklet, and validating netdev registration and RX
reference counts before acquiring ECU references.

Link: https://lkml.kernel.org/r/179202402875.794844.18141702206663586949.b4-review@b4

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..632e9aed6cf52 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c

[ ... ]

> @@ -99,8 +138,17 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
>  void j1939_ecu_unmap_all(struct j1939_priv *priv)
>  {
>  	int i;
> +	struct j1939_ecu *ecu;
>  
>  	write_lock_bh(&priv->lock);
> +	/* Although j1939_ecu_get() from j1939_ecu_map_locked() from
> +	 * j1939_ecu_tasklet_handler() will fail if netdev is no longer
> +	 * registered or __j1939_rx_release() was called, cancel the 250ms
> +	 * timer in order to speed up teardown operation.
> +	 */
> +	list_for_each_entry(ecu, &priv->ecus, list)
> +		j1939_ecu_timer_cancel(ecu);
> +	/* 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);
> @@ -109,10 +157,11 @@ void j1939_ecu_unmap_all(struct j1939_priv *priv)
>  
>  void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  {
> -	/* The ECU is held here and released in the
> +	/* The corresponding j1939_ecu_put() is in
>  	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel().
>  	 */
> -	j1939_ecu_get(ecu);
> +	if (!j1939_ecu_get(ecu))
> +		return;
>  
>  	/* Schedule timer in 250 msec to commit address change. */
>  	hrtimer_start(&ecu->ac_timer, ms_to_ktime(250),
> @@ -121,29 +170,99 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  
>  void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
>  {
> +	/* Drop the ref from j1939_ecu_timer_start() if j1939_ecu_timer_handler() was not called.
> +	 * Don't drop the ref from j1939_ecu_timer_start() if j1939_ecu_timer_handler() was already
> +	 * called.
> +	 */
>  	if (hrtimer_cancel(&ecu->ac_timer))
>  		j1939_ecu_put(ecu);

[Severity: High]
As reported by Sashiko on v5 [1], can calling hrtimer_cancel(&ecu->ac_timer)
in j1939_ecu_timer_cancel() while holding write_lock_bh(&priv->lock) in
j1939_ecu_unmap_all() and j1939_ac_process() still cause an AB-BA deadlock on
PREEMPT_RT kernels?

Because ecu->ac_timer is set up with HRTIMER_MODE_REL_SOFT, if ecu->ac_timer
is currently running its callback when hrtimer_cancel() is called,
hrtimer_try_to_cancel() returns -1 and hrtimer_cancel_wait_running() acquires
spin_lock_bh(&base->cpu_base->softirq_expiry_lock) while priv->lock is held:

  j1939_ecu_unmap_all() / j1939_ac_process()
    write_lock_bh(&priv->lock)
    j1939_ecu_timer_cancel()
      hrtimer_cancel(&ecu->ac_timer)
        hrtimer_cancel_wait_running()
          spin_lock_bh(&base->cpu_base->softirq_expiry_lock)

However, softirq_expiry_lock is a per-CPU lock held by hrtimer_run_softirq()
across all HRTIMER_MODE_REL_SOFT timer callbacks on that CPU. In
net/can/j1939/transport.c, session->txtimer (j1939_tp_txtimer) and
session->rxtimer (j1939_tp_rxtimer) also use HRTIMER_MODE_REL_SOFT and acquire
priv->lock under softirq_expiry_lock:

  hrtimer_run_softirq()
    spin_lock_bh(&cpu_base->softirq_expiry_lock)
    __hrtimer_run_queues()
      j1939_tp_txtimer() / j1939_tp_rxtimer()
        j1939_send_one()
          j1939_ac_fixup()
            j1939_ecu_get_by_name() -> read_lock_bh(&priv->lock)
            j1939_ecu_unmap() -> write_lock_bh(&ecu->priv->lock)

If one CPU holds write_lock_bh(&priv->lock) and waits in
hrtimer_cancel_wait_running() for ecu->ac_timer on another CPU, and that
other CPU finishes j1939_ecu_timer_handler() and advances in
__hrtimer_run_queues() to an expired session->txtimer or session->rxtimer,
won't the two CPUs deadlock on softirq_expiry_lock and priv->lock?

> +	/* Drop the ref from j1939_ecu_timer_handler() if test_and_set_bit() in
> +	 * j1939_ecu_timer_handler() succeeded and j1939_ecu_tasklet_handler() is not yet called.
> +	 * Don't drop the ref from j1939_ecu_timer_handler() if test_and_set_bit() in
> +	 * j1939_ecu_timer_handler() failed or j1939_ecu_tasklet_handler() was already called.
> +	 */
> +	if (test_and_clear_bit(0, &ecu->flags))
> +		j1939_ecu_put(ecu);
>  }
>  
>  static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
>  {
>  	struct j1939_ecu *ecu =
>  		container_of(hrtimer, struct j1939_ecu, ac_timer);
> +
> +	/* The corresponding j1939_ecu_put() is in
> +	 * j1939_ecu_tasklet_handler() or j1939_ecu_timer_cancel().
> +	 */
> +	if (!j1939_ecu_get(ecu))
> +		goto out;

[ ... ]

> +	if (likely(!test_and_set_bit(0, &ecu->flags))) {
> +		tasklet_schedule(&ecu->ac_tasklet);
> +	} else {
> +		netdev_warn(ecu->priv->ndev,
> +			    "Skipped mapping ECU, addr: 0x%02x, name: 0x%016llx.\n",
> +			    ecu->addr, ecu->name);
> +		j1939_ecu_put(ecu);
> +	}
> +out:
> +	/* Drop the ref from j1939_ecu_timer_start(). */
> +	j1939_ecu_put(ecu);
> +	return HRTIMER_NORESTART;
> +}
> +
> +static void j1939_ecu_tasklet_handler(struct tasklet_struct *tasklet)
> +{
> +	struct j1939_ecu *ecu = container_of(tasklet, struct j1939_ecu, ac_tasklet);
>  	struct j1939_priv *priv = ecu->priv;
>  
>  	write_lock_bh(&priv->lock);
> +	/* Check if j1939_ecu_timer_cancel() was called. */
> +	if (!test_and_clear_bit(0, &ecu->flags))
> +		goto out;
>  	/* TODO: can we test if ecu->addr is unicast before starting
>  	 * the timer?
>  	 */
>  	j1939_ecu_map_locked(ecu);
> -
> -	/* The corresponding j1939_ecu_get() is in
> -	 * j1939_ecu_timer_start().
> -	 */
> -	j1939_ecu_put(ecu);
> + out:
>  	write_unlock_bh(&priv->lock);
> -
> -	return HRTIMER_NORESTART;
> +	/* Drop the ref from j1939_ecu_timer_handler(). */
> +	j1939_ecu_put(ecu);
>  }

[Severity: Critical]
As reported by Sashiko on v5 [2], doesn't this drop the reference from
j1939_ecu_timer_handler() twice when j1939_ecu_timer_cancel() races with the
scheduled tasklet?

When ecu->ac_timer fires, j1939_ecu_timer_handler() takes one new reference on
ecu for the tasklet handoff, sets bit 0 of ecu->flags, schedules
ecu->ac_tasklet, and drops the initial timer reference from
j1939_ecu_timer_start().

If j1939_ecu_timer_cancel(ecu) is then called under write_lock_bh(&priv->lock)
(from j1939_ac_process() or j1939_ecu_unmap_all()) before
j1939_ecu_tasklet_handler() acquires write_lock_bh(&priv->lock):

  j1939_ecu_timer_cancel()
    if (test_and_clear_bit(0, &ecu->flags))
      j1939_ecu_put(ecu);

j1939_ecu_timer_cancel() clears bit 0 and drops the tasklet's reference, while
ecu->ac_tasklet remains scheduled.

When j1939_ecu_tasklet_handler() subsequently runs, test_and_clear_bit(0,
&ecu->flags) returns false and jumps to out, which unlocks priv->lock and
still calls j1939_ecu_put(ecu) a second time for the same reference. Won't
this underflow ecu->kref and prematurely free struct j1939_ecu?

Furthermore, if j1939_ac_process() calls j1939_ecu_timer_cancel(ecu) followed
by j1939_ecu_timer_start(ecu), clearing bit 0 in j1939_ecu_timer_cancel()
while ecu->ac_tasklet is still queued allows a subsequent
j1939_ecu_timer_handler() to see bit 0 clear if the tasklet was delayed.

[1] https://lore.kernel.org/r/d0590019-f0c9-4288-9653-a7b6056012af@I-love.SAKURA.ne.jp
[2] https://lore.kernel.org/r/d0590019-f0c9-4288-9653-a7b6056012af@I-love.SAKURA.ne.jp

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/c9de355a-d66e-4f28-9636-a6fef8fe8b84@I-love.SAKURA.ne.jp?part=1