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

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

[Tetsuo Handa]

This is a squashed patch that fixes three bugs.

Step 1: Fix possibility of list corruption in __j1939_ecu_release().

  Since not all callers are calling j1939_ecu_put() with priv->lock held
  for write, list corruption might happen if the last user called
  j1939_ecu_put() without priv->lock held for write.

  Fix this problem by deferring release steps to RCU context where we can
  unconditionally hold priv->lock for write.

  By deferring to RCU context, readers might find ECUs which have already
  reached __j1939_ecu_release(). Therefore, allow using an ECU only when
  __j1939_ecu_release() is not yet called (indicated by ecu->kref > 0).

Step 2: Fix possibility of deadlock in j1939_ecu_timer_cancel().

  Since j1939_ecu_timer_handler() takes priv->lock whereas hrtimer_cancel()
  from j1939_ecu_timer_cancel() with priv->lock already held synchronously
  waits for j1939_ecu_timer_handler() to complete, an AB-BA deadlock can
  happen.

  Fix this problem by using two-stage hrtimer (ac_wait_timer waits for
  250ms timeout, and hands over to ac_map_timer for committing changes) and
  ecu->flags for tracking the state of handing over.

  The fix for step 1 makes it possible to call j1939_ecu_put() regardless
  of whether priv->lock is held for write, and also makes it possible to
  wait for ac_wait_timer/ac_map_timer to complete using hrtimer_cancel()
  before kfree(ecu).

Step 3: Fix "struct j1939_ecu" refcount leak.

  Since

    j1939_ecu_get(ecu);
    priv->ents[ecu->addr] = ecu;

  in j1939_ecu_map_locked() from j1939_ecu_timer_handler() can succeed
  even after

    priv->ents[ecu->addr] = NULL;
    j1939_ecu_put(ecu);

  in j1939_ecu_unmap_locked() from j1939_ecu_unmap_all() from
  __j1939_rx_release() from j1939_netdev_stop() has completed,
  syzbot is reporting "struct j1939_ecu" refcount leak.

    unregister_netdevice: waiting for vcan0 to become free. Usage count = 3
    ref_tracker: netdev@ffff888038720720 has 1/2 users at
         __netdev_tracker_alloc include/linux/netdevice.h:4554 [inline]
         netdev_hold include/linux/netdevice.h:4583 [inline]
         j1939_ecu_create_locked+0x1fc/0x4d0 net/can/j1939/bus.c:159
         j1939_local_ecu_get+0x1b8/0x250 net/can/j1939/bus.c:293
         j1939_sk_bind+0x89b/0xdb0 net/can/j1939/socket.c:529
         __sys_bind_socket net/socket.c:1947 [inline]
         __sys_bind_socket net/socket.c:1939 [inline]
         __sys_bind+0x1a9/0x260 net/socket.c:1978
         __do_sys_bind net/socket.c:1983 [inline]
         __se_sys_bind net/socket.c:1981 [inline]
         __x64_sys_bind+0x72/0xb0 net/socket.c:1981
         do_syscall_x64 arch/x86/entry/syscall_64.c:61 [inline]
         do_syscall_64+0x123/0x790 arch/x86/entry/syscall_64.c:84
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

    ref_tracker: netdev@ffff888038720720 has 1/2 users at
         __netdev_tracker_alloc include/linux/netdevice.h:4554 [inline]
         netdev_hold include/linux/netdevice.h:4583 [inline]
         j1939_priv_create net/can/j1939/main.c:140 [inline]
         j1939_netdev_start+0x2b8/0x990 net/can/j1939/main.c:268
         j1939_sk_bind+0x479/0xdb0 net/can/j1939/socket.c:506
         __sys_bind_socket net/socket.c:1947 [inline]
         __sys_bind_socket net/socket.c:1939 [inline]
         __sys_bind+0x1a9/0x260 net/socket.c:1978
         __do_sys_bind net/socket.c:1983 [inline]
         __se_sys_bind net/socket.c:1981 [inline]
         __x64_sys_bind+0x72/0xb0 net/socket.c:1981
         do_syscall_x64 arch/x86/entry/syscall_64.c:61 [inline]
         do_syscall_64+0x123/0x790 arch/x86/entry/syscall_64.c:84
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

  Fix this problem by canceling pending address claim timers from
  j1939_ecu_unmap_all() and making j1939_ecu_get() fail unless three
  conditions listed below are met.

    (a) A netdev which is associated with j1939_priv is still registered
        (indicated by priv->ndev->reg_state == NETREG_REGISTERED).

    (b) __j1939_rx_release() is not yet called (indicated by
        priv->rx_kref > 0), for j1939_ecu_unmap_all() won't be called
        again after __j1939_rx_release() from j1939_netdev_stop() from
        j1939_netdev_notify(NETDEV_UNREGISTER) is once triggered.

    (c) __j1939_ecu_release() is not yet called (indicated by
        ecu->kref > 0)

  The fix for step 1 makes it possible to call j1939_ecu_put() regardless
  of whether priv->lock is held for write.

