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

["Hölzl, Alexander" <alexander.hoelzl@gmx.net>]

Am 13.10.2026 um 06:56 schrieb netdev-bot+sashiko@kernel.org:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 3 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The new BAM-transmitter echo branches can bring an aborted
>    session back to life.
> - [Medium] Forged TP.DT frames can stall a BAM transmission for as long
>    as they keep coming, and it never times out.
> - [Medium] The new re-arm in the RX path (cancel, then schedule) is not
>    atomic and holds no lock, so it can leak session references.
> - [Low] The commit message understates the bug and has no Fixes: tag.
> - [Low] The new J1939_TP_BAM_ECHO_TIMEOUT_MS (250) constant does not
>    control the timeouts that actually wait for an echo.
> 
> --
> 
> can: j1939: fix potential race condition in BAM segmentation
> 
> This patch moves txtimer scheduling for BAM transmissions out of the
> transmit path and into the echo (tx ack) handlers. The 50ms frame spacing
> timer is now armed in j1939_xtp_rx_rts() and j1939_xtp_rx_dat_one() when
> the looped back BAM or DT frame arrives, instead of in
> j1939_session_tx_rts() and j1939_session_tx_dat().
> 
>> If there is a high busload and the BAM frame is stuck in the controller
>> for some time, the txtimer might elapse before the looped back frame is
>> received and the session will stall.
> 
> [Severity: Low]
> Is "stall" the right word for what happens before this patch?
> 
> If the 50ms txtimer fires before the BAM echo arrives, last_cmd is still 0.
> j1939_xtp_txnext_transmiter() then calls j1939_session_tx_rts() again, and
> that call returns early:
> 
> 	if (dat[0] == session->last_txcmd)
> 		/* done already */
> 		return 0;
> 
> Nothing re-arms the txtimer after that. The 250ms rxtimer armed in
> j1939_session_tx_rts() then fires, and j1939_tp_rxtimer() aborts the
> session:
> 
> 	__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> 
> So user space sees the transfer aborted with EHOSTUNREACH, not a stall.
> Could the commit message describe that?
> 
> This also fixes behaviour that goes back to the original J1939 code. Should
> it carry this tag so stable picks it up?
> 
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> 

