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

[Oleksij Rempel <o.rempel@pengutronix.de>]

Hi Felix,

Please do not send CAN related patches to the netdev list, and please
use [PATCH can] tag instead.

On Fri, Jul 31, 2026 at 03:42:55PM +0200, Felix Hoffmann wrote:
> 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.
> 
> Do not wait for priv->lock from the soft hrtimer callback. If
> address-claim processing currently owns it, move the expiry forward by 1 ms
> and restart the timer. This lets a concurrent hrtimer_cancel() finish and
> remove the requeued timer. Without a cancellation, mapping is retried
> shortly.
> 
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:GPT5.6-Sol
> Signed-off-by: Felix Hoffmann <f3lix.dev@gmx.de>
> ---
> 
> The deadlock was reproduced three times on a 2-vCPU kernel with KASAN
> and lockdep. Cross-CPU GDB stacks showed hrtimer_cancel() and the timer
> callback waiting on the same j1939_priv lock and hrtimer.
> 
> With this change, the reproducer completed 420 stress rounds, and the
> patched CAN J1939 syzkaller campaign remained operational. The trigger
> drops to UID and GID 65534 before opening its CAN sockets. A minimal
> reproducer and the complete stack capture are available privately on
> request.
> 
>  net/can/j1939/bus.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a71937..ac654dc8872e 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -131,7 +131,17 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
>  		container_of(hrtimer, struct j1939_ecu, ac_timer);
>  	struct j1939_priv *priv = ecu->priv;
>  
> -	write_lock_bh(&priv->lock);
> +	/*
> +	 * j1939_ac_process() cancels this timer while holding priv->lock.
> +	 * Don't block here, otherwise the timer and receive paths can deadlock
> +	 * waiting for each other on different CPUs. Retry shortly if address
> +	 * claim processing currently owns the lock.
> +	 */
> +	if (!write_trylock(&priv->lock)) {

write_trylock() will not take a lock even if read lock is taken on the
receive path. Means, this operation will be delayed not only by the rare
address claim frames, but also by any frame on the bus.

Since, the problem is real, and this fix is good enough for stable, i'm
ok with it + some nit picks.

Update the comment and commit message to something like this:
        /* j1939_ac_process() cancels this timer while holding priv->lock.
         * Don't block here, otherwise the timer and receive paths can
         * deadlock waiting for each other on different CPUs. Retry shortly
         * if the lock is held, by the address claim writer or by a reader
         * on the receive path.
         */

> +		hrtimer_forward_now(hrtimer, ms_to_ktime(1));

Please add a define for 1 ms retry. No magic numbers.

If the bus load will really hit us so strong that we will retry for
too long, it will be good to have some rate limited warning after ~5
retries. Also comment on that 5 retires is not spec related, it is more
"seems to be reasonable" number.

> +		return HRTIMER_RESTART;
> +	}
> +
>  	/* TODO: can we test if ecu->addr is unicast before starting
>  	 * the timer?
>  	 */
> @@ -141,7 +151,7 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
>  	 * j1939_ecu_timer_start().
>  	 */
>  	j1939_ecu_put(ecu);
> -	write_unlock_bh(&priv->lock);
> +	write_unlock(&priv->lock);

Please explain in the commit message, why changing write_*_bh() to write_*()
is a safe option.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |