[PATCH v11 1/3] can: j1939: fix possibility of UAF in __j1939_ecu_release()

[PATCH v11 1/3] can: j1939: fix possibility of UAF in __j1939_ecu_release()

[Tetsuo Handa]

Since j1939_ac_recv() and j1939_ac_fixup() are calling j1939_ecu_put()
without priv->lock held for write, list corruption and/or UAF 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).

Since pending ecu->ac_timer is not canceled during j1939_ecu_unmap_all()
or __j1939_rx_release(), ecu->ac_timer can call j1939_ecu_timer_handler()
after __j1939_ecu_release() or j1939_module_exit().

Fix this problem by explicitly canceling ecu->ac_timer before kfree().

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/can/j1939/bus.c        | 68 ++++++++++++++++++++++++++++----------
 net/can/j1939/j1939-priv.h |  4 +++
 net/can/j1939/main.c       | 13 ++++++++
 3 files changed, 67 insertions(+), 18 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..1cee3239c41a 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -14,25 +14,54 @@
 
 #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);
+	/* 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);
 }
 
+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, readers might find ECUs with ecu->kref == 0. Therefore,
+	 * 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 bool __must_check j1939_ecu_get(struct j1939_ecu *ecu)
+{
+	/* Don't get an ECU ref if __j1939_ecu_release() was called. */
+	return kref_get_unless_zero(&ecu->kref);
+}
+
+static bool __must_check 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 +85,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 +140,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 +154,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 +170,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 +220,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 +246,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 +263,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 v11 2/3] can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()

[Tetsuo Handa]

Since j1939_ecu_timer_cancel() is calling hrtimer_cancel() with priv->lock
held for write while j1939_ecu_timer_handler() takes priv->lock, a hard-
lockup (if CONFIG_PREEMPT_RT=n && CONFIG_SMP=y) or a deadlock between
priv->lock and cpu_base->softirq_expiry_lock (if CONFIG_PREEMPT_RT=y) will
happen when hrtimer_cancel() calls hrtimer_cancel_wait_running().

The comment for "struct j1939_priv"->lock says that priv->lock is used for
protecting "struct list_head ecus" and "struct j1939_addr_ent ents[256]".
But j1939_ac_process() is using priv->lock for also serializing timer
management across multiple frames. This makes it impossible to temporarily
drop priv->lock in order to safely call hrtimer_cancel(). Therefore, fix
this problem by dropping an address claim frame when address claim timer
cannot be canceled.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/can/j1939/address-claim.c |  3 ++-
 net/can/j1939/bus.c           | 12 ++++++++++--
 net/can/j1939/j1939-priv.h    |  2 +-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/address-claim.c b/net/can/j1939/address-claim.c
index ca4ad6cdd5cb..89ba8c6fbe74 100644
--- a/net/can/j1939/address-claim.c
+++ b/net/can/j1939/address-claim.c
@@ -212,7 +212,8 @@ 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);
+	if (!j1939_ecu_timer_cancel(ecu))
+		goto out_ecu_put;
 
 	if (j1939_address_is_idle(skcb->addr.sa)) {
 		j1939_ecu_unmap_locked(ecu);
diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index 1cee3239c41a..fb9f6f889549 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -151,11 +151,19 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
 		      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. */
+		return true;
+	case 1:	/* Successfully canceled. */
 		/* Drop the ref from j1939_ecu_timer_start(). */
 		j1939_ecu_put(ecu);
+		return true;
+	default: /* Currently executing. */
+		/* Let the caller decide how to handle this situation. */
+		return false;
+	}
 }
 
 static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index 623aeb2e54e1..49ce008acffb 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -205,7 +205,7 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb);
 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);
+bool __must_check j1939_ecu_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);
-- 
2.56.0

[PATCH v11 3/3] 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

  (1) canceling ecu->ac_timer from j1939_ecu_unmap_all()

unconditionally and

  (2) preventing ecu->ac_timer from re-arming in j1939_ecu_timer_start()
  (3) making j1939_ecu_map_locked() no-op

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