Reported-by: syzbot+e2af46126e0644cbebdd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e2af46126e0644cbebdd
Link: https://lkml.kernel.org/r/179202402875.794844.18141702206663586949.b4-review@b4
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Previous version is "[PATCH v4] can: j1939: fix j1939_ecu refcount leak"
at https://lkml.kernel.org/r/4b2c8d95-d17e-4f36-a340-abf84693d24a@I-love.SAKURA.ne.jp .

 net/can/j1939/address-claim.c |   8 +-
 net/can/j1939/bus.c           | 189 ++++++++++++++++++++++++++++------
 net/can/j1939/j1939-priv.h    |  11 +-
 net/can/j1939/main.c          |   5 +
 4 files changed, 174 insertions(+), 39 deletions(-)

diff --git a/net/can/j1939/address-claim.c b/net/can/j1939/address-claim.c
index ca4ad6cdd5cb..821af11e9be4 100644
--- a/net/can/j1939/address-claim.c
+++ b/net/can/j1939/address-claim.c
@@ -160,8 +160,8 @@ static void j1939_ac_process(struct j1939_priv *priv, struct sk_buff *skb)
 	 * but only if the ECU was mapped before. So "ecu" still
 	 * belongs to us.
 	 *
-	 * j1939_ecu_timer_start() will increment the ref counter
-	 * before it starts the timer, so we can put the ecu when
+	 * j1939_ecu_wait_timer_start() will increment the ref counter
+	 * before it starts the wait timer, so we can put the ecu when
 	 * leaving this function.
 	 */
 	ecu = j1939_ecu_get_by_name_locked(priv, name);
@@ -212,7 +212,7 @@ 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);
+	j1939_ecu_wait_timer_cancel(ecu);
 
 	if (j1939_address_is_idle(skcb->addr.sa)) {
 		j1939_ecu_unmap_locked(ecu);
@@ -237,7 +237,7 @@ static void j1939_ac_process(struct j1939_priv *priv, struct sk_buff *skb)
 		}
 	}
 
-	j1939_ecu_timer_start(ecu);
+	j1939_ecu_wait_timer_start(ecu);
  out_ecu_put:
 	j1939_ecu_put(ecu);
  out_unlock_bh:
diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..7980c87e004f 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -14,25 +14,64 @@
 
 #include "j1939-priv.h"
 
-static void __j1939_ecu_release(struct kref *kref)
+static void __j1939_ecu_release_rcu(struct rcu_head *rcu)
 {
-	struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
+	struct j1939_ecu *ecu = container_of(rcu, struct j1939_ecu, rcu);
 	struct j1939_priv *priv = ecu->priv;
 
+	write_lock_bh(&priv->lock);
 	list_del(&ecu->list);
+	write_unlock_bh(&priv->lock);
+	/* There should be no ref on this ECU, but we need to make sure that neither
+	 * j1939_ecu_wait_timer_handler() or j1939_ecu_map_timer_handler() is running
+	 * before releasing this ECU.
+	 *
+	 * We can use hrtimer_cancel(&ecu->ac_wait_timer), for
+	 * j1939_ecu_wait_timer_handler() does not hold priv->lock.
+	 *
+	 * We can use hrtimer_cancel(&ecu->ac_map_timer), for
+	 * we are not holding priv->lock here.
+	 */
+	WARN_ON(hrtimer_cancel(&ecu->ac_wait_timer));
+	WARN_ON(hrtimer_cancel(&ecu->ac_map_timer));
 	netdev_put(priv->ndev, &ecu->priv_dev_tracker);
 	kfree(ecu);
 	j1939_priv_put(priv);
 }
 
