[PATCH v9 1/2] can: j1939: fix possibility of list corruption in __j1939_ecu_release()

[PATCH v9 1/2] can: j1939: fix possibility of list corruption in __j1939_ecu_release()

[Tetsuo Handa]

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 WQ context where we can
unconditionally hold priv->lock for write.

By deferring to WQ 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).

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
This patch allows us to use tasklet_kill() in __j1939_ecu_release_work()
when addressing "can: j1939: ABBA deadlock in j1939_ecu_timer_cancel()
under priv->lock" problem.

 net/can/j1939/bus.c        | 64 +++++++++++++++++++++++++++-----------
 net/can/j1939/j1939-priv.h |  4 +++
 net/can/j1939/main.c       | 13 ++++++++
 3 files changed, 63 insertions(+), 18 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..3422e3133800 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -14,25 +14,50 @@
 
 #include "j1939-priv.h"
 
-static void __j1939_ecu_release(struct kref *kref)
+struct workqueue_struct *j1939_ecu_wq;
+
+static void __j1939_ecu_release_work(struct work_struct *work)
 {
-	struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
+	struct j1939_ecu *ecu = container_of(work, struct j1939_ecu, release_work);
 	struct j1939_priv *priv = ecu->priv;
 
+	write_lock_bh(&priv->lock);
 	list_del(&ecu->list);
+	write_unlock_bh(&priv->lock);
 	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 WQ context. By deferring to
+	 * WQ 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().
+	 */
+	INIT_WORK(&ecu->release_work, __j1939_ecu_release_work);
+	queue_work(j1939_ecu_wq, &ecu->release_work);
+}
+
 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)
 {
-	kref_get(&ecu->kref);
+	/* Don't get an ECU ref if __j1939_ecu_release() was called. */
+	return kref_get_unless_zero(&ecu->kref);
+}
+
+static __must_check bool j1939_ecu_valid(struct j1939_ecu *ecu)
+{
+	/* 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 +81,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;
 }
@@ -109,10 +136,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),
@@ -122,6 +150,7 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
 void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
 {
 	if (hrtimer_cancel(&ecu->ac_timer))
+		/* Drop the ref from j1939_ecu_timer_start(). */
 		j1939_ecu_put(ecu);
 }
 
@@ -137,9 +166,7 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
 	 */
 	j1939_ecu_map_locked(ecu);
 
-	/* The corresponding j1939_ecu_get() is in
-	 * j1939_ecu_timer_start().
-	 */
+	/* Drop the ref from j1939_ecu_timer_start(). */
 	j1939_ecu_put(ecu);
 	write_unlock_bh(&priv->lock);
 
@@ -189,8 +216,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 +242,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 +259,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..623aeb2e54e1 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -42,6 +42,8 @@ struct j1939_ecu {
 
 	/* count users, to help transport protocol decide for interaction */
 	int nusers;
+
+	struct work_struct release_work;
 };
 
 struct j1939_priv {
@@ -345,4 +347,6 @@ void j1939_session_timers_cancel(struct j1939_session *session);
 /* CAN protocol */
 extern const struct can_proto j1939_can_proto;
 
+extern struct workqueue_struct *j1939_ecu_wq;
+
 #endif /* _J1939_PRIV_H_ */
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 5e5e6c228f22..22c272f8dfde 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -401,6 +401,12 @@ static __init int j1939_module_init(void)
 
 	pr_info("can: SAE J1939\n");
 
+	j1939_ecu_wq = alloc_workqueue("j1939_ecu", WQ_HIGHPRI | WQ_PERCPU, 0);
+	if (!j1939_ecu_wq) {
+		ret = -ENOMEM;
+		goto fail_wq;
+	}
+
 	ret = register_netdevice_notifier(&j1939_netdev_notifier);
 	if (ret)
 		goto fail_notifier;
@@ -416,6 +422,8 @@ static __init int j1939_module_init(void)
  fail_sk:
 	unregister_netdevice_notifier(&j1939_netdev_notifier);
  fail_notifier:
+	destroy_workqueue(j1939_ecu_wq);
+ fail_wq:
 	return ret;
 }
 
