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

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

[Alexander Hölzl]

There is a potential race condition in segmented messages using
the BAM protocol. When initiating a tx BAM session, first
the BAM itself is sent and then the txtimer is scheduled.
The tx timeout for BAM is currently hardcoded to 50ms.
As soon as the loopbacked BAM frame, acting as a tx
acknowledgment is received, the session->last_cmd field
is set to J1939_TP_CMD_BAM.
When the txtimer elapses the function j1939_xtp_txnext_transmiter
is called, which checks session->last_cmd and continues with
sending a data frame only if session->last_cmd is set to J1939_TP_CMD_BAM.
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.
This problem becomes very obvious when changing the tx timeout
for BAMs to the lowest value allowed by the specification of 10ms.
This patch fixes the problem by moving the tx timer scheduling for
BAM transmissions such that the timer is scheduled when the tx ack
is received. This also fixes the problem that when consecutive
data frames are stuck in the controller they will be sent
without any delay.

Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
---
 net/can/j1939/transport.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..c61a021880c0 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -7,7 +7,6 @@
 //                         Marc Kleine-Budde <kernel@pengutronix.de>
 // Copyright (c) 2017-2019 Pengutronix,
 //                         Oleksij Rempel <kernel@pengutronix.de>
-
 #include <linux/can/skb.h>
 #include <net/can.h>
 
@@ -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
+
 #define J1939_ETP_CMD_RTS 0x14
 #define J1939_ETP_CMD_CTS 0x15
 #define J1939_ETP_CMD_DPO 0x16
@@ -766,7 +768,6 @@ static int j1939_session_tx_rts(struct j1939_session *session)
 
 	session->last_txcmd = dat[0];
 	if (dat[0] == J1939_TP_CMD_BAM) {
-		j1939_tp_schedule_txtimer(session, 50);
 		j1939_tp_set_rxtimeout(session, 250);
 	} else {
 		j1939_tp_set_rxtimeout(session, 1250);
@@ -866,12 +867,19 @@ static int j1939_session_tx_dat(struct j1939_session *session)
 		session->last_txcmd = 0xff;
 		pkt_done++;
 		session->pkt.tx++;
-		pdelay = j1939_cb_is_broadcast(&session->skcb) ? 50 :
-			j1939_tp_packet_delay;
 
-		if (session->pkt.tx < session->pkt.total && pdelay) {
-			j1939_tp_schedule_txtimer(session, pdelay);
+		/* For BAM transfer the tx timer is scheduled after receiving
+		 * the looped back frame as a tx ack. This means that here the
+		 * timer is only scheduled for directed transfers.
+		 */
+		pdelay = j1939_tp_packet_delay;
+		if (j1939_cb_is_broadcast(&session->skcb)) {
 			break;
+		} else {
+			if (session->pkt.tx < session->pkt.total && pdelay) {
+				j1939_tp_schedule_txtimer(session, j1939_tp_packet_delay);
+				break;
+			}
 		}
 	}
 
@@ -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);
+		}
 	} else {
 		if (!session->transmission) {
 			j1939_session_txtimer_cancel(session);
@@ -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);
+		}
 	} else if (do_cts_eoma) {
 		j1939_tp_set_rxtimeout(session, 1250);
 		if (!session->transmission)
-- 
2.56.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: j1939: session reference leak on concurrent txtimer scheduling
--

commit 8433158f9fc2fee1a98accad978bba5e01cfc621
Author: Alexander Hölzl <alexander.hoelzl@gmx.net>

can: j1939: fix potential race condition in BAM segmentation

This patch addresses a race condition in segmented BAM messages by moving
the tx timer scheduling such that it is scheduled when the tx ack is
received. This aims to prevent session stalls when a BAM frame is stuck and
fixes the behavior for consecutive data frames.

> 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
[ ... ]
> @@ -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);
> +		}
>  	} else if (do_cts_eoma) {
>  		j1939_tp_set_rxtimeout(session, 1250);
>  		if (!session->transmission)
>  			j1939_tp_schedule_txtimer(session, 0);

[Severity: High]
Does this do_cts_eoma path leak session references when multiple packets are
processed rapidly?

When a receiver session processes data packets in j1939_xtp_rx_dat_one(), it
evaluates do_cts_eoma = true if pkt.rx >= pkt.last. Because pkt.last is
initialized to 0 and never updated for receiver sessions, this condition is
true for every data packet.

Consequently, j1939_tp_schedule_txtimer(session, 0) is called for every
packet without first cancelling any pending timer.

Since j1939_tp_schedule_txtimer() unconditionally calls
j1939_session_get(session) before starting the timer, calling it multiple
times before the timer fires (e.g. in a NAPI batch) will re-arm the timer
and leak the extra references, as the timer callback only drops a single
reference.

Should this path also call j1939_session_txtimer_cancel(session) first,
similar to the new block added above for remain?

>  	} else {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011153119.120302-1-alexander.hoelzl@gmx.net?part=1

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

[Hölzl, Alexander]

Am 11.10.2026 um 17:48 schrieb sashiko-bot@kernel.org:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] can: j1939: session reference leak on concurrent txtimer scheduling
> --
> 
> commit 8433158f9fc2fee1a98accad978bba5e01cfc621
> Author: Alexander Hölzl <alexander.hoelzl@gmx.net>
> 
> can: j1939: fix potential race condition in BAM segmentation
> 
> This patch addresses a race condition in segmented BAM messages by moving
> the tx timer scheduling such that it is scheduled when the tx ack is
> received. This aims to prevent session stalls when a BAM frame is stuck and
> fixes the behavior for consecutive data frames.
> 
>> 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
> [ ... ]
>> @@ -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);
>> +		}
>>   	} else if (do_cts_eoma) {
>>   		j1939_tp_set_rxtimeout(session, 1250);
>>   		if (!session->transmission)
>>   			j1939_tp_schedule_txtimer(session, 0);
> 
> [Severity: High]
> Does this do_cts_eoma path leak session references when multiple packets are
> processed rapidly?

This review I don't really know how to react to. This is not code I have 
modified so even if it addresses an existing issue it should not be 
handled in this commit.> When a receiver session processes data packets 
in j1939_xtp_rx_dat_one(), it
> evaluates do_cts_eoma = true if pkt.rx >= pkt.last. Because pkt.last is
> initialized to 0 and never updated for receiver sessions, this condition is
> true for every data packet.
> 
This is also not a correct statement as pkt.last is updated in 
j1939_xtp_rx_cts_one even for receiving sessions?> Consequently, 
j1939_tp_schedule_txtimer(session, 0) is called for every
> packet without first cancelling any pending timer.
> 
> Since j1939_tp_schedule_txtimer() unconditionally calls
> j1939_session_get(session) before starting the timer, calling it multiple
> times before the timer fires (e.g. in a NAPI batch) will re-arm the timer
> and leak the extra references, as the timer callback only drops a single
> reference.
> 
> Should this path also call j1939_session_txtimer_cancel(session) first,
> similar to the new block added above for remain?
> 
>>   	} else {
> 

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

[netdev-bot+sashiko]

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

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

> +
>  #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?

[ ... ]

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

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011153119.120302-1-alexander.hoelzl%40gmx.net

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

[Hölzl, Alexander]

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

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

[Oleksij Rempel]

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 |