[PATCH v3 1/2] can: j1939: make hrtimer callback lockless and free ECUs asyncronously

[PATCH v3 1/2] can: j1939: make hrtimer callback lockless and free ECUs asyncronously

[Dmitry Antipov]

In j1939_ecu_map_locked(), it is not needed to lock 'struct j1939_priv'
just to claim ECU slot. This may be accomplished by using cmpxchg() on
ECU slot pointer. This way, convert j1939_ecu_map_locked() to
j1939_ecu_map_atomic() and make j1939_ecu_timer_handler() lockless.

Now, if hrtimer handler is lockless, the following calls from multiple
handlers may trigger concurrent modifications of ECU list:

-> j1939_ecu_timer_handler()
   -> j1939_ecu_put()
      -> __j1939_ecu_release()
         -> list_del()

To avoid this, offload actual ECU removal to schedule_work(). Since
the work handler runs in process context, this is a way better place
to perform an operation which requires 'struct j1939_priv' locking
(and suits PREEMPT_RT better as well).

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: initial version to join the series
---
 net/can/j1939/bus.c        | 27 ++++++++++++++++-----------
 net/can/j1939/j1939-priv.h |  1 +
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..a347594b8c1c 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -14,17 +14,27 @@
 
 #include "j1939-priv.h"
 
-static void __j1939_ecu_release(struct kref *kref)
+static void __j1939_ecu_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, work);
 	struct j1939_priv *priv = ecu->priv;
 
+	write_lock(&priv->lock);
 	list_del(&ecu->list);
 	netdev_put(priv->ndev, &ecu->priv_dev_tracker);
 	kfree(ecu);
+	write_unlock(&priv->lock);
+
 	j1939_priv_put(priv);
 }
 