@@ -424,6 +432,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_work() is
+	 * no longer running before unmapping memory for this module.
+	 */
+	destroy_workqueue(j1939_ecu_wq);
 }
 
 module_init(j1939_module_init);
-- 
2.56.0

[PATCH v9 2/2] can: j1939: fix "struct j1939_ecu" refcount leak

[Tetsuo Handa]

syzbot is reporting "struct j1939_ecu" refcount leak, for

  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.

Fix this problem by canceling ecu->ac_timer from j1939_ecu_unmap_all(),
preventing ecu->ac_timer from re-arming in j1939_ecu_timer_start() /
making j1939_ecu_map_locked() no-op / failing j1939_local_ecu_get()
when a netdev which is associated with j1939_priv is no longer registered
(indicated by ecu->priv->ndev->reg_state != NETREG_REGISTERED) or
__j1939_rx_release() was called (indicated by ecu->priv->rx_kref == 0).

Reported-by: syzbot+e2af46126e0644cbebdd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e2af46126e0644cbebdd
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
To save cost of AI reviews and to quickly close frequently reproducing bug,
patch for "can: j1939: ABBA deadlock in j1939_ecu_timer_cancel() under
priv->lock" problem is deferred.

 net/can/j1939/bus.c | 65 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 3 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index 3422e3133800..4844b92aa028 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -24,6 +24,11 @@ static void __j1939_ecu_release_work(struct work_struct *work)
 	write_lock_bh(&priv->lock);
 	list_del(&ecu->list);
 	write_unlock_bh(&priv->lock);
+	/* There can't be any ref on this ECU because __j1939_ecu_release()
+	 * was already called. But we need to make sure that
+	 * j1939_ecu_timer_handler() is no longer running before kfree().
+	 */
+	WARN_ON(hrtimer_cancel(&ecu->ac_timer));
 	netdev_put(priv->ndev, &ecu->priv_dev_tracker);
 	kfree(ecu);
 	j1939_priv_put(priv);
@@ -54,6 +59,17 @@ static __must_check bool j1939_ecu_get(struct j1939_ecu *ecu)
 	return kref_get_unless_zero(&ecu->kref);
 }
 
+static bool j1939_ecu_verify_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) && j1939_ecu_get(ecu);
+}
+
 static __must_check bool j1939_ecu_valid(struct j1939_ecu *ecu)
 {
 	/* Treat an ECU as invalid if __j1939_ecu_release() was called. */
@@ -81,7 +97,7 @@ static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
 	if (!j1939_address_is_unicast(ecu->addr))
 		return;
 
-	if (!j1939_ecu_get(ecu))
+	if (!j1939_ecu_verify_get(ecu))
 		return;
 
 	ent = &priv->ents[ecu->addr];
@@ -126,8 +142,39 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
 void j1939_ecu_unmap_all(struct j1939_priv *priv)
 {
 	int i;
+	struct j1939_ecu *ecu;
 
+restart:
 	write_lock_bh(&priv->lock);
+	/* Need to shutdown timers now, for module unloading might start
+	 * 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_calcel() without priv->lock. */
+			if (!j1939_ecu_get(ecu))
+				continue;
+			break;
+		}
+		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 restart;
+	}
+	/* 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);
@@ -137,9 +184,9 @@ void j1939_ecu_unmap_all(struct j1939_priv *priv)
 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. */
