Re: [PATCH can v2] can: j1939: avoid address-claim timer deadlock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless calls to `j1939_ecu_put()` can drop the last reference to an ECU, triggering `__j1939_ecu_release()` and modifying `priv->ecus` locklessly.
- [High] Permanent memory leak of `j1939_ecu` and `j1939_priv` when the network interface is brought down while an address claim timer is pending.
--

--- Patch [1]: [PATCH can v2] can: j1939: avoid address-claim timer deadlock ---
commit 08d11140bccb342c8267ab26aee9e24877e324a3
Author: Felix Hoffmann <f3lix.dev@gmx.de>

can: j1939: avoid address-claim timer deadlock

j1939_ac_process() holds priv->lock while synchronously canceling an
ECU's address-claim hrtimer. The timer callback takes the same lock. If the
callback starts on another CPU after the receive path takes the lock, the
callback waits for priv->lock while hrtimer_cancel() waits for the callback
to finish. This deadlocks both CPUs and makes the system unresponsive.

Cc: stable@vger.kernel.org
Signed-off-by: Felix Hoffmann <f3lix.dev@gmx.de>

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..eb9a3fd847f42 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -10,10 +10,19 @@
>   * Since rtnetlink, no real bus is used.
>   */
>  
> +#include <linux/net.h>
> +
>  #include <net/sock.h>
>  
>  #include "j1939-priv.h"
>  
> +#define J1939_ECU_TIMER_RETRY_DELAY_MS 1
> +/*
> + * J1939 does not specify a lock-contention retry count. Five retries are a
> + * pragmatic threshold for warning about an unusual address-claim delay.
> + */
> +#define J1939_ECU_TIMER_RETRY_WARN 5
> +
>  static void __j1939_ecu_release(struct kref *kref)
>  {
>  	struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
>  	struct j1939_priv *priv = ecu->priv;
>  
>  	list_del(&ecu->list);

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to list
corruption?

If an ECU is concurrently unmapped (dropping its primary reference) while
another thread holds a temporary reference (e.g., in j1939_ac_recv() or
j1939_ac_fixup()), dropping the temporary reference locklessly via
j1939_ecu_put() will reduce the refcount to 0.

This triggers __j1939_ecu_release(), which performs list_del(&ecu->list) on
priv->ecus without holding priv->lock.

Can this be triggered remotely by sending precisely timed address claim and
release packets to cause a use-after-free or panic?

[ ... ]

> @@ -109,6 +118,8 @@ void j1939_ecu_unmap_all(struct j1939_priv *priv)
>  
>  void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  {

[Severity: High]
This is a pre-existing issue, but does this leak j1939_ecu and j1939_priv when
the network interface is brought down?

When a J1939 interface is unregistered, j1939_ecu_unmap_all() clears mapped
ECUs from priv->ents, but it only iterates over mapped ECUs and ignores ECUs
in priv->ecus that have a pending timer:

net/can/j1939/bus.c:j1939_ecu_unmap_all() {
        for (i = 0; i < ARRAY_SIZE(priv->ents); i++)
                if (priv->ents[i].ecu)
                        j1939_ecu_unmap_locked(priv->ents[i].ecu);
}

It seems this fails to cancel pending ac_timer hrtimers for ECUs in the
middle of address claiming.

When the uncancelled timer subsequently fires, j1939_ecu_timer_handler()
executes and maps the ECU back into the now-abandoned j1939_priv.

Since interface teardown (j1939_netdev_notify()) won't run
j1939_ecu_unmap_all() again, does this cause a permanent leak blocking
unregister_netdevice()?

> +	ecu->ac_timer_retries = 0;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803101503.21023-1-f3lix.dev@gmx.de?part=1