We might want to add "(READ_ONCE(priv->ndev->flags) & IFF_UP) &&" to
j1939_ecu_verify_get() for (2) and (3) if we want j1939_ecu_unmap_all()
 from j1939_netdev_notify(NETDEV_DOWN) path to behave like
j1939_ecu_unmap_all() from j1939_netdev_notify(NETDEV_UNREGISTER) path,
but that should be a different patch. This patch focuses on fixing
j1939_ecu_unmap_all() from j1939_netdev_notify(NETDEV_UNREGISTER) path
which is reported as
"unregister_netdevice: waiting for vcan0 to become free. Usage count = 3"
problem by syzbot.

Reported-by: syzbot+e2af46126e0644cbebdd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e2af46126e0644cbebdd
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/can/j1939/bus.c    | 40 +++++++++++++++++++++++++++++++++++++---
 net/can/j1939/socket.c | 10 +++++++++-
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index fb9f6f889549..5bc46e4ea693 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -58,6 +58,17 @@ static bool __must_check j1939_ecu_get(struct j1939_ecu *ecu)
 	return kref_get_unless_zero(&ecu->kref);
 }
 
+static bool __must_check 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 bool __must_check j1939_ecu_valid(struct j1939_ecu *ecu)
 {
 	/* Treat an ECU as invalid if __j1939_ecu_release() was called. */
@@ -85,7 +96,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];
@@ -130,8 +141,31 @@ 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;
+		if (j1939_ecu_timer_cancel(ecu))
+			continue;
+		/* Get a ref, in order to call hrtimer_cancel() without priv->lock. */
+		if (!j1939_ecu_get(ecu))
+			continue;
+		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);
@@ -141,9 +175,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. */
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 50a598ef5fd4..6e22ec5e88f5 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -484,7 +484,15 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr_unsized *uaddr, in
 			goto out_release_sock;
 		}
 
