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

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` lock=
lessly.
- [High] Permanent memory leak of `j1939_ecu` and `j1939_priv` when the net=
work interface is brought down while an address claim timer is pending.
--

--- Patch [1]: [PATCH can v2] can: j1939: avoid address-claim timer deadloc=
k ---
commit 08d11140bccb342c8267ab26aee9e24877e324a3
Author: Felix Hoffmann <[email protected]>

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: [email protected]
Signed-off-by: Felix Hoffmann <[email protected]>

> 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.
>   */
> =20
> +#include <linux/net.h>
> +
>  #include <net/sock.h>
> =20
>  #include "j1939-priv.h"
> =20
> +#define J1939_ECU_TIMER_RETRY_DELAY_MS 1
> +/*
> + * J1939 does not specify a lock-contention retry count. Five retries ar=
e 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 =3D container_of(kref, struct j1939_ecu, kref);
>  	struct j1939_priv *priv =3D ecu->priv;
> =20
>  	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)
> =20
>  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 w=
hen
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 =3D 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 =3D 0;
> +

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803101503.2102=
[email protected]?part=3D1