Re: [PATCH v3] can: j1939: fix potential race condition in BAM segmentation

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

On Tue, Oct 13, 2026 at 11:35:24AM +0200, Hölzl, Alexander wrote:
> > [Severity: Medium]
> > Could this cancel then re-arm sequence leak session references if two DT
> > frames for the same BAM session are processed at the same time?
> > 
> > j1939_tp_schedule_txtimer() always takes a reference:
> > 
> > 	j1939_session_get(session);
> > 	hrtimer_start(&session->txtimer, ms_to_ktime(msec),
> > 		      HRTIMER_MODE_REL_SOFT);
> > 
> > Nothing serialises j1939_xtp_rx_dat_one() calls for the same session.
> > j1939_session_get_by_addr() holds active_session_list_lock only during the
> > lookup.
> > 
> > Two CPUs could handle TP.DT frames for this session at once. One example is
> > the af_can loopback netif_rx() for a device without IFF_ECHO, racing a
> > frame from the bus or an injected one. Another is two CAN_RAW injectors on
> > vcan. The interleaving would be:
> > 
> > CPU1                              CPU2
> > j1939_session_txtimer_cancel()
> >    hrtimer_cancel() == 1, put
> >                                    j1939_session_txtimer_cancel()
> >                                      hrtimer_cancel() == 0
> > j1939_tp_schedule_txtimer()
> >    j1939_session_get()
> >    hrtimer_start()
> >                                    j1939_tp_schedule_txtimer()
> >                                      j1939_session_get()
> >                                      hrtimer_start()
> > 
> > When the timer fires, only one reference is dropped. The rxtimer has the
> > same problem through j1939_tp_set_rxtimeout().
> > 
> > A leaked session never reaches j1939_session_destroy(). Its j1939_priv
> > reference and priv_dev_tracker netdev reference are then never released,
> > and unregistering the CAN netdev would hang waiting for the device to
> > become free.
> > 
> > j1939_xtp_rx_cts_one() already uses the same unlocked pattern for directed
> > sessions. Before this patch, though, the RX path never touched the txtimer
> > for BAM transmitter sessions.
> > 
> This also sounds plausible, how should this be fixed? Should additional
> locks be added? The session list lock is probably not the correct one to use
> as this is a global lock over all sessions.
> Additionally is the same type of race condition then found in
> j1939_tp_set_rxtimeout? It does basically the same thing and could also be
> called concurrently while e.g. receiving a can frame and handling a timer
> callback?
> 
> CPU 1 callstack:
> j1939_tp_recv -> j1939_xtp_rx_dat -> j1939_xtp_rx_dat_one ->
> j1939_tp_set_rxtimeout
> 
> CPU 2 callstack:
> j1939_tp_txtimer -> j1939_xtp_txnext_transmiter -> j1939_session_tx_dat ->
> j1939_tp_set_rxtimeout
> 
> Could this cause a similar leak as described above?

Can you please take a look here:
https://docs.kernel.org/dev-tools/context-analysis.html

I hope it will help to resole some of the locking issues.

-- 
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 |