-		if (ndev->reg_state != NETREG_REGISTERED) {
+		/* Do nothing and fail early if netdev is no longer registered.
+		 * This check is racy but we don't need to recheck after j1939_jsk_add()
+		 * is called, for either j1939_sk_netdev_event_unregister() from
+		 * j1939_netdev_notify(NETDEV_UNREGISTER) (which is retried by
+		 * netdev_wait_allrefs_any() as long as priv->rx_kref held by
+		 * j1939_netdev_start() is preventing j1939_priv_set(priv->ndev, NULL)
+		 * from being called) or j1939_sk_release() will take care of this race.
+		 */
+		if (READ_ONCE(ndev->reg_state) != NETREG_REGISTERED) {
 			dev_put(ndev);
 			ret = -ENODEV;
 			goto out_release_sock;
-- 
2.56.0

Re: [PATCH v11 2/3] can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: j1939: j1939_ac_process() drops address claim when timer
  handler is running
--

commit 80762ad6fa3d1299e982ed85f13042798ab71255
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() with hrtimer_try_to_cancel() in
j1939_ecu_timer_cancel() and drops incoming address claim frames in
j1939_ac_process() when the address claim timer is currently executing.

> diff --git a/net/can/j1939/address-claim.c b/net/can/j1939/address-claim.c
> index ca4ad6cdd5cbf..89ba8c6fbe74d 100644
> --- a/net/can/j1939/address-claim.c
> +++ b/net/can/j1939/address-claim.c
> @@ -212,7 +212,8 @@ 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);
> +	if (!j1939_ecu_timer_cancel(ecu))
> +		goto out_ecu_put;
>  
>  	if (j1939_address_is_idle(skcb->addr.sa)) {
>  		j1939_ecu_unmap_locked(ecu);

[Severity: Medium]
Can jumping to out_ecu_put when j1939_ecu_timer_cancel() returns false
leave ecu->addr and priv->ents[] out of sync with the address claim
frame?

If a new address claim or J1939_IDLE_ADDR frame arrives for the same ECU
while j1939_ecu_timer_handler() is running on another CPU, the handler
can be spinning on write_lock_bh(&priv->lock):

CPU0                              CPU1
j1939_ac_process()
  write_lock_bh(&priv->lock)
                                  j1939_ecu_timer_handler()
                                    write_lock_bh(&priv->lock)
  j1939_ecu_timer_cancel(ecu)
    hrtimer_try_to_cancel() -> -1
  goto out_ecu_put;
  write_unlock_bh(&priv->lock)
                                    j1939_ecu_map_locked(ecu)
                                      <-- maps stale ecu->addr

Even if j1939_ecu_timer_handler() has already dropped priv->lock and is
returning, bailing out skips unmapping the old address, updating
ecu->addr, and starting the 250 ms timer for the new address.  For a
local ECU claiming a new address, j1939_ac_fixup() has already unmapped
the old address on TX, so dropping the echoed RX frame leaves the ECU
permanently unmapped and causes subsequent sendmsg() calls to fail with
-EADDRNOTAVAIL.

Since both j1939_ac_process() and j1939_ecu_timer_handler() serialize on
priv->lock, could timer cancellation be tracked under priv->lock so the
handler skips j1939_ecu_map_locked() when superseded instead of dropping
the frame?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/a1dd9760-7815-400a-a39c-a1a03cf8262e@I-love.SAKURA.ne.jp?part=2

Re: [PATCH v11 2/3] can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()

[netdev-bot+sashiko]

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

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

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] j1939_ac_process() (net/can/j1939/address-claim.c) now gives
  up on an Address Claimed frame whenever hrtimer_try_to_cancel()…
- [Low] On the new hrtimer_try_to_cancel() == -1 path, the in-code
  comment and Documentation/networking/j1939.rst no longer match the…

Pre-existing issues:
- [High] At the target commit, j1939_ecu_unmap_all()
  (net/can/j1939/bus.c) only walks priv->ents[] and calls…

--

can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()

The patch switches j1939_ecu_timer_cancel() from hrtimer_cancel() to
hrtimer_try_to_cancel(). This avoids waiting for a running ac_timer
callback while priv->lock is held for write. If the callback is running,
j1939_ecu_timer_cancel() now returns false and j1939_ac_process() ignores
the address claim frame.

> diff --git a/net/can/j1939/address-claim.c b/net/can/j1939/address-claim.c
> index ca4ad6cdd5cbf..89ba8c6fbe74d 100644
> --- a/net/can/j1939/address-claim.c
> +++ b/net/can/j1939/address-claim.c
> @@ -212,7 +212,8 @@ 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);
> +	if (!j1939_ecu_timer_cancel(ecu))
> +		goto out_ecu_put;

[Severity: Medium]

If the previous ac_timer callback is running, this path skips several
steps: the idle/unmap handling, the ecu->addr update, the contention
check against prev, and j1939_ecu_timer_start(). Could the kernel then
end up committing the superseded claim instead of the new one?

Once j1939_ac_process() releases priv->lock, the running handler takes
it and maps the ECU at the old, unchanged ecu->addr:

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

Say the new claim moved the ECU to a different SA, or was a Cannot-Claim
with SA 0xFE that should have unmapped it. That update seems to be lost.
The old NAME<->SA pairing would then stay in priv->ents[] until the NAME
sends another AC, and ACs are not periodic.

The frame is also not really dropped, as the commit message suggests,
because j1939_can_recv() still passes it on:

net/can/j1939/main.c:j1939_can_recv() {
	...
	j1939_ac_recv(priv, skb);

	if (j1939_tp_recv(priv, skb))
		goto done;

	j1939_simple_recv(priv, skb);
	j1939_sk_recv(priv, skb);
	...
}

So userspace sees the NAME at the new SA while the kernel keeps it at
the old SA. Would j1939_name_to_addr() then return the stale SA? If so,
j1939_ac_fixup() would fill in the wrong SA for sockets bound by NAME,
and the wrong DA for dst_name sends.

For local ECUs, j1939_ac_fixup() unmaps the ECU when the new claim is
transmitted, but the running handler would then map it back at the old
SA.