+static void __j1939_ecu_release(struct kref *kref)
+{
+	struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
+
+	/* Since not all callers are calling j1939_ecu_put() with priv->lock
+	 * held for write, defer actual release to RCU context. By deferring to
+	 * RCU context, priv->ecus readers might find ECUs with ecu->kref == 0.
+	 * Therefore, priv->ecus readers must validate with j1939_ecu_valid() or
+	 * j1939_ecu_get().
+	 */
+	call_rcu(&ecu->rcu, __j1939_ecu_release_rcu);
+}
+
 void j1939_ecu_put(struct j1939_ecu *ecu)
 {
 	kref_put(&ecu->kref, __j1939_ecu_release);
 }
 
-static void j1939_ecu_get(struct j1939_ecu *ecu)
+static __must_check bool j1939_ecu_get(struct j1939_ecu *ecu)
+{
+	struct j1939_priv *priv = ecu->priv;
+
+	/* Don't get an ECU ref if netdev is no longer registered or
+	 * __j1939_rx_release() was called or __j1939_ecu_release() was called.
+	 */
+	return READ_ONCE(priv->ndev->reg_state) == NETREG_REGISTERED &&
+		kref_read(&priv->rx_kref) && kref_get_unless_zero(&ecu->kref);
+}
+
+static __must_check bool j1939_ecu_valid(struct j1939_ecu *ecu)
 {
-	kref_get(&ecu->kref);
+	/* Treat an ECU as invalid if __j1939_ecu_release() was called. */
+	return kref_read(&ecu->kref);
 }
 
 static bool j1939_ecu_is_mapped_locked(struct j1939_ecu *ecu)
@@ -56,15 +95,17 @@ static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
 	if (!j1939_address_is_unicast(ecu->addr))
 		return;
 
+	if (!j1939_ecu_get(ecu))
+		return;
+
 	ent = &priv->ents[ecu->addr];
 
 	if (ent->ecu) {
 		netdev_warn(priv->ndev, "Trying to map already mapped ECU, addr: 0x%02x, name: 0x%016llx. Skip it.\n",
 			    ecu->addr, ecu->name);
+		j1939_ecu_put(ecu);
 		return;
 	}
-
-	j1939_ecu_get(ecu);
 	ent->ecu = ecu;
 	ent->nusers += ecu->nusers;
 }
@@ -101,48 +142,122 @@ void j1939_ecu_unmap_all(struct j1939_priv *priv)
 	int i;
 
 	write_lock_bh(&priv->lock);
+	if (READ_ONCE(priv->ndev->reg_state) != NETREG_REGISTERED) {
+		struct j1939_ecu *ecu;
+
+		/* Although j1939_ecu_get() from j1939_ecu_map_locked() from
+		 * j1939_ecu_map_timer_handler_locked() will fail if netdev is
+		 * no longer registered, try to bail out early by dropping
+		 * the ref from j1939_ecu_wait_timer_start() now in order to
+		 * speed up teardown operation.
+		 */
+		list_for_each_entry(ecu, &priv->ecus, list)
+			if (j1939_ecu_valid(ecu))
+				j1939_ecu_wait_timer_cancel(ecu);
+	}
 	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)
+void j1939_ecu_wait_timer_start(struct j1939_ecu *ecu)
 {
-	/* The ECU is held here and released in the
-	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel().
+	lockdep_assert_held(&ecu->priv->lock);
+
+	/* The corresponding j1939_ecu_put() is in j1939_ecu_map_timer_handler_locked()
+	 * or j1939_ecu_wait_timer_cancel() or j1939_ecu_wait_timer_handler().
 	 */
-	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),
+	hrtimer_start(&ecu->ac_wait_timer, ms_to_ktime(250),
 		      HRTIMER_MODE_REL_SOFT);
 }
 