I called it stall because that's how it looks like when looking at the 
bus traces. The tag I can add as well.>> diff --git 
a/net/can/j1939/transport.c b/net/can/j1939/transport.c
>> index 8fcfd13e5e6f8..c61a021880c01 100644
>> --- a/net/can/j1939/transport.c
>> +++ b/net/can/j1939/transport.c
> 
> [ ... ]
> 
>> @@ -26,6 +25,9 @@
>>   #define J1939_TP_CMD_BAM 0x20
>>   #define J1939_TP_CMD_ABORT 0xff
>>   
>> +#define J1939_TP_BAM_FRAME_SPACING_MS 50
>> +#define J1939_TP_BAM_ECHO_TIMEOUT_MS 250
> 
> [Severity: Low]
> This isn't a bug, but J1939_TP_BAM_ECHO_TIMEOUT_MS does not seem to set the
> timeouts that actually wait for an echo. Those still use a bare 250.
> 
> In j1939_session_tx_rts(), after the BAM is sent:
> 
> 	if (dat[0] == J1939_TP_CMD_BAM) {
> 		j1939_tp_set_rxtimeout(session, 250);

In this case the macro should probably used to be consistent.

> 
> In j1939_session_tx_dat(), after each DT frame:
> 
> 	if (pkt_done)
> 		j1939_tp_set_rxtimeout(session, 250);
> 
> The macro is only used in j1939_xtp_rx_rts() and j1939_xtp_rx_dat_one().
> Both run after the echo has already arrived, and j1939_session_tx_dat()
> overwrites that rxtimer about 50ms later.
> 
> Should the echo wait sites use the macro instead?

In my patch, I'm scheduling the rxtimeout in the rx functions when the 
corresponding frame is received. It would be cleaner to do it in 
j1939_session_tx_dat and get rid of the calls to j1939_tp_set_rxtimeout 
I've added. The J1939_TP_BAM_ECHO_TIMEOUT_MS macro should then probably 
be renamed to be a generic J1939_TP_ECHO_TIMEOUT_MS and can then be used 
in j1939_session_tx_rts and j1939_session_tx_dat.

> 
>> +
>>   #define J1939_ETP_CMD_RTS 0x14
> 
> [ ... ]
> 
>> @@ -1806,8 +1814,13 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
>>   	session->last_cmd = cmd;
>>   
>>   	if (cmd == J1939_TP_CMD_BAM) {
>> -		if (!session->transmission)
>> +		if (!session->transmission) {
>>   			j1939_tp_set_rxtimeout(session, 750);
>> +		} else {
>> +			j1939_session_txtimer_cancel(session);
>> +			j1939_tp_schedule_txtimer(session, J1939_TP_BAM_FRAME_SPACING_MS);
>> +			j1939_tp_set_rxtimeout(session, J1939_TP_BAM_ECHO_TIMEOUT_MS);
>> +		}
> 
> [Severity: Medium]
> Can a late echo bring an aborted session back to life here? The same
> question applies to the new branch in j1939_xtp_rx_dat_one().
> 
> Both branches re-arm the txtimer and rxtimer without
> active_session_list_lock and without checking session->state. Under the
> bus load this patch targets, a BAM or DT echo can arrive more than 250ms
> late. By then j1939_tp_rxtimer() has already taken the abort path under the
> list lock:
> 
> j1939_tp_rxtimer()
>      j1939_session_list_lock(session->priv);
>      ...
>          hrtimer_start(&session->rxtimer,
>                        ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
>                        HRTIMER_MODE_REL_SOFT);
>          __j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> 
> That sets J1939_SESSION_WAITING_ABORT, sets err to EHOSTUNREACH and arms
> the forced deactivation timer.
> 
> The session is still on active_session_list, so the late echo is still
> delivered here. j1939_tp_set_rxtimeout() then cancels the abort rxtimer and
> replaces it with a normal 250ms one. The txtimer fires 50ms later:
> 
> j1939_tp_txtimer()
>    j1939_xtp_txnext_transmiter()
>      j1939_session_tx_dat()
> 
> None of these check session->state. More DT frames therefore go out for a
> session that was already reported as aborted.
> 
> Each of those echoes goes through the same re-arm in
> j1939_xtp_rx_dat_one(), which pushes the forced deactivation back again.
> The session can even reach j1939_session_completed() after user space was
> told the transfer was aborted.
> 
> There also seems to be a narrower window against
> j1939_cancel_active_session() (socket close or NETDEV_DOWN). It cancels the
> timers under active_session_list_lock:
> 
> 	if (hrtimer_try_to_cancel(&session->txtimer) == 1)
> 		j1939_session_put(session);
> 
> The RX path still holds the reference from j1939_session_get_by_addr(). If
> it re-arms after this point, one more frame is sent for a session in
> J1939_SESSION_DONE.
> 
> Before this patch, the BAM transmitter echo path only updated last_cmd and
> pkt.rx. Should these new branches check session->state under the list lock
> before re-arming?
> 
> [ ... ]

This sounds plausible, I can add the checks.

> 
>> @@ -1961,6 +1974,12 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
>>   	} else if (remain) {
>>   		if (!session->transmission)
>>   			j1939_tp_set_rxtimeout(session, 750);
>> +		else if (j1939_cb_is_broadcast(&session->skcb)) {
>> +			/* cancel the txtimer to avoid leaking session references */
>> +			j1939_session_txtimer_cancel(session);
>> +			j1939_tp_schedule_txtimer(session, J1939_TP_BAM_FRAME_SPACING_MS);
>> +			j1939_tp_set_rxtimeout(session, J1939_TP_BAM_ECHO_TIMEOUT_MS);
>> +		}
> 
> [Severity: Medium]
> Can forged TP.DT frames hold off a BAM transmission indefinitely here?
> 
> j1939_can_recv() sets J1939_ECU_LOCAL_SRC based only on the SA:
> 
> 	if (j1939_address_is_unicast(skcb->addr.sa) &&
> 	    priv->ents[skcb->addr.sa].nusers)
> 		skcb->flags |= J1939_ECU_LOCAL_SRC;
> 
> So j1939_xtp_rx_dat() routes any TP.DT frame with the host's SA and DA 0xff
> to the transmitter session's j1939_xtp_rx_dat_one(). That frame can come
> from another node on the bus or from a local CAN_RAW socket.
> 
> A frame with packet != session->pkt.rx (for example, dat[0] = 1 sent
> repeatedly) does not advance pkt.rx:
> 
> 	if (packet == session->pkt.rx)
> 		session->pkt.rx++;
> 
> It still sets remain, though, so this branch cancels and re-arms the 50ms
> txtimer and the 250ms rxtimer. The memcmp mismatch is only logged once.
> 
> Suppose such a frame arrives at least every 50ms. The txtimer never fires,
> so no data goes out. The rxtimer never fires either, so the session is never
> aborted. Later BAM transfers queued for that SA would hang behind it as
> well.
> 
> Before this patch, the transmitter paced itself with its own txtimer, and
> this branch ignored transmitter sessions.
> 
> Should this branch re-arm only when the frame is the echo of the packet
> just sent (packet == session->pkt.tx - 1), or when pkt.rx actually
> advanced?
> 
This is also plausible, I can also add this check. Alternatively one 
could also check if the skb is really an echo as is done in the can raw 
implementation and only handle actual echo frames and discard frames 
sent by other nodes.
> [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?

pw-bot: cr