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

[sashiko-bot@kernel.org]

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