+static void __j1939_ecu_release(struct kref *kref)
+{
+	struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
+
+	schedule_work(&ecu->work);
+}
+
 void j1939_ecu_put(struct j1939_ecu *ecu)
 {
 	kref_put(&ecu->kref, __j1939_ecu_release);
@@ -46,26 +56,23 @@ static bool j1939_ecu_is_mapped_locked(struct j1939_ecu *ecu)
 
 /* ECU device interface */
 /* map ECU to a bus address space */
-static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
+static void j1939_ecu_map_atomic(struct j1939_ecu *ecu)
 {
 	struct j1939_priv *priv = ecu->priv;
 	struct j1939_addr_ent *ent;
 
-	lockdep_assert_held(&priv->lock);
-
 	if (!j1939_address_is_unicast(ecu->addr))
 		return;
 
 	ent = &priv->ents[ecu->addr];
 
-	if (ent->ecu) {
+	if (cmpxchg(&ent->ecu, NULL, ecu)) {
 		netdev_warn(priv->ndev, "Trying to map already mapped ECU, addr: 0x%02x, name: 0x%016llx. Skip it.\n",
 			    ecu->addr, ecu->name);
 		return;
 	}
 
 	j1939_ecu_get(ecu);
-	ent->ecu = ecu;
 	ent->nusers += ecu->nusers;
 }
 
@@ -129,19 +136,16 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
 {
 	struct j1939_ecu *ecu =
 		container_of(hrtimer, struct j1939_ecu, ac_timer);
-	struct j1939_priv *priv = ecu->priv;
 
-	write_lock_bh(&priv->lock);
 	/* TODO: can we test if ecu->addr is unicast before starting
 	 * the timer?
 	 */
-	j1939_ecu_map_locked(ecu);
+	j1939_ecu_map_atomic(ecu);
 
 	/* The corresponding j1939_ecu_get() is in
 	 * j1939_ecu_timer_start().
 	 */
 	j1939_ecu_put(ecu);
-	write_unlock_bh(&priv->lock);
 
 	return HRTIMER_NORESTART;
 }
@@ -156,6 +160,7 @@ struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name)
 	if (!ecu)
 		return ERR_PTR(-ENOMEM);
 	kref_init(&ecu->kref);
+	INIT_WORK(&ecu->work, __j1939_ecu_work);
 	netdev_hold(priv->ndev, &ecu->priv_dev_tracker, gfp_any());
 	ecu->addr = J1939_IDLE_ADDR;
 	ecu->name = name;
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..080d5b4c8c23 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -37,6 +37,7 @@ struct j1939_ecu {
 	/* indicates that this ecu successfully claimed @sa as its address */
 	struct hrtimer ac_timer;
 	struct kref kref;
+	struct work_struct work;
 	struct j1939_priv *priv;
 	netdevice_tracker priv_dev_tracker;
 
-- 
2.55.0

[PATCH v3 2/2] can: j1939: cancel all pending ECUs on device stop

[Dmitry Antipov]

Cancel all pending ECUs in j1939_netdev_stop(). This is needed
to prevent the case when stopped device no longer processes ECUs
and, since ECU holds the reference to 'struct j1939_priv', the
latter (and ECU itself) is never freed.

Reported-by: syzbot+489e907b2a026a6f5fa0@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=489e907b2a026a6f5fa0
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: rebase on top of lockless hrtimer handler
    and asynchronous ECU freeing
v2: add extra precaution to avoid ABBA deadlock between
    j1939_ecu_cancel_all() and timer callbacks (Sashiko)
---
 net/can/j1939/bus.c        | 10 ++++++++++
 net/can/j1939/j1939-priv.h |  1 +
 net/can/j1939/main.c       |  1 +
 3 files changed, 12 insertions(+)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index a347594b8c1c..b021a77a0898 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -132,6 +132,16 @@ void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
 		j1939_ecu_put(ecu);
 }
 
+void j1939_ecu_cancel_all(struct j1939_priv *priv)
+{
+	struct j1939_ecu *ecu, *tmp;
+
+	write_lock_bh(&priv->lock);
+	list_for_each_entry_safe(ecu, tmp, &priv->ecus, list)
+		j1939_ecu_timer_cancel(ecu);
+	write_unlock_bh(&priv->lock);
+}
+
 static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
 {
 	struct j1939_ecu *ecu =
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index 080d5b4c8c23..311d89f9b473 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -205,6 +205,7 @@ struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name);
 
 void j1939_ecu_timer_start(struct j1939_ecu *ecu);
 void j1939_ecu_timer_cancel(struct j1939_ecu *ecu);
+void j1939_ecu_cancel_all(struct j1939_priv *priv);
 void j1939_ecu_unmap_all(struct j1939_priv *priv);
 
 struct j1939_priv *j1939_netdev_start(struct net_device *ndev);
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 5e5e6c228f22..1f68ba803700 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -306,6 +306,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
 
 void j1939_netdev_stop(struct j1939_priv *priv)
 {
+	j1939_ecu_cancel_all(priv);
 	kref_put_mutex(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock);
 	j1939_priv_put(priv);
 }
-- 
2.55.0

Re: [PATCH v3 2/2] can: j1939: cancel all pending ECUs on device stop

[sashiko-bot]

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

New issues:
- [High] Unconditional cancellation of ECU timers in j1939_netdev_stop() breaks address claiming for all sockets.
- [High] Race condition allows ECU and priv memory leak because timers are cancelled before the RX path is disabled.

Pre-existing issues:
- [High] Deadlock in __j1939_ecu_work due to acquiring write_lock(&priv->lock) without disabling softirqs.
- [High] Module unload while asynchronous ECU workqueue items are still pending causes an Execution-After-Free panic.
--

commit 5eb4b9f58ccbd7c9b7e110c01dfe98b7a95acee8
Author: Dmitry Antipov <dmantipov@yandex.ru>

can: j1939: cancel all pending ECUs on device stop

This commit cancels all pending ECUs in j1939_netdev_stop() to prevent
a situation where a stopped device no longer processes ECUs. This
ensures the ECU and its reference to the j1939_priv structure are
properly freed.

Link: https://syzkaller.appspot.com/bug?extid=489e907b2a026a6f5fa0

[ ... ]

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index 5e5e6c228f225..1f68ba8037004 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
> @@ -306,6 +306,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
>  
>  void j1939_netdev_stop(struct j1939_priv *priv)
>  {
> +	j1939_ecu_cancel_all(priv);
>  	kref_put_mutex(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock);

[Severity: High]
Does unconditionally cancelling all ECU timers here break address claiming
for all sockets?

When any single socket closes, it invokes j1939_netdev_stop(), which
unconditionally cancels all ECU timers for the shared priv structure.

Does this forcefully abort pending address claim timers for every other
active socket on the interface, leaving those ECUs unmapped permanently?

Should the timer cancellation be properly scoped to the destruction of the
last socket in the __j1939_rx_release callback instead?


[Severity: High]
Can this sequence race and cause an ECU memory leak?

If a socket is closed, initiating j1939_netdev_stop(), j1939_ecu_cancel_all()
runs. If a new AC frame is received on the bus before __j1939_rx_release()
disables CAN RX, it triggers j1939_ac_process() which starts a new ECU timer.

Since j1939_ecu_unmap_all() already ran synchronously during teardown, the
new ECU timer will fire later, taking a reference and leaving the new ECU
permanently unmapped. 

Does this permanently leak both the ECU and its referenced j1939_priv?

>  	j1939_priv_put(priv);
>  }

[ ... ]


This is a pre-existing issue, but I noticed a potential deadlock here:

    net/can/j1939/bus.c:__j1939_ecu_work() {
        ...
        write_lock(&priv->lock);
        list_del(&ecu->list);
        ...
    }

[Severity: High]
Does acquiring write_lock(&priv->lock) without disabling softirqs cause a
deadlock?

Because write_lock() disables preemption but not softirqs, the workqueue
thread can be preempted by the CAN RX softirq on the same CPU. 

If a CAN RX interrupt arrives and triggers j1939_ac_process() in softirq
context, it will spin on write_lock_bh(&priv->lock), resulting in a lock
inversion deadlock. 

Should this use write_lock_bh() instead?


This is a pre-existing issue, but I also noticed a missing synchronization
here:

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

[Severity: High]
Can unloading the module while asynchronous ECU workqueue items are still
pending cause an Execution-After-Free panic?

When a socket closes, __j1939_ecu_release defers freeing the ECU via
schedule_work(&ecu->work). 

Because j1939_module_exit() lacks a flush_scheduled_work() or similar
synchronization call, the module memory can be freed before the workqueue
executes __j1939_ecu_work. 

Does this crash the kernel when the worker thread branches to the unmapped
function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828064621.91364-1-dmantipov@yandex.ru?part=2

Re: [PATCH v3 1/2] can: j1939: make hrtimer callback lockless and free ECUs asyncronously

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing bottom-half disable in __j1939_ecu_work() causes a deadlock with softirq RX handlers.
- [High] Lockless read-modify-write of ent->nusers in j1939_ecu_map_atomic() causes lost updates and accounting corruption.
- [High] Asynchronous ECU removal work is not flushed on module unload, causing use-after-free panics.
- [High] Publish-before-initialize TOCTOU race in j1939_ecu_map_atomic() exposes the ECU pointer before taking a reference.
- [High] Delayed removal from the ECU list causes concurrent list traversals to encounter an ECU with a zero reference count.
--

commit ff05e907d647c94fe0051f49164f837f1fa99aa5
Author: Dmitry Antipov <dmantipov@yandex.ru>

can: j1939: make hrtimer callback lockless and free ECUs asyncronously

This patch removes the lock from j1939_ecu_timer_handler() and uses cmpxchg() 
to map the device. It also defers removal to a workqueue to avoid concurrent 
modification of the list.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..a347594b8c1c5 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -14,17 +14,27 @@
>  
>  #include "j1939-priv.h"
>  
> -static void __j1939_ecu_release(struct kref *kref)
> +static void __j1939_ecu_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, work);
>  	struct j1939_priv *priv = ecu->priv;
>  
> +	write_lock(&priv->lock);