@@ -149,6 +196,9 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
 
 void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
 {
+	/* FIXME: Calling hrtimer_cancel() under priv->lock can deadlock between
+	 * priv->lock and cpu_base->softirq_expiry_lock if CONFIG_PREEMPT_RT=y.
+	 */
 	if (hrtimer_cancel(&ecu->ac_timer))
 		/* Drop the ref from j1939_ecu_timer_start(). */
 		j1939_ecu_put(ecu);
@@ -310,6 +360,15 @@ int j1939_local_ecu_get(struct j1939_priv *priv, name_t name, u8 sa)
 
 	write_lock_bh(&priv->lock);
 
+	/* Do nothing and fail 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))) {
+		err = -ENODEV;
+		goto done;
+	}
+
 	if (j1939_address_is_unicast(sa))
 		priv->ents[sa].nusers++;
 
-- 
2.56.0

Re: [PATCH v9 1/2] can: j1939: fix possibility of list corruption in __j1939_ecu_release()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] can: j1939: missing Fixes trailer in commit message
--

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

> can: j1939: 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 WQ context where we can
> unconditionally hold priv->lock for write.
>
> By deferring to WQ 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).
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

[Severity: Low]
Since this fixes unlocked list removal in __j1939_ecu_release(), should
the commit message include a Fixes tag:

  Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/396b5de7-3fe0-474f-9194-b4816fde7182@I-love.SAKURA.ne.jp?part=1

Re: [PATCH v9 2/2] can: j1939: fix "struct j1939_ecu" refcount leak

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: j1939: race between j1939_local_ecu_get() and
  j1939_jsk_add() on unbind
--

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

can: j1939: fix "struct j1939_ecu" refcount leak

This commit cancels ECU address-claim timers during j1939_ecu_unmap_all() and
blocks taking new ECU references or binding local ECUs once the netdev is no
longer registered or rx_kref reaches zero.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..7346248a18020 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c

[ ... ]

> @@ -310,6 +360,15 @@ int j1939_local_ecu_get(struct j1939_priv *priv, name_t name, u8 sa)
>  
>  	write_lock_bh(&priv->lock);
>  
> +	/* Do nothing and fail 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))) {
> +		err = -ENODEV;
> +		goto done;
> +	}
> +
>  	if (j1939_address_is_unicast(sa))
>  		priv->ents[sa].nusers++;
>  

[Severity: Medium]
Can j1939_sk_bind() still race with NETDEV_UNREGISTER between
j1939_local_ecu_get() and j1939_jsk_add(), leaving the socket bound and
holding references to struct j1939_ecu, struct j1939_priv, and priv->ndev
after j1939_sk_netdev_event_unregister() has finished?

In j1939_sk_bind(), the socket is not on priv->j1939_socks (or has just been
removed by j1939_jsk_del() on re-bind) when j1939_local_ecu_get() runs, and
j1939_jsk_add() is only called after j1939_local_ecu_get() drops priv->lock:

net/can/j1939/socket.c:j1939_sk_bind() {
    ...
	/* get new references */
	ret = j1939_local_ecu_get(priv, jsk->addr.src_name, jsk->addr.sa);
	if (ret) {
    ...
	}

	j1939_jsk_add(priv, jsk);
    ...
}

Because unregister_netdevice_many_notify() sets NETREG_UNREGISTERING without
holding priv->lock or the socket lock, the two paths can interleave:

CPU0                                  CPU1
j1939_sk_bind()
  j1939_local_ecu_get()
    write_lock_bh(&priv->lock)
    reg_state == NETREG_REGISTERED
    j1939_ecu_create_locked()
    write_unlock_bh(&priv->lock)
                                      unregister_netdevice_many_notify()
                                        dev->reg_state = NETREG_UNREGISTERING
                                        j1939_netdev_notify()
                                          j1939_sk_netdev_event_unregister()
                                            /* jsk not in priv->j1939_socks */
  j1939_jsk_add(priv, jsk)
    /* jsk left bound after unregister */

j1939_sk_netdev_event_unregister() misses jsk because it only scans
priv->j1939_socks, so jsk remains bound with its ECU, priv, and netdev
references held until the socket is closed, blocking unregister_netdevice().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/396b5de7-3fe0-474f-9194-b4816fde7182@I-love.SAKURA.ne.jp?part=2

