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

[PATCH] 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 | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index df93d57907da..d00f0c158bb8 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
@@ -754,7 +756,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);
@@ -854,12 +855,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;
+			}
 		}
 	}
 
@@ -1793,8 +1801,12 @@ 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_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);
@@ -1948,6 +1960,10 @@ 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)) {
+			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.43.0

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

[Markus Koeniger]

Hi Alexander,

I really appreciate your solution to schedule the TX timer for a BAM transfer after receiving the looped-back frame.

We use external CAN controllers in our system, which introduce some jitter into the transmit path. As a result, messages were sent from time to time too quickly and violated the 50 ms minimum interval. Your patch solves this problem.

Tested-by: Markus Koeniger markus.koeniger@liebherr.com

Best regards,
Markus

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

On 01.10.2026 12:42:21, Markus Koeniger wrote:
> I really appreciate your solution to schedule the TX timer for a BAM
> transfer after receiving the looped-back frame.

> We use external CAN controllers in our system, which introduce some
> jitter into the transmit path. As a result, messages were sent from
> time to time too quickly and violated the 50 ms minimum interval. Your
> patch solves this problem.
>
> Tested-by: Markus Koeniger markus.koeniger@liebherr.com

Thanks for testing, however I'm not sure if we can get this patch
upstream if sashiko complains about it.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Hölzl, Alexander]

Hello,
Am 01.10.2026 um 12:57 schrieb Marc Kleine-Budde:
> On 01.10.2026 12:42:21, Markus Koeniger wrote:
>> I really appreciate your solution to schedule the TX timer for a BAM
>> transfer after receiving the looped-back frame.
>
Glad that I could help.

>> We use external CAN controllers in our system, which introduce some
>> jitter into the transmit path. As a result, messages were sent from
>> time to time too quickly and violated the 50 ms minimum interval. Your
>> patch solves this problem.
>>
>> Tested-by: Markus Koeniger markus.koeniger@liebherr.com
> 
> Thanks for testing, however I'm not sure if we can get this patch
> upstream if sashiko complains about it.
>
I wasn't planning on letting this patch stall. Next week I'll try to 
address sashiko comment's as well as the other patch I still have open.

Additionally while I'm at it I just wanted to ask, is it intended 
behavior that the kernel implementation strictly serializes all J1939 
sessions. E.g when sending a segmented message directed to destination 
address A it is not possible to have a second session open targeting 
destination address B. According to the standard this is allowed and not
being able to do so can result in very low performance in some 
use-cases. This especially true if one of the sessions is a BAM session 
transmitting a longer message, as there are 50ms pauses between each frame.

> regards,
> Marc
> 

regards,
Alexander

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

[Oleksij Rempel]

Hi,

On Thu, Oct 01, 2026 at 01:18:27PM +0200, Hölzl, Alexander wrote:
> Hello,
> Am 01.10.2026 um 12:57 schrieb Marc Kleine-Budde:
> > On 01.10.2026 12:42:21, Markus Koeniger wrote:
> > > I really appreciate your solution to schedule the TX timer for a BAM
> > > transfer after receiving the looped-back frame.
> > 
> Glad that I could help.
>
> > > We use external CAN controllers in our system, which introduce some
> > > jitter into the transmit path. As a result, messages were sent from
> > > time to time too quickly and violated the 50 ms minimum interval. Your
> > > patch solves this problem.
> > > 
> > > Tested-by: Markus Koeniger markus.koeniger@liebherr.com
> > 
> > Thanks for testing, however I'm not sure if we can get this patch
> > upstream if sashiko complains about it.
> > 
> I wasn't planning on letting this patch stall. Next week I'll try to address
> sashiko comment's as well as the other patch I still have open.

Nice, thx.

I'm working right now on  j1939 selftests for core functionality. Hope
it will be ready this week. If not, next week i'll be in Prag on E-OSS
conference...

> Additionally while I'm at it I just wanted to ask, is it intended behavior
> that the kernel implementation strictly serializes all J1939 sessions. E.g
> when sending a segmented message directed to destination address A it is not
> possible to have a second session open targeting destination address B.
> According to the standard this is allowed and not
> being able to do so can result in very low performance in some use-cases.
> This especially true if one of the sessions is a BAM session transmitting a
> longer message, as there are 50ms pauses between each frame.

Is it not working with a separate socket? If I remember it correctly,
this behavior should be supported if two sockets send to separate
addresses. Withing one socket, frames should be serialized.

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

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

[Hölzl, Alexander]

Am 01.10.2026 um 13:37 schrieb Oleksij Rempel:
> Hi,
> 
> On Thu, Oct 01, 2026 at 01:18:27PM +0200, Hölzl, Alexander wrote:
>> Hello,
>> Am 01.10.2026 um 12:57 schrieb Marc Kleine-Budde:
>>> On 01.10.2026 12:42:21, Markus Koeniger wrote:
>>>> I really appreciate your solution to schedule the TX timer for a BAM
>>>> transfer after receiving the looped-back frame.
>>>
>> Glad that I could help.
>>
>>>> We use external CAN controllers in our system, which introduce some
>>>> jitter into the transmit path. As a result, messages were sent from
>>>> time to time too quickly and violated the 50 ms minimum interval. Your
>>>> patch solves this problem.
>>>>
>>>> Tested-by: Markus Koeniger markus.koeniger@liebherr.com
>>>
>>> Thanks for testing, however I'm not sure if we can get this patch
>>> upstream if sashiko complains about it.
>>>
>> I wasn't planning on letting this patch stall. Next week I'll try to address
>> sashiko comment's as well as the other patch I still have open.
> 
> Nice, thx.
> 
> I'm working right now on  j1939 selftests for core functionality. Hope
> it will be ready this week. If not, next week i'll be in Prag on E-OSS
> conference...
> 
Ah that's good to know. In the CTS hold patch  I've also implemented 
some tests (https://lkml.org/lkml/2026/7/7/85) but I guess they'll 
mostly be superfluous then?>> Additionally while I'm at it I just wanted 
to ask, is it intended behavior
>> that the kernel implementation strictly serializes all J1939 sessions. E.g
>> when sending a segmented message directed to destination address A it is not
>> possible to have a second session open targeting destination address B.
>> According to the standard this is allowed and not
>> being able to do so can result in very low performance in some use-cases.
>> This especially true if one of the sessions is a BAM session transmitting a
>> longer message, as there are 50ms pauses between each frame.
> 
> Is it not working with a separate socket? If I remember it correctly,
> this behavior should be supported if two sockets send to separate
> addresses. Withing one socket, frames should be serialized.
> 
I've never tried it with multiple sockets but if you say so I'm sure 
it'll work :). For my use case specifically this quite inconvenient 
behavior but I guess I might be abusing the stack a little bit :).
I've written a patch which allows interleaved tx-sessions per socket but 
if the kernel behaves as designed then that's probably not something 
which I should try to get into the mainline.