[Severity: High]
Does this code expose the lock to deadlocks with softirqs?

Because __j1939_ecu_work() runs in process context and uses write_lock(), a
network softirq could interrupt this and try to acquire the same priv->lock via
read_lock_bh() or write_lock_bh() during frame reception.

Should this use write_lock_bh() instead?

>  	list_del(&ecu->list);
>  	netdev_put(priv->ndev, &ecu->priv_dev_tracker);
>  	kfree(ecu);
> +	write_unlock(&priv->lock);
> +
>  	j1939_priv_put(priv);
>  }
>  
> +static void __j1939_ecu_release(struct kref *kref)
> +{
> +	struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
> +
> +	schedule_work(&ecu->work);

[Severity: High]
Does scheduling the removal work leave the object on the priv->ecus list with a
zero reference count?

Concurrent network paths traversing the list under read_lock_bh() might
find this zero-refcount object and call j1939_ecu_get(), triggering a
kref_get() on a zero refcount and causing refcount saturation.

Should the object be removed from the list synchronously before scheduling the
work?

[Severity: High]
Does this deferral create a use-after-free regression during module unload?

If the last socket is closed, dropping references and scheduling this
work, and the module is immediately unloaded via rmmod, the workqueue might 
execute __j1939_ecu_work() after the module's memory is freed.

Is there a missing flush of the scheduled work in the j1939_module_exit() path?

> +}
> +
>  void j1939_ecu_put(struct j1939_ecu *ecu)

[ ... ]

> @@ -46,26 +56,23 @@ static bool j1939_ecu_is_mapped_locked(struct j1939_ecu *ecu)
>  
>  /* ECU device interface */
>  /* map ECU to a bus address space */
> -static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
> +static void j1939_ecu_map_atomic(struct j1939_ecu *ecu)
>  {
>  	struct j1939_priv *priv = ecu->priv;
>  	struct j1939_addr_ent *ent;
>  
> -	lockdep_assert_held(&priv->lock);
> -
>  	if (!j1939_address_is_unicast(ecu->addr))
>  		return;
>  
>  	ent = &priv->ents[ecu->addr];
>  
> -	if (ent->ecu) {
> +	if (cmpxchg(&ent->ecu, NULL, ecu)) {

[Severity: High]
Does this cmpxchg expose the pointer to concurrent threads before its
reference count is incremented?

If another thread concurrently unmaps the object and calls j1939_ecu_put(), it
could drop a reference that this mapping thread has not yet acquired. If the
refcount drops to zero, the object would be queued for deletion, and the
subsequent j1939_ecu_get() here would increment a zero refcount.

Should j1939_ecu_get() be called before the cmpxchg?

>  		netdev_warn(priv->ndev, "Trying to map already mapped ECU, addr: 0x%02x, name: 0x%016llx. Skip it.\n",
>  			    ecu->addr, ecu->name);
>  		return;
>  	}
>  
>  	j1939_ecu_get(ecu);
> -	ent->ecu = ecu;
>  	ent->nusers += ecu->nusers;

[Severity: High]
Does this unprotected arithmetic race with other threads?

Since the update to ent->nusers is no longer protected by priv->lock,
concurrent threads could perform read-modify-write operations on ent->nusers
simultaneously, causing lost updates to the user accounting.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828064621.91364-1-dmantipov@yandex.ru?part=1