-void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
+static void j1939_ecu_map_timer_handler_locked(struct j1939_ecu *ecu)
 {
-	if (hrtimer_cancel(&ecu->ac_timer))
+	if (!test_and_clear_bit(0, &ecu->flags))
+		return;
+	/* 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_wait_timer_start(). */
+	j1939_ecu_put(ecu);
+}
+
+void j1939_ecu_wait_timer_cancel(struct j1939_ecu *ecu)
+{
+	if (hrtimer_cancel(&ecu->ac_wait_timer))
+		/* The corresponding j1939_ecu_get() is in j1939_ecu_wait_timer_start(). */
 		j1939_ecu_put(ecu);
+	j1939_ecu_map_timer_handler_locked(ecu);
 }
 
-static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
+static enum hrtimer_restart j1939_ecu_map_timer_handler(struct hrtimer *hrtimer)
 {
 	struct j1939_ecu *ecu =
-		container_of(hrtimer, struct j1939_ecu, ac_timer);
+		container_of(hrtimer, struct j1939_ecu, ac_map_timer);
 	struct j1939_priv *priv = ecu->priv;
 
 	write_lock_bh(&priv->lock);
-	/* 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);
+	j1939_ecu_map_timer_handler_locked(ecu);
 	write_unlock_bh(&priv->lock);
+	return HRTIMER_NORESTART;
+}
 
+static enum hrtimer_restart j1939_ecu_wait_timer_handler(struct hrtimer *hrtimer)
+{
+	struct j1939_ecu *ecu =
+		container_of(hrtimer, struct j1939_ecu, ac_wait_timer);
+
+	/*
+	 * Since we can't commit this address change from this context because we need to hold
+	 * priv->lock for write before committing while hrtimer_cancel() (which waits for this
+	 * context to complete) is called with priv->lock held for write, schedule
+	 * j1939_ecu_map_timer_handler_locked() (which is called from a different context with
+	 * priv->lock held for write) to commit this address change.
+	 *
+	 * The !test_and_set_bit() in j1939_ecu_wait_timer_handler() and
+	 * the test_and_clear_bit() in j1939_ecu_map_timer_handler_locked() pair guarantees that
+	 * the ref from j1939_ecu_wait_timer_start() is dropped for exactly once.
+	 *
+	 * The !test_and_set_bit() condition here is likely true due to reasons listed below.
+	 *
+	 *   (1) j1939_ecu_wait_timer_cancel(ecu) and j1939_ecu_wait_timer_start(ecu) are
+	 *       serialized by ecu->priv->lock
+	 *   (2) scheduling of ecu->ac_map_timer does not happen if
+	 *       hrtimer_cancel(&ecu->ac_wait_timer) from j1939_ecu_wait_timer_cancel(ecu) was
+	 *       called before j1939_ecu_wait_timer_handler(&ecu->ac_wait_timer) is fired
+	 *   (3) scheduling of ecu->ac_map_timer completes before
+	 *       hrtimer_cancel(&ecu->ac_wait_timer) from j1939_ecu_wait_timer_cancel(ecu) returns
+	 *       if j1939_ecu_wait_timer_handler(&ecu->ac_wait_timer) was fired before
+	 *       hrtimer_cancel(&ecu->ac_wait_timer) from j1939_ecu_wait_timer_cancel(ecu) is
+	 *       called
+	 *   (4) ecu->ac_map_timer has shorter expiry time than ecu->ac_wait_timer, which should
+	 *       fire j1939_ecu_map_timer_handler(&ecu->ac_map_timer) before
+	 *       j1939_ecu_wait_timer_handler(&ecu->ac_wait_timer) is fired as long as
+	 *       ecu->ac_map_timer is scheduled by previous j1939_ecu_wait_timer_handler()
+	 *       before ecu->ac_wait_timer is scheduled by current j1939_ecu_wait_timer_start()
+	 *
+	 * However, if something significantly delayed firing of j1939_ecu_map_timer_handler()
+	 * enough to fire j1939_ecu_wait_timer_handler() before j1939_ecu_map_timer_handler() is
+	 * fired, blindly scheduling ecu->ac_map_timer will fail to drop the ref from previous
+	 * j1939_ecu_wait_timer_start(). Therefore, drop the ref from current
+	 * j1939_ecu_wait_timer_start() if such race condition happened.
+	 */
+	if (likely(!test_and_set_bit(0, &ecu->flags))) {
+		hrtimer_start(&ecu->ac_map_timer, ms_to_ktime(0), HRTIMER_MODE_REL_SOFT);
+	} else {
+		netdev_warn(ecu->priv->ndev,
+			    "Skipped mapping ECU, addr: 0x%02x, name: 0x%016llx.\n",
+			    ecu->addr, ecu->name);
+		/* The corresponding j1939_ecu_get() is in j1939_ecu_wait_timer_start(). */
+		j1939_ecu_put(ecu);
+	}
 	return HRTIMER_NORESTART;
 }
 
