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

[PATCH v8 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 v8 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[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 undoing j1939_ecu_get() in j1939_ecu_map_locked()
and failing j1939_ecu_create_locked() 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).

For now we don't cancel ecu->ac_timer from j1939_ecu_unmap_all(), for
we can't safely call hrtimer_cancel() with priv->lock held. However,
since ecu->ac_timer expires in 250ms, for now let's tolerate delaying
completion of NETDEV_UNREGISTER event for less than one second.

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>
---
 net/can/j1939/bus.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index 3422e3133800..77b678806057 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -84,6 +84,15 @@ static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
 	if (!j1939_ecu_get(ecu))
 		return;
 
+	/* Drop the ref 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)) {
+		j1939_ecu_put(ecu);
+		return;
+	}
+
 	ent = &priv->ents[ecu->addr];
 
 	if (ent->ecu) {
@@ -179,6 +188,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);
-- 
2.56.0

Re: [PATCH v8 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] can: j1939: use-after-free in j1939_ecu_timer_handler() on module exit
- [High] can: j1939: missing reg_state check in j1939_local_ecu_get() on bind
--

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

can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

This commit fixes a struct j1939_ecu reference count leak when
j1939_ecu_timer_handler() maps an ECU after __j1939_rx_release() has already
unmapped all ECUs. It drops the ECU reference in j1939_ecu_map_locked() and
rejects ECU creation in j1939_ecu_create_locked() if the net_device is no
longer registered or priv->rx_kref is zero.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..77b6788060573 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -84,6 +84,15 @@ static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
>  	if (!j1939_ecu_get(ecu))
>  		return;
>  
> +	/* Drop the ref 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)) {
> +		j1939_ecu_put(ecu);
> +		return;
> +	}

[Severity: High]
Because ecu->ac_timer is not canceled during __j1939_rx_release() ->
j1939_ecu_unmap_all(), can this cause a use-after-free if the can-j1939
module is unloaded while ecu->ac_timer is still armed?

When an Address Claim frame is processed in j1939_ac_process(),
j1939_ecu_timer_start() arms ecu->ac_timer for 250 ms while holding a
reference on ecu. When the last J1939 socket is closed via
j1939_sk_release() -> j1939_netdev_stop() -> __j1939_rx_release(),
ecu->ac_timer remains active and holds its reference on ecu, so
__j1939_ecu_release() is not yet called and ecu->release_work is not yet
queued on j1939_ecu_wq:

net/can/j1939/bus.c:__j1939_ecu_release() {
	...
	INIT_WORK(&ecu->release_work, __j1939_ecu_release_work);
	queue_work(j1939_ecu_wq, &ecu->release_work);
}

Neither struct j1939_ecu nor struct j1939_priv holds a module reference on
can-j1939; only open sockets hold a reference on j1939_proto.owner. If the
module is unloaded during the 250 ms window before ecu->ac_timer expires,
j1939_module_exit() destroys j1939_ecu_wq immediately because no work is
queued on it yet:

net/can/j1939/main.c:j1939_module_exit() {
	can_proto_unregister(&j1939_can_proto);

	unregister_netdevice_notifier(&j1939_netdev_notifier);

	destroy_workqueue(j1939_ecu_wq);
}

When ecu->ac_timer fires afterward, wouldn't j1939_ecu_timer_handler()
execute unmapped module text or queue ecu->release_work onto the destroyed
j1939_ecu_wq?

[ ... ]

> @@ -179,6 +188,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);

[Severity: High]
Does placing this check in j1939_ecu_create_locked() rather than in
j1939_local_ecu_get() still allow j1939_sk_bind() to succeed on an
unregistering net_device and leak references on struct j1939_priv, struct
j1939_ecu, and struct net_device?

In j1939_sk_bind(), the initial check of ndev->reg_state runs without
rtnl_lock before jsk is added to priv->j1939_socks (and is skipped in the
re-bind branch after j1939_jsk_del() removes jsk from priv->j1939_socks). If
unregister_netdevice() runs j1939_netdev_notify(NETDEV_UNREGISTER) ->
j1939_sk_netdev_event_unregister() before j1939_sk_bind() reaches
j1939_jsk_add(), j1939_sk_netdev_event_unregister() will not see jsk in
priv->j1939_socks.

When j1939_sk_bind() then calls j1939_local_ecu_get(),
j1939_ecu_create_locked() is bypassed whenever name == 0 or when
j1939_ecu_get_by_name_locked() finds an existing ECU in priv->ecus (such as
an ECU kept alive for 250 ms by an uncancelled ecu->ac_timer):

net/can/j1939/bus.c:j1939_local_ecu_get() {
	...
	if (!name)
		goto done;

	ecu = j1939_ecu_get_by_name_locked(priv, name);
	if (!ecu)
		ecu = j1939_ecu_create_locked(priv, name);
	...
}

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_netdev_stop(priv);
		jsk->priv = NULL;
		synchronize_rcu();
		j1939_priv_put(priv);
		goto out_release_sock;
	}

	j1939_jsk_add(priv, jsk);
	...
}

In those cases, j1939_local_ecu_get() returns 0 and j1939_jsk_add() adds jsk
after j1939_sk_netdev_event_unregister() has already finished. Should the
reg_state and rx_kref check be placed in j1939_local_ecu_get() under
priv->lock instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/3d7e48fa-9af5-43e2-84f7-884e61f9d179@I-love.SAKURA.ne.jp?part=2

Re: [PATCH v8 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[Tetsuo Handa]

On 2026/11/05 21:24, sashiko-bot@kernel.org wrote:
> [Severity: High]
> Because ecu->ac_timer is not canceled during __j1939_rx_release() ->
> j1939_ecu_unmap_all(), can this cause a use-after-free if the can-j1939
> module is unloaded while ecu->ac_timer is still armed?
> 
> When an Address Claim frame is processed in j1939_ac_process(),
> j1939_ecu_timer_start() arms ecu->ac_timer for 250 ms while holding a
> reference on ecu. When the last J1939 socket is closed via
> j1939_sk_release() -> j1939_netdev_stop() -> __j1939_rx_release(),
> ecu->ac_timer remains active and holds its reference on ecu, so
> __j1939_ecu_release() is not yet called and ecu->release_work is not yet
> queued on j1939_ecu_wq:
> 
> net/can/j1939/bus.c:__j1939_ecu_release() {
> 	...
> 	INIT_WORK(&ecu->release_work, __j1939_ecu_release_work);
> 	queue_work(j1939_ecu_wq, &ecu->release_work);
> }
> 
> Neither struct j1939_ecu nor struct j1939_priv holds a module reference on
> can-j1939; only open sockets hold a reference on j1939_proto.owner. If the
> module is unloaded during the 250 ms window before ecu->ac_timer expires,
> j1939_module_exit() destroys j1939_ecu_wq immediately because no work is
> queued on it yet:
> 
> net/can/j1939/main.c:j1939_module_exit() {
> 	can_proto_unregister(&j1939_can_proto);
> 
> 	unregister_netdevice_notifier(&j1939_netdev_notifier);
> 
> 	destroy_workqueue(j1939_ecu_wq);
> }
> 
> When ecu->ac_timer fires afterward, wouldn't j1939_ecu_timer_handler()
> execute unmapped module text or queue ecu->release_work onto the destroyed
> j1939_ecu_wq?

I tried "rmmod can_j1939" while the kernel is unable to make progress due to
"kernel:unregister_netdevice: waiting for vcan0 to become free. Usage count = 11351".

The "rmmod can_j1939" operation is blocked at NETDEV_UNREGISTER event until
the vcan0's refcount drops to 1. Since j1939_ecu and j1939_priv are holding
a ref to the vcan0, module unload operation won't complete until all j1939_ecu
and j1939_priv are released. Therefore, I suspect that "[High] can: j1939:
use-after-free in j1939_ecu_timer_handler() on module exit" is a false-positive.

 INFO: task rmmod:10306 blocked for more than 122 seconds.
       Not tainted 7.3.0 #36
 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
 task:rmmod           state:D stack:0     pid:10306 tgid:10306 ppid:934    task_flags:0x400100 flags:0x00080000
 Call Trace:
  <TASK>
  __schedule+0x809/0x1c70
  schedule+0xb2/0x100
  schedule_preempt_disabled+0x16/0x30
  rwsem_down_write_slowpath+0x2f7/0x5f0
  down_write+0x8e/0xb0
  unregister_netdevice_notifier+0x26/0xe0
  __se_sys_delete_module+0x22d/0x350
  do_syscall_64+0x126/0x480
  entry_SYSCALL_64_after_hwframe+0x76/0x7e
 RIP: 0033:0x7f5f867993eb
 RSP: 002b:00007ffe0ffe8318 EFLAGS: 00000202 ORIG_RAX: 00000000000000b0
 RAX: ffffffffffffffda RBX: 000055d36cfbb4b0 RCX: 00007f5f867993eb
 RDX: 0000000000000000 RSI: 0000000000000800 RDI: 000055d36cfbb510
 RBP: 00007ffe0ffe8340 R08: 0000000000000000 R09: 0000000000000000
 R10: 0000000000000000 R11: 0000000000000202 R12: 0000000000000000
 R13: 000055d36cfbb4b0 R14: 00007ffe0ffe8590 R15: 0000000000000000
  </TASK>
 INFO: task rmmod:10306 <writer> blocked on an rw-semaphore likely owned by task kworker/u512:0:12 <reader>
 task:kworker/u512:0  state:D stack:0     pid:12    tgid:12    ppid:2      task_flags:0x4208060 flags:0x00080000
 Workqueue: netns cleanup_net
 Call Trace:
  <TASK>
  __schedule+0x809/0x1c70
  schedule+0xb2/0x100
  schedule_timeout+0x100/0x180
  msleep+0x3e/0x80
  netdev_run_todo+0x36f/0x6b0
  default_device_exit_batch+0x2d7/0x330
  ops_undo_list+0x202/0x450
  cleanup_net+0x2bc/0x4b0
  process_scheduled_works+0x44d/0x7d0
  worker_thread+0x2f5/0x4a0
  kthread+0x140/0x1a0
  ret_from_fork+0x203/0x490
  ret_from_fork_asm+0x1a/0x30

Re: [PATCH v8 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[Tetsuo Handa]

On 2026/11/05 23:42, Tetsuo Handa wrote:
> I tried "rmmod can_j1939" while the kernel is unable to make progress due to
> "kernel:unregister_netdevice: waiting for vcan0 to become free. Usage count = 11351".
> 
> The "rmmod can_j1939" operation is blocked at NETDEV_UNREGISTER event until
> the vcan0's refcount drops to 1. Since j1939_ecu and j1939_priv are holding
> a ref to the vcan0, module unload operation won't complete until all j1939_ecu
> and j1939_priv are released. Therefore, I suspect that "[High] can: j1939:
> use-after-free in j1939_ecu_timer_handler() on module exit" is a false-positive.
Well, this is not a false-positive when teardown of a network namespace is not involved.

Re: [PATCH v8 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] j1939_module_exit() (net/can/j1939/main.c:430-440) calls
  destroy_workqueue(j1939_ecu_wq) without making sure no ECU reference…
- [Low] This patch fixes a real, long-standing bug, but its commit
  message has no Fixes: tag and no Cc: stable.
- [Low] The commit message only mentions a "possibility of list
  corruption".

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  deadlock.

--

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

This commit moves the final release of struct j1939_ecu (list_del(),
netdev_put(), kfree() and j1939_priv_put()) to a new j1939_ecu_wq
workqueue. That way the list removal always runs under
write_lock_bh(&priv->lock).

ECUs with a zero refcount can now still be found on priv->ecus and in
priv->ents[]. To handle this, j1939_ecu_get() becomes
kref_get_unless_zero(), and lookups skip ECUs whose kref has dropped to
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]
Is "list corruption" the whole story? Before this patch,
__j1939_ecu_release() did this with no lock held:

	list_del(&ecu->list);
	netdev_put(priv->ndev, &ecu->priv_dev_tracker);
	kfree(ecu);
	j1939_priv_put(priv);

At the same time, other CPUs can walk priv->ecus under read_lock_bh().
Two examples are j1939_ecu_find_by_name_locked() (called through
j1939_name_to_addr() on the TX path) and j1939_ecu_get_by_name_locked().
That looks like a use-after-free of struct j1939_ecu, not just corrupted
list pointers.

Could the message also name the callers that drop references without the
lock, j1939_ac_recv() and j1939_ac_fixup()? It could also say that the
netdev tracker reference and the j1939_priv reference are now released
later from j1939_ecu_wq, not synchronously.

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

[Severity: Low]
This fixes a long-standing memory-safety issue. Should it have a Fixes:
tag and Cc: stable?

The old lockless last-put path looks like this:

CPU A
j1939_ac_recv()
  ecu = j1939_ecu_get_by_addr(priv, skcb->addr.sa);
  ...
                                 CPU B
                                 NETDEV_DOWN
                                   j1939_ecu_unmap_all()
                                     j1939_ecu_unmap_locked()
                                       j1939_ecu_put()  <- mapping ref
  j1939_ecu_put(ecu);            <- last ref, list_del()/kfree() unlocked

The lockless callers, j1939_ac_recv() on RX and j1939_ac_fixup() on TX,
look like they go back to the commit that added the protocol:

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

The next patch in the series ("can: j1939: fix "struct j1939_ecu"
refcount leak in j1939_ecu_map_locked()") has a Fixes: tag. It also
depends on the kref_get_unless_zero() semantics added here. Since this
patch has no tag, stable backporters have no hint that they need it as a
prerequisite.

> 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
j1939_ecu_timer_cancel() deadlock against j1939_ecu_timer_handler()?

j1939_ac_process() takes write_lock_bh(&priv->lock) and cancels the timer
while still holding it. The soft hrtimer callback takes the same lock
before doing anything else:

CPU A (CAN RX softirq)
j1939_ac_recv()
  j1939_ac_process()
    write_lock_bh(&priv->lock);
    ...
    j1939_ecu_timer_cancel(ecu)
      hrtimer_cancel(&ecu->ac_timer)
        hrtimer_try_to_cancel()        <- returns -1, callback running
        hrtimer_cancel_wait_running()  <- loops

CPU B (hrtimer softirq)
j1939_ecu_timer_handler()
  write_lock_bh(&priv->lock);          <- spins, CPU A holds it

On !PREEMPT_RT, hrtimer_cancel_wait_running() is only cpu_relax(), so
both CPUs spin forever in softirq context. On PREEMPT_RT it also
deadlocks, through softirq_expiry_lock and the sleeping rwlock.

The trigger is an address-claim frame for an ECU whose ac_timer is
expiring on another CPU. The frame must carry a different SA, so the
early goto out_ecu_put is not taken. Remote CAN peers can send such
frames.

The note below the --- line calls this "can: j1939: ABBA deadlock in
j1939_ecu_timer_cancel() under priv->lock". The next patch in the series
also says "we can't safely call hrtimer_cancel() with priv->lock held".
Neither patch fixes it yet.

[ ... ]

> 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]
Can a pending ecu->ac_timer still queue release work after this
destroy_workqueue() call?

destroy_workqueue() drains work that is already queued. It does not stop
new work from being queued. The ac_timer holds an ECU reference taken in
j1939_ecu_timer_start(), and nothing on the teardown path cancels the
timer:

- __j1939_rx_release() only calls j1939_ecu_unmap_all().
- The NETDEV_DOWN and NETDEV_UNREGISTER handlers don't cancel it either.

A possible sequence:

1. An address-claim frame arrives, and j1939_ac_process() ->
   j1939_ecu_timer_start() arms the 250 ms soft hrtimer.
2. The last j1939 socket is closed. j1939_netdev_stop() runs, and the
   module reference held through the proto owner is dropped.
3. rmmod runs j1939_module_exit() -> destroy_workqueue(j1939_ecu_wq).
4. The timer fires:

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

If the timer fires during the drain, __queue_work() rejects the work with
a WARN. The ECU, its netdev tracker reference and its j1939_priv
reference then leak, and netdev unregister can hang waiting for the
device to become free.

If the timer fires after destroy_workqueue() returns, queue_work()
accesses the freed workqueue.

The root cause, a pending ac_timer that outlives module exit, predates
this patch. But this patch adds the dependency on the global workqueue,
and the new comment promises more than destroy_workqueue() alone can
guarantee.

The next patch in the series also says "For now we don't cancel
ecu->ac_timer from j1939_ecu_unmap_all()". Do the ECU hrtimers need to be
cancelled before the workqueue is destroyed?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/3d7e48fa-9af5-43e2-84f7-884e61f9d179%40I-love.SAKURA.ne.jp

Re: [PATCH v8 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[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] Undisclosed backport dependency.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a two-
  CPU deadlock.

--

can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

This commit makes j1939_ecu_map_locked() drop the ECU reference it just
took, and makes j1939_ecu_create_locked() fail with -ENODEV, when the
netdev is no longer registered or __j1939_rx_release() has already run.
This keeps j1939_ecu_timer_handler() from re-mapping an ECU after
j1939_ecu_unmap_all() has finished.

> For now we don't cancel ecu->ac_timer from j1939_ecu_unmap_all(), for
> we can't safely call hrtimer_cancel() with priv->lock held. However,
> since ecu->ac_timer expires in 250ms, for now let's tolerate delaying
> completion of NETDEV_UNREGISTER event for less than one second.

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Still,
the rule given here (hrtimer_cancel() is not safe with priv->lock held)
already seems to be broken in j1939_ac_process() in
net/can/j1939/address-claim.c.

j1939_ac_process() takes priv->lock for write. While still holding it, it
calls j1939_ecu_timer_cancel(), which calls hrtimer_cancel() on
ecu->ac_timer:

CPU0 (CAN RX softirq)
j1939_can_recv()->j1939_ac_recv()->j1939_ac_process()
    write_lock_bh(&priv->lock);
    ...
    /* cancel pending (previous) address claim */
    j1939_ecu_timer_cancel(ecu);
        hrtimer_cancel(&ecu->ac_timer);

The timer callback takes the same lock:

CPU1 (hrtimer softirq)
__run_hrtimer()->j1939_ecu_timer_handler()
    write_lock_bh(&priv->lock);

Say ac_timer expires on CPU1, and j1939_ecu_timer_handler() is already
spinning on priv->lock. At that moment CPU0 gets an ADDRESS_CLAIMED frame
with the same NAME but a different SA.

hrtimer_try_to_cancel() returns -1 while the callback is running, so
hrtimer_cancel() keeps retrying in hrtimer_cancel_wait_running(). On
!PREEMPT_RT that is cpu_relax(). On PREEMPT_RT it blocks on the softirq
expiry lock while the rwlock is held.

Can this leave both CPUs spinning forever with BH disabled? The trigger is
ADDRESS_CLAIMED frames, which any node on the bus or a local vcan sender can
send.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..77b6788060573 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -84,6 +84,15 @@ static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
>  	if (!j1939_ecu_get(ecu))
>  		return;
>  
> +	/* Drop the ref 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)) {
> +		j1939_ecu_put(ecu);
> +		return;
> +	}
> +

[Severity: Low]
This isn't a bug, but is there an undisclosed backport dependency here?

The context line "if (!j1939_ecu_get(ecu)) return;" comes from the earlier
patch "can: j1939: fix possibility of list corruption in
__j1939_ecu_release()". That patch turned j1939_ecu_get() into a
__must_check bool wrapper around kref_get_unless_zero(). It also moved the
call ahead of the ent->ecu check.

In the trees covered by the Fixes: tag, the code still looks like this:

static void j1939_ecu_get(struct j1939_ecu *ecu)
{
	kref_get(&ecu->kref);
}

and j1939_ecu_map_locked() calls it after the ent->ecu check. So this hunk
won't apply as-is.

The prerequisite has no Fixes: or Cc: stable tag, and this commit doesn't
mention it. Since this patch carries "Fixes: 9d71dd0c7009", stable tooling
will try to pick it up on its own.

Could the prerequisite get a Fixes:/Cc: stable tag, or could this commit
note the dependency? Either way, backporters would know whether to take
both patches or adapt this one by hand.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/3d7e48fa-9af5-43e2-84f7-884e61f9d179%40I-love.SAKURA.ne.jp