hrtimer_try_to_cancel() also returns -1 for as long as
hrtimer_callback_running() is true, not only while the handler is
spinning on priv->lock. That includes the window after the handler's
write_unlock_bh() but before __run_hrtimer() clears base->running. On
PREEMPT_RT this window can be wider, because the softirq thread can be
preempted.

In that window the old hrtimer_cancel() would have waited briefly
without deadlocking, then processed the claim correctly. Are claims that
used to be handled correctly now lost too?

The commit message says it is impossible to drop priv->lock, and
"Therefore" the frame is dropped. Is ignoring the claim the only option
that keeps the lock held?

One alternative: on -1, carry on with the update and re-arm the timer.
Calling hrtimer_start() on a timer whose callback is running is allowed.
j1939_ecu_timer_handler() could then skip the mapping if
hrtimer_is_queued() is true, or if a per-ECU generation counter has
changed.

This code is still the same at the end of the series.

[Severity: Low]

This isn't a bug, but the comment above the call still says "cancel
pending (previous) address claim". On the new false path, the previous
claim is not cancelled, because its running handler maps the ECU at the
old SA. The new claim is the one that gets discarded. Could a comment
here say what happens to the frame in that case?

j1939_ecu_timer_cancel() says "Let the caller decide how to handle this
situation". However, j1939_ac_process(), the only caller, does not
document what it chose.

Documentation/networking/j1939.rst says that after binding with a new
SA and sending a valid address claim:

  This restarts the state machine in the kernel (and any other
  participant on the bus) for this NAME.

It also says the NAME-SA pair becomes valid if nobody contests it within
250ms. When this race hits, the state does not change and no 250ms timer
is started for the new claim. Should the documentation be updated to
match?

>  
>  	if (j1939_address_is_idle(skcb->addr.sa)) {
>  		j1939_ecu_unmap_locked(ecu);
> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index c49c96dea7960..c6278d7cb1b1f 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -151,11 +151,19 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  		      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. */
> +		return true;
> +	case 1:	/* Successfully canceled. */
>  		/* Drop the ref from j1939_ecu_timer_start(). */
>  		j1939_ecu_put(ecu);
> +		return true;
> +	default: /* Currently executing. */
> +		/* Let the caller decide how to handle this situation. */
> +		return false;
> +	}
>  }

[Severity: High]

This is a pre-existing issue and was not introduced by this patch. At
this commit, j1939_ecu_unmap_all() does not cancel any pending
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);
}

Suppose an AC frame arms ac_timer just before the last socket closes:

j1939_sk_release()
  j1939_netdev_stop()
    __j1939_rx_release()
      j1939_can_rx_unregister(priv);
      j1939_ecu_unmap_all(priv);
      j1939_priv_set(priv->ndev, NULL);

The timer can still fire up to 250ms later. Can j1939_ecu_timer_handler()
then map the ECU back into the detached priv, taking an ECU reference
that is never dropped? That would leak the ECU, the priv and the netdev
tracker reference, and unregister_netdevice could wait forever.

Can the handler also run after j1939_module_exit() has destroyed
j1939_ecu_wq and the module has been unloaded?

The hrtimer_cancel() in __j1939_ecu_release_work() can't help here,
because the armed timer itself holds an ECU reference.

The later patch in this series, "can: j1939: fix "struct j1939_ecu"
refcount leak", appears to fix this. With it, j1939_ecu_unmap_all()
walks priv->ecus and tries to cancel each timer under priv->lock. If
that fails, it drops priv->lock, holds a temporary reference, and calls
hrtimer_cancel().

j1939_ecu_timer_start() and j1939_ecu_map_locked() also switch to
j1939_ecu_verify_get(). It refuses once rx_kref is 0 or the netdev is
unregistered.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/a1dd9760-7815-400a-a39c-a1a03cf8262e%40I-love.SAKURA.ne.jp

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not mention the net/can/j1939/socket.c
  hunk at all.