@@ -152,6 +267,13 @@ struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name)
 
 	lockdep_assert_held(&priv->lock);
 
+	/* Don't create a new ECU if netdev is no longer registered or
+	 * __j1939_rx_release() was called.
+	 */
+	if (READ_ONCE(priv->ndev->reg_state) != NETREG_REGISTERED ||
+	    !kref_read(&priv->rx_kref))
+		return ERR_PTR(-ENODEV);
+
 	ecu = kzalloc_obj(*ecu, gfp_any());
 	if (!ecu)
 		return ERR_PTR(-ENOMEM);
@@ -160,7 +282,9 @@ struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name)
 	ecu->addr = J1939_IDLE_ADDR;
 	ecu->name = name;
 
-	hrtimer_setup(&ecu->ac_timer, j1939_ecu_timer_handler, CLOCK_MONOTONIC,
+	hrtimer_setup(&ecu->ac_wait_timer, j1939_ecu_wait_timer_handler, CLOCK_MONOTONIC,
+		      HRTIMER_MODE_REL_SOFT);
+	hrtimer_setup(&ecu->ac_map_timer, j1939_ecu_map_timer_handler, CLOCK_MONOTONIC,
 		      HRTIMER_MODE_REL_SOFT);
 	INIT_LIST_HEAD(&ecu->list);
 
@@ -189,8 +313,8 @@ struct j1939_ecu *j1939_ecu_get_by_addr_locked(struct j1939_priv *priv, u8 addr)
 		return NULL;
 
 	ecu = j1939_ecu_find_by_addr_locked(priv, addr);
-	if (ecu)
-		j1939_ecu_get(ecu);
+	if (ecu && !j1939_ecu_get(ecu))
+		ecu = NULL;
 
 	return ecu;
 }
@@ -215,7 +339,7 @@ static struct j1939_ecu *j1939_ecu_find_by_name_locked(struct j1939_priv *priv,
 	lockdep_assert_held(&priv->lock);
 
 	list_for_each_entry(ecu, &priv->ecus, list) {
-		if (ecu->name == name)
+		if (ecu->name == name && j1939_ecu_valid(ecu))
 			return ecu;
 	}
 
@@ -232,11 +356,12 @@ struct j1939_ecu *j1939_ecu_get_by_name_locked(struct j1939_priv *priv,
 	if (!name)
 		return NULL;
 
-	ecu = j1939_ecu_find_by_name_locked(priv, name);
-	if (ecu)
-		j1939_ecu_get(ecu);
+	list_for_each_entry(ecu, &priv->ecus, list) {
+		if (ecu->name == name && j1939_ecu_get(ecu))
+			return ecu;
+	}
 
-	return ecu;
+	return NULL;
 }
 
 struct j1939_ecu *j1939_ecu_get_by_name(struct j1939_priv *priv, name_t name)
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..48769eab6c3d 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -35,13 +35,18 @@ struct j1939_ecu {
 	u8 addr;
 
 	/* indicates that this ecu successfully claimed @sa as its address */
-	struct hrtimer ac_timer;
+	struct hrtimer ac_wait_timer;
+	struct hrtimer ac_map_timer;
 	struct kref kref;
 	struct j1939_priv *priv;
 	netdevice_tracker priv_dev_tracker;
 
 	/* count users, to help transport protocol decide for interaction */
 	int nusers;
+
+	struct rcu_head rcu;
+	/* Bit 0: Ready to commit address change. */
+	unsigned long flags;
 };
 
 struct j1939_priv {
@@ -202,8 +207,8 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb);
 /* network management */
 struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name);
 