Re: [PATCH v9 1/2] can: j1939: fix possibility of list corruption in __j1939_ecu_release()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] At this commit, j1939_module_exit() relies on
  destroy_workqueue(j1939_ecu_wq) to stop all __j1939_ecu_release_work().
- [Low] Commit "can: j1939: fix possibility of list corruption in
  __j1939_ecu_release()" fixes a real memory-safety bug but has no…
- [Low] The commit message says only that 'list corruption might happen'
  and does not name the callers that put without the lock.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is still
  present at the end of the series.

--

can: j1939: fix possibility of list corruption in __j1939_ecu_release()

This patch moves the release steps for struct j1939_ecu out of
__j1939_ecu_release() and into a new j1939_ecu_wq workqueue. There,
priv->lock can be held for write around list_del(). Lookups now use
kref_get_unless_zero() and kref_read(), so they skip ECUs whose refcount
has already reached zero.

> 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.

[Severity: Low]
Does "list corruption" cover the whole problem? Could the commit message
name the callers that drop the last reference without the lock, and the
use-after-free that follows?

For example, j1939_ac_recv() gets its reference from
j1939_ecu_get_by_addr(), which drops priv->lock before it returns. It
then puts that reference without the lock:

net/can/j1939/address-claim.c:j1939_ac_recv() {
    ...
		ecu = j1939_ecu_get_by_addr(priv, skcb->addr.sa);
		if (ecu) {
			skcb->addr.src_name = ecu->name;
			j1939_ecu_put(ecu);
		}
    ...
}

If that put is the last one, the old __j1939_ecu_release() runs
list_del(&ecu->list) and kfree(ecu) without priv->lock.

At the same time, another CPU can be walking priv->ecus under
read_lock_bh() in j1939_ecu_get_by_name_locked(),
j1939_ecu_find_by_name_locked() or j1939_name_to_addr(). That CPU would
dereference the entry being freed. The old j1939_ecu_get_by_name_locked()
could also kref_get() an ECU whose refcount had already reached zero.

Since frames on the CAN bus can trigger this, should the message call it
a use-after-free?

> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

[Severity: Low]
Should this have a Fixes: tag and a Cc: stable? The unlocked list_del()
and kfree() in __j1939_ecu_release() seem to go back to the original
J1939 commit:

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

The next patch in this series, "can: j1939: fix "struct j1939_ecu"
refcount leak", has that tag.

This patch also uses WQ_PERCPU, which older trees don't have, so
backports will need changes. Could the stable intent be stated
explicitly?

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

[ ... ]