- [Low] In j1939_ecu_unmap_all() (net/can/j1939/bus.c), the comment on
  the second j1939_ecu_put() reads "Drop the ref from default: above."…

--

can: j1939: fix "struct j1939_ecu" refcount leak

This patch fixes a j1939_ecu refcount leak. The leak happens when
j1939_ecu_timer_handler() remaps an ECU after j1939_ecu_unmap_all() has
already unmapped it. The patch cancels ecu->ac_timer in
j1939_ecu_unmap_all() and adds j1939_ecu_verify_get(), which stops
j1939_ecu_timer_start() and j1939_ecu_map_locked() from taking new refs
once the netdev is unregistered or priv->rx_kref has reached zero.

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

> We might want to add "(READ_ONCE(priv->ndev->flags) & IFF_UP) &&" to
> j1939_ecu_verify_get() for (2) and (3) if we want j1939_ecu_unmap_all()
>  from j1939_netdev_notify(NETDEV_DOWN) path to behave like
> j1939_ecu_unmap_all() from j1939_netdev_notify(NETDEV_UNREGISTER) path,
> but that should be a different patch. This patch focuses on fixing
> j1939_ecu_unmap_all() from j1939_netdev_notify(NETDEV_UNREGISTER) path
> which is reported as
> "unregister_netdevice: waiting for vcan0 to become free. Usage count = 3"
> problem by syzbot.

[Severity: Low]
Is this description of the call path accurate? In j1939_netdev_notify(),
only the NETDEV_DOWN case calls j1939_ecu_unmap_all() directly:

net/can/j1939/main.c:j1939_netdev_notify() {
    ...
	case NETDEV_DOWN:
		j1939_cancel_active_session(priv, NULL);
		j1939_sk_netdev_event_netdown(priv);
		j1939_ecu_unmap_all(priv);
		break;
	case NETDEV_UNREGISTER:
		j1939_cancel_active_session(priv, NULL);
		j1939_sk_netdev_event_netdown(priv);
		j1939_sk_netdev_event_unregister(priv);
		break;
    ...
}

The NETDEV_UNREGISTER path reaches j1939_ecu_unmap_all() only indirectly,
and only when the last rx_kref is dropped:

j1939_sk_netdev_event_unregister()
  j1939_netdev_stop()
    kref_put_mutex(&priv->rx_kref, __j1939_rx_release, ...)
      __j1939_rx_release()
        j1939_ecu_unmap_all()

The commit message also doesn't mention the net/can/j1939/socket.c hunk.
That hunk changes the reg_state check in j1939_sk_bind() to use
READ_ONCE(). It also adds a long comment explaining why a bind() racing
with NETDEV_UNREGISTER needs no recheck. The new reg_state test in
j1939_ecu_verify_get() relies on that reasoning.

Could the changelog describe the indirect UNREGISTER path and the
j1939_sk_bind() change?

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index c6278d7cb1b1f..f444a1f33582b 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c

[ ... ]

> @@ -130,8 +141,31 @@ 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);

[ ... ]

> +	list_for_each_entry(ecu, &priv->ecus, list) {
> +		if (!j1939_ecu_valid(ecu))
> +			continue;
> +		if (j1939_ecu_timer_cancel(ecu))
> +			continue;
> +		/* Get a ref, in order to call hrtimer_cancel() without priv->lock. */
> +		if (!j1939_ecu_get(ecu))
> +			continue;
> +		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);

[Severity: Low]
This isn't a bug, but does "default: above" point to the right place?
j1939_ecu_unmap_all() has no default: label. The nearest one is the
"Currently executing" case in j1939_ecu_timer_cancel(), and that case
takes no reference:

	default: /* Currently executing. */
		/* Let the caller decide how to handle this situation. */
		return false;

The ref dropped here comes from the j1939_ecu_get(ecu) call a few lines
earlier in the same loop.

This file tracks get/put pairs through comments like this one. Could the
comment name that j1939_ecu_get() call instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/a1dd9760-7815-400a-a39c-a1a03cf8262e%40I-love.SAKURA.ne.jp