-void j1939_ecu_timer_start(struct j1939_ecu *ecu);
-void j1939_ecu_timer_cancel(struct j1939_ecu *ecu);
+void j1939_ecu_wait_timer_start(struct j1939_ecu *ecu);
+void j1939_ecu_wait_timer_cancel(struct j1939_ecu *ecu);
 void j1939_ecu_unmap_all(struct j1939_priv *priv);
 
 struct j1939_priv *j1939_netdev_start(struct net_device *ndev);
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 5e5e6c228f22..bdf536440ade 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -424,6 +424,11 @@ static __exit void j1939_module_exit(void)
 	can_proto_unregister(&j1939_can_proto);
 
 	unregister_netdevice_notifier(&j1939_netdev_notifier);
+
+	/* We need to make sure that __j1939_ecu_release_rcu() is
+	 * no longer running before unmapping memory for this module.
+	 */
+	rcu_barrier();
 }
 
 module_init(j1939_module_init);
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] can: j1939: uncanceled ac_map_timer and bogus map in j1939_ecu_wait_timer_cancel()
- [High] can: j1939: uncancelled ac_wait_timer in j1939_ecu_unmap_all() when netdev is registered
- [High] can: j1939: PREEMPT_RT lock inversion between priv->lock and softirq_expiry_lock
--

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

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

This patch defers j1939_ecu release to RCU context, splits the address claim
hrtimer into a two-stage timer (ac_wait_timer and ac_map_timer), and gates
ECU reference acquisition on netdevice registration and rx_kref state.

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..7980c87e004f1 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c

[ ... ]

> @@ -101,48 +142,122 @@ void j1939_ecu_unmap_all(struct j1939_priv *priv)
>  	int i;
>  
>  	write_lock_bh(&priv->lock);
> +	if (READ_ONCE(priv->ndev->reg_state) != NETREG_REGISTERED) {
> +		struct j1939_ecu *ecu;
> +
> +		/* Although j1939_ecu_get() from j1939_ecu_map_locked() from
> +		 * j1939_ecu_map_timer_handler_locked() will fail if netdev is
> +		 * no longer registered, try to bail out early by dropping
> +		 * the ref from j1939_ecu_wait_timer_start() now in order to
> +		 * speed up teardown operation.
> +		 */
> +		list_for_each_entry(ecu, &priv->ecus, list)
> +			if (j1939_ecu_valid(ecu))
> +				j1939_ecu_wait_timer_cancel(ecu);
> +	}

[Severity: High]
What happens when j1939_ecu_unmap_all() is called while priv->ndev->reg_state
is still NETREG_REGISTERED?

j1939_ecu_unmap_all() is called from two paths: __j1939_rx_release() (via
j1939_netdev_stop() when the last bound J1939 socket is closed or unbound)
and j1939_netdev_notify() on NETDEV_DOWN:

net/can/j1939/main.c:__j1939_rx_release() {
	...
	j1939_can_rx_unregister(priv);
	j1939_ecu_unmap_all(priv);
	j1939_priv_set(priv->ndev, NULL);
	mutex_unlock(&j1939_netdev_lock);
}

When the last bound socket is closed while the netdevice is still registered,
reg_state == NETREG_REGISTERED is true, so j1939_ecu_unmap_all() skips
canceling pending 250ms ac_wait_timer instances on priv->ecus, and
__j1939_rx_release() then detaches priv from priv->ndev via
j1939_priv_set(priv->ndev, NULL).

Any unmapped ECU with an armed ac_wait_timer continues holding its ecu->kref
reference from j1939_ecu_wait_timer_start(), so __j1939_ecu_release() is not
called and call_rcu(&ecu->rcu, __j1939_ecu_release_rcu) is not queued yet.
Because can_ml->j1939_priv is now NULL, a subsequent NETDEV_UNREGISTER
notification sees j1939_priv_get_by_ndev(ndev) == NULL and cannot cancel the
timer either.