> @@ -122,6 +150,7 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
>  {
>  	if (hrtimer_cancel(&ecu->ac_timer))
> +		/* Drop the ref from j1939_ecu_timer_start(). */
>  		j1939_ecu_put(ecu);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can the synchronous
hrtimer_cancel() here deadlock against j1939_ecu_timer_handler()?

j1939_ac_process() calls j1939_ecu_timer_cancel() while it holds
priv->lock for write:

net/can/j1939/address-claim.c:j1939_ac_process() {
    ...
	write_lock_bh(&priv->lock);
    ...
	/* cancel pending (previous) address claim */
	j1939_ecu_timer_cancel(ecu);
    ...
}

The timer callback takes the same lock first:

net/can/j1939/bus.c:j1939_ecu_timer_handler() {
    ...
	write_lock_bh(&priv->lock);
    ...
}

CPU0                                  CPU1
j1939_can_recv()
 j1939_ac_recv()
  j1939_ac_process()
   write_lock_bh(&priv->lock)
                                      j1939_ecu_timer_handler()
                                       write_lock_bh(&priv->lock)
                                       <spins>
   j1939_ecu_timer_cancel()
    hrtimer_cancel()
     <callback running, loops>

hrtimer_try_to_cancel() keeps returning -1 while the callback runs. On
!PREEMPT_RT, hrtimer_cancel_wait_running() is only cpu_relax(), so
neither CPU makes progress, and bottom halves are disabled on both.

The trigger is an address-claim frame for the same name with a
different source address. It has to arrive just as that ECU's 250 ms
ac_timer starts running on another CPU, so any node on the CAN bus can
cause it.

The notes after the commit message call this a separate problem ("ABBA
deadlock in j1939_ecu_timer_cancel() under priv->lock"). The FIXME that
the later patch in this series adds in j1939_ecu_timer_cancel() only
mentions the PREEMPT_RT softirq_expiry_lock variant.

Does the plain spin-versus-wait case above on !PREEMPT_RT need handling
too? It still looks present at the end of the series.

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index d9384cf0e356f..2030f24fce704 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c

[ ... ]

> @@ -424,6 +432,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_work() is
> +	 * no longer running before unmapping memory for this module.
> +	 */
> +	destroy_workqueue(j1939_ecu_wq);
>  }

[Severity: Medium]
At this commit, can a pending ecu->ac_timer still queue release work
after destroy_workqueue() has run?

destroy_workqueue() only drains work that is already queued. At this
point, j1939_ecu_unmap_all() does not cancel ecu->ac_timer:

net/can/j1939/bus.c:j1939_ecu_unmap_all() {
	...
	write_lock_bh(&priv->lock);
	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);
}

The timer still holds the ECU reference taken in
j1939_ecu_timer_start(). If rmmod runs within 250 ms of the last j1939
socket closing, the timer can fire afterwards and do this:

j1939_ecu_timer_handler()
  j1939_ecu_put()
    __j1939_ecu_release()
      queue_work(j1939_ecu_wq, &ecu->release_work);

That would queue work on a workqueue that has already been destroyed.

The timer outliving the module was already a problem before this patch,
because j1939_ecu_timer_handler() is itself module text.

The later patch in this series, "can: j1939: fix "struct j1939_ecu"
refcount leak", appears to fix this. It makes j1939_ecu_unmap_all()
cancel every ac_timer and drop the timer references. It also gates
j1939_ecu_timer_start() and j1939_ecu_map_locked() with
j1939_ecu_verify_get(), which fails once priv->rx_kref is zero.

Would it make sense to reorder or combine the two patches, so that this
commit is safe by itself for bisection and backports?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/396b5de7-3fe0-474f-9194-b4816fde7182%40I-love.SAKURA.ne.jp

Re: [PATCH v9 2/2] can: j1939: fix "struct j1939_ecu" refcount leak

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] can: j1939: race between j1939_local_ecu_get() and
  j1939_jsk_add() on unbind
- [Low] Undeclared stable dependency.
- [Low] The new '!kref_read(&priv->rx_kref)' condition in
  j1939_local_ecu_get() (bus.c:366-367) can never be true.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
  a FIXME that describes it wrongly.

--

can: j1939: fix "struct j1939_ecu" refcount leak

This commit cancels ecu->ac_timer in j1939_ecu_unmap_all(). It also stops
j1939_ecu_timer_start(), j1939_ecu_map_locked() and j1939_local_ecu_get()
from taking new references once the netdev is unregistered or
__j1939_rx_release() has run. The goal is to fix a syzbot-reported ECU
refcount leak, where the timer handler re-maps an ECU after
j1939_ecu_unmap_all() has finished.

Closes: https://syzkaller.appspot.com/bug?extid=e2af46126e0644cbebdd

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..7346248a18020 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c

[ ... ]