Furthermore, if the can-j1939 module is unloaded via j1939_module_exit()
during that 250ms window after the socket closes, rcu_barrier() returns
immediately because call_rcu() has not been invoked for ecu->rcu. When
ac_wait_timer fires up to 250ms later, wouldn't the hrtimer subsystem invoke
j1939_ecu_wait_timer_handler() in unmapped module memory?

Similarly, when j1939_netdev_notify() handles NETDEV_DOWN while sockets
remain bound (reg_state == NETREG_REGISTERED and rx_kref > 0), pending
ac_wait_timer instances are not canceled in j1939_ecu_unmap_all(), allowing
them to expire while the interface is down and map ECUs into priv->ents[] via
j1939_ecu_map_locked().

>  	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)
> +void j1939_ecu_wait_timer_start(struct j1939_ecu *ecu)
>  {
> -	/* The ECU is held here and released in the
> -	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel().
> +	lockdep_assert_held(&ecu->priv->lock);
> +
> +	/* The corresponding j1939_ecu_put() is in j1939_ecu_map_timer_handler_locked()
> +	 * or j1939_ecu_wait_timer_cancel() or j1939_ecu_wait_timer_handler().
>  	 */
> -	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),
> +	hrtimer_start(&ecu->ac_wait_timer, ms_to_ktime(250),
>  		      HRTIMER_MODE_REL_SOFT);
>  }
>  
> -void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
> +static void j1939_ecu_map_timer_handler_locked(struct j1939_ecu *ecu)
>  {
> -	if (hrtimer_cancel(&ecu->ac_timer))
> +	if (!test_and_clear_bit(0, &ecu->flags))
> +		return;
> +	/* 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_wait_timer_start(). */
> +	j1939_ecu_put(ecu);
> +}
> +
> +void j1939_ecu_wait_timer_cancel(struct j1939_ecu *ecu)
> +{
> +	if (hrtimer_cancel(&ecu->ac_wait_timer))
> +		/* The corresponding j1939_ecu_get() is in j1939_ecu_wait_timer_start(). */
>  		j1939_ecu_put(ecu);
> +	j1939_ecu_map_timer_handler_locked(ecu);
>  }

[Severity: High]
Can calling j1939_ecu_map_timer_handler_locked(ecu) inside
j1939_ecu_wait_timer_cancel() leave ecu->ac_map_timer queued after the timer
reference is dropped, and also map an ECU whose address claim is being
canceled?

When ecu->ac_wait_timer expires, j1939_ecu_wait_timer_handler() sets bit 0 of
ecu->flags and calls hrtimer_start(&ecu->ac_map_timer, ms_to_ktime(0),
HRTIMER_MODE_REL_SOFT). Because hrtimer_start() computes softexpires from the
current ktime_get() (which has advanced past basenow sampled at the start of
__hrtimer_run_queues()), ecu->ac_map_timer is left queued for a subsequent
softirq timer pass.

If j1939_ecu_wait_timer_cancel(ecu) is called under write_lock_bh(&priv->lock)
(from j1939_ac_process() or j1939_ecu_unmap_all()) after
j1939_ecu_wait_timer_handler() has queued ac_map_timer but before
j1939_ecu_map_timer_handler() acquires priv->lock:

1. hrtimer_cancel(&ecu->ac_wait_timer) returns 0, and
   j1939_ecu_wait_timer_cancel(ecu) calls
   j1939_ecu_map_timer_handler_locked(ecu) without canceling
   ecu->ac_map_timer.

2. j1939_ecu_map_timer_handler_locked(ecu) clears bit 0 of ecu->flags, calls
   j1939_ecu_map_locked(ecu), and calls j1939_ecu_put(ecu) while
   ecu->ac_map_timer remains queued in the hrtimer subsystem.

3. If the caller then drops the last reference on ecu (for example in
   j1939_ecu_unmap_all(), or at out_ecu_put in j1939_ac_process() when an
   idle address is received or address contention is lost),
   __j1939_ecu_release_rcu() can run before ac_map_timer fires (such as on
   CONFIG_PREEMPT_RT or when softirq delivery on the target CPU is delayed)
   and trigger WARN_ON(hrtimer_cancel(&ecu->ac_map_timer)).

4. If j1939_ac_process() instead starts a new address claim cycle via
   j1939_ecu_wait_timer_start(ecu), clearing bit 0 of ecu->flags while the
   prior cycle's ac_map_timer is still queued allows the new cycle's
   j1939_ecu_wait_timer_handler() to set bit 0 and race with the earlier
   cycle's pending j1939_ecu_map_timer_handler(), which can clear bit 0 and
   drop the new cycle's timer kref prematurely.

5. In addition, calling j1939_ecu_map_locked(ecu) when canceling a timer
   maps the ECU even though the address claim is being aborted or replaced,
   which can also emit a spurious netdev_warn("Trying to map already mapped
   ECU...") if another contending ECU is already mapped at ecu->addr.

>  
> -static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
> +static enum hrtimer_restart j1939_ecu_map_timer_handler(struct hrtimer *hrtimer)
>  {
>  	struct j1939_ecu *ecu =
> -		container_of(hrtimer, struct j1939_ecu, ac_timer);
> +		container_of(hrtimer, struct j1939_ecu, ac_map_timer);
>  	struct j1939_priv *priv = ecu->priv;
>  
>  	write_lock_bh(&priv->lock);
> -	/* 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);
> +	j1939_ecu_map_timer_handler_locked(ecu);
>  	write_unlock_bh(&priv->lock);
> +	return HRTIMER_NORESTART;
> +}

[ ... ]

> @@ -160,7 +282,9 @@ struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name)
>  	ecu->addr = J1939_IDLE_ADDR;
>  	ecu->name = name;
>  
> -	hrtimer_setup(&ecu->ac_timer, j1939_ecu_timer_handler, CLOCK_MONOTONIC,
> +	hrtimer_setup(&ecu->ac_wait_timer, j1939_ecu_wait_timer_handler, CLOCK_MONOTONIC,
> +		      HRTIMER_MODE_REL_SOFT);
> +	hrtimer_setup(&ecu->ac_map_timer, j1939_ecu_map_timer_handler, CLOCK_MONOTONIC,
>  		      HRTIMER_MODE_REL_SOFT);

[Severity: High]
Does configuring both ac_wait_timer and ac_map_timer with
HRTIMER_MODE_REL_SOFT still leave a lock inversion with priv->lock on
CONFIG_PREEMPT_RT?

On CONFIG_PREEMPT_RT, hrtimer_run_softirq() holds the per-CPU
cpu_base->softirq_expiry_lock across all HRTIMER_MODE_REL_SOFT timer
callbacks on that CPU:

CPU 0 (softirq timer expiry)
hrtimer_run_softirq()
  spin_lock_bh(&cpu_base->softirq_expiry_lock)
  __hrtimer_run_queues()
    j1939_ecu_map_timer_handler()
      write_lock_bh(&priv->lock)

Meanwhile, j1939_ac_process() and j1939_ecu_unmap_all() call
j1939_ecu_wait_timer_cancel() under write_lock_bh(&priv->lock). If
ac_wait_timer is currently executing its callback, hrtimer_cancel() calls
hrtimer_cancel_wait_running(&ecu->ac_wait_timer), which acquires the same
per-CPU softirq_expiry_lock on CONFIG_PREEMPT_RT:

CPU 1
j1939_ac_process()
  write_lock_bh(&priv->lock)
  j1939_ecu_wait_timer_cancel()
    hrtimer_cancel(&ecu->ac_wait_timer)
      hrtimer_cancel_wait_running()
        spin_lock_bh(&base->cpu_base->softirq_expiry_lock)

Because softirq_expiry_lock is shared per-CPU across all softirq hrtimers
rather than per-timer, if CPU 0 finishes j1939_ecu_wait_timer_handler() and
moves on to j1939_ecu_map_timer_handler() before CPU 1 enters
hrtimer_cancel_wait_running(), can't CPU 0 and CPU 1 deadlock on
softirq_expiry_lock and priv->lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/d0590019-f0c9-4288-9653-a7b6056012af@I-love.SAKURA.ne.jp?part=1