> @@ -126,8 +142,39 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
>  void j1939_ecu_unmap_all(struct j1939_priv *priv)
>  {
>  	int i;
> +	struct j1939_ecu *ecu;
>  
> +restart:
>  	write_lock_bh(&priv->lock);
> +	/* Need to shutdown timers now, for module unloading might start
> +	 * 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;

[Severity: Low]
This commit has a Fixes: tag for 9d71dd0c7009, so it will be queued for
the stable trees. Does it depend on the previous patch in the series, "can:
j1939: fix possibility of list corruption in __j1939_ecu_release()"?

That commit has no Fixes: or Cc: stable tag. Neither commit mentions the
dependency.

This j1939_ecu_put() can drop the last reference while priv->ecus is being
walked with the non-_safe list_for_each_entry() under priv->lock. The same
applies to the j1939_ecu_put() in the "already mapped" branch of
j1939_ecu_map_locked(). Both are safe only because the prerequisite moves
list_del() and kfree() into __j1939_ecu_release_work().

Without the prerequisite, __j1939_ecu_release() does list_del(&ecu->list)
and kfree(ecu) synchronously. list_next_entry() would then read freed
memory.

j1939_ecu_valid() and the bool return of j1939_ecu_get() also come from
the prerequisite. A straight backport would therefore fail to build, but a
hand-adapted one could end up with this use-after-free.

Could the prerequisite get a Fixes: or Cc: stable tag, or could this commit
name the dependency?

> +		default: /* Currently executing. */
> +			/* Get a ref, in order to call hrtimer_calcel() without priv->lock. */
> +			if (!j1939_ecu_get(ecu))
> +				continue;
> +			break;
> +		}

[ ... ]

> @@ -149,6 +196,9 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  
>  void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
>  {
> +	/* FIXME: Calling hrtimer_cancel() under priv->lock can deadlock between
> +	 * priv->lock and cpu_base->softirq_expiry_lock if CONFIG_PREEMPT_RT=y.
> +	 */

[Severity: High]
This deadlock is pre-existing and was not introduced by this patch. Is the
new FIXME right to limit it to CONFIG_PREEMPT_RT=y, though?

j1939_ac_process() calls j1939_ecu_timer_cancel() while it holds
priv->lock:

net/can/j1939/address-claim.c:j1939_ac_process() {
    ...
	write_lock_bh(&priv->lock);
    ...
	/* cancel pending (previous) address claim */
	j1939_ecu_timer_cancel(ecu);
    ...
}

The timer callback takes the same lock:

net/can/j1939/bus.c:j1939_ecu_timer_handler() {
    ...
	write_lock_bh(&priv->lock);
    ...
	j1939_ecu_map_locked(ecu);
    ...
}

Suppose the 250 ms ac_timer expires on another CPU while
j1939_ac_process() is running for the same ECU:

CPU A                               CPU B
j1939_ac_process()
  write_lock_bh(&priv->lock)
                                    __run_hrtimer()
                                      j1939_ecu_timer_handler()
                                        write_lock_bh(&priv->lock)
                                        <spins>
  j1939_ecu_timer_cancel()
    hrtimer_cancel()
      hrtimer_try_to_cancel() returns -1
      hrtimer_cancel_wait_running()
      <loops forever>

Without CONFIG_PREEMPT_RT, hrtimer_cancel_wait_running() is just
cpu_relax(). Wouldn't this be a hard lockup with BH disabled on any SMP
kernel?

j1939_ac_process() runs from j1939_can_recv()->j1939_ac_recv() for every
Address Claimed frame. A peer on the bus can reach this window by sending a
second address claim for the same NAME with a different SA.

The new j1939_ecu_unmap_all() already avoids this pattern: it calls only
hrtimer_try_to_cancel() under priv->lock, and calls hrtimer_cancel() after
dropping the lock.

The notes below the --- line say that the fix for the "ABBA deadlock in
j1939_ecu_timer_cancel() under priv->lock" problem is deferred. Could the
FIXME describe the general deadlock instead? Could the deferred fix also
cover non-RT kernels?

>  	if (hrtimer_cancel(&ecu->ac_timer))
>  		/* Drop the ref from j1939_ecu_timer_start(). */
>  		j1939_ecu_put(ecu);
> @@ -310,6 +360,15 @@ int j1939_local_ecu_get(struct j1939_priv *priv, name_t name, u8 sa)
>  
>  	write_lock_bh(&priv->lock);
>  
> +	/* Do nothing and fail 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))) {

[Severity: Medium]
Does this check fully close the window it is aimed at?

j1939_local_ecu_get() tests priv->ndev->reg_state under priv->lock.
The socket only becomes visible to j1939_sk_netdev_event_unregister()
later, when j1939_sk_bind() calls j1939_jsk_add() under
priv->j1939_socks_lock:

j1939_sk_bind()
    ...
    ret = j1939_local_ecu_get(priv, jsk->addr.src_name, jsk->addr.sa);
    ...
    j1939_jsk_add(priv, jsk);

In between, jsk is not on priv->j1939_socks. On a first bind,
j1939_jsk_add() hasn't run yet. On a re-bind, j1939_jsk_del() has
already removed jsk and cleared J1939_SOCK_BOUND.

unregister_netdevice_many_notify() sets reg_state to
NETREG_UNREGISTERING without taking priv->lock or lock_sock().
Nothing orders it against the bind path:

CPU A                         CPU B
j1939_sk_bind()
  j1939_local_ecu_get()
    sees NETREG_REGISTERED
    gets/creates local ECU
                              unregister_netdevice_many_notify()
                                reg_state = NETREG_UNREGISTERING
                                j1939_netdev_notify()
                                  j1939_sk_netdev_event_unregister()
                                    jsk not on j1939_socks
  j1939_jsk_add(priv, jsk)

The socket then stays J1939_SOCK_BOUND on priv->j1939_socks after
NETDEV_UNREGISTER has completed. It keeps its references on the local
j1939_ecu, on j1939_priv and on priv->ndev. So unregister_netdevice()
blocks on the netdev refcount until userspace closes the socket.

The window is narrow. It also needs an interface unregister or netns
teardown racing with bind(). Before this patch the window was wider,
since there was no check at all. Still, the commit message presents
failing j1939_local_ecu_get() as what stops new references after
unregister, and this path gets around it.

Could the bind path re-check reg_state after j1939_jsk_add(), under
priv->j1939_socks_lock, and undo the bind if the device is going
away? Or is there a better lock to serialize
j1939_sk_netdev_event_unregister() against j1939_sk_bind()?

[Severity: Low]
Can the !kref_read(&priv->rx_kref) condition ever be true here?

The only caller is j1939_sk_bind(), and it holds an rx_kref reference when
it calls j1939_local_ecu_get():

j1939_sk_bind()
    priv = j1939_netdev_start(ndev);
    ...
    ret = j1939_local_ecu_get(priv, jsk->addr.src_name, jsk->addr.sa);

On a first bind, j1939_netdev_start() either calls kref_init() on rx_kref
or calls kref_get() on it under j1939_netdev_lock. __j1939_rx_release()
clears can_ml->j1939_priv under the same mutex. A priv found there should
therefore always have rx_kref > 0.

On a re-bind, the socket is already J1939_SOCK_BOUND and keeps its rx_kref
until j1939_netdev_stop(). That only runs after j1939_local_ecu_get()
fails. j1939_sk_netdev_event_unregister() can't drop the reference
concurrently, because bind holds lock_sock().

The commit message also describes this check as effective:

    failing j1939_local_ecu_get()
    when a netdev which is associated with j1939_priv is no longer registered
    (indicated by ecu->priv->ndev->reg_state != NETREG_REGISTERED) or
    __j1939_rx_release() was called (indicated by ecu->priv->rx_kref == 0).

Should the rx_kref check be dropped here, leaving it only in
j1939_ecu_verify_get()? Should the commit message be updated to match?

> +		err = -ENODEV;
> +		goto done;
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/396b5de7-3fe0-474f-9194-b4816fde7182%40I-love.SAKURA.ne.jp