Re: [PATCH v2] Fix J1939 implementation not handling holds correctly

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

Hello,
once again thank you for your thorough review :)
I'll try to send the next patch by tomorrow evening.

On 5/17/26 09:46, Oleksij Rempel wrote:
> Hi Alexander,
> 
> On Sat, May 16, 2026 at 04:35:25PM +0200, Alexander Hölzl wrote:
>> The J1939 protocol allows the receiver of directed segemented messages
>> to hold the data transfer. The kernel implementation did not handle hold
>> messages correctly was not able to resume from a hold.
>>
>> Fix J1939 RTS/CTS session not being able to resume from hold.
>> Replace hardcoded timeout with define.
>> Add CTS hold behavior tests.
>>
>> Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
>> ---
>> Compared to the last patch I removed all of the todo comments I had
>> still had in the code. I will implement hold notification in the error
>> queue in different patch.
>>
>> I replaced the hardcoded hold timeout with a define. I can replace the
>> other hardcoded timeouts in a different patch if you want me to, but
>> for now I only touched code related to the holds.
>> I also added a helper function to check if a CTS is a hold.
>>
>> I also added a baseline test case as you wanted. Altough in the future
>> it should probably moved to another file which specfically tests normal
>> RTS/CTS behavior.
>>
>> Also addressed all the comments Sashiko had on the test file and I'm
>> also now sending an EOMA in the test as without it between every test
>> there was a 1250ms wait until the session timed out...
> 
> Nice!
> 
>>   net/can/j1939/transport.c                     |  48 ++-
> 
> Please split fix and testing patches. Otherwise reverting it for testing
> will break testing (at least it will make things a bit more
> complicated).
Sure that's a good point! Also the test script test_cts_hold.sh is very 
much based on the  test_raw_filter.sh script. Now that I'm thinking 
about it, it would be probably cleaner to move out the common parts into 
a helper file?>
>>   tools/testing/selftests/net/can/.gitignore    |   1 +
>>   tools/testing/selftests/net/can/Makefile      |   8 +-
>>   tools/testing/selftests/net/can/config        |   1 +
>>   .../testing/selftests/net/can/test_cts_hold.c | 322 ++++++++++++++++++
>>   .../selftests/net/can/test_cts_hold.sh        |  45 +++
>>   6 files changed, 406 insertions(+), 19 deletions(-)
>>   create mode 100644 tools/testing/selftests/net/can/test_cts_hold.c
>>   create mode 100755 tools/testing/selftests/net/can/test_cts_hold.sh
> 
> 
>> +/* Time until session invalidation upon reception of a hold message.
>> + * Corresponds to T4 in the specification.
> 
> I would add here documentation references, it would help another me in
> two yars to understand things :)
> ISO 11783-3 2018 - 5.10.3.5 Connection closure
> SAE J1939-21 2001 - 5.10.2.4 Connection Closure
> 
> You can update version if you have more recent variants.
Oh yeah, the 2001 version is quite old, I'm using the 2022 revision.
Although I don't think there are any changes in the spec which would be
relevant for this commit in there.
Will add the comments.>
>> + */
>> +#define J1939_CTS_HOLD_TIMEOUT_MS 1050
>> +
>>   enum j1939_xtp_abort {
>>   	J1939_XTP_NO_ABORT = 0,
>>   	J1939_XTP_ABORT_BUSY = 1,
>> @@ -1428,6 +1433,11 @@ j1939_xtp_rx_eoma(struct j1939_priv *priv, struct sk_buff *skb,
>>   	j1939_session_put(session);
>>   }
>>   
> 
> Here comment:
> SAE J1939-21 2001 - 5.10.2.4 Connection Closure
> ISO 11783-3 2018 - 5.11.5.4 Extended Connection Mode Clear To Send (ETP.CM_CTS)
> 
> The number of packets to send can be set to 0 to hold the connection
> 
>> +static inline bool j1939_cts_is_hold(const struct sk_buff *skb)
>> +{
>> +	return (!skb->data[1]);
>> +}
>> +
>>   static void
>>   j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>>   {
>> @@ -1442,9 +1452,15 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>>   
>>   	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
>>   
>> -	if (session->last_cmd == dat[0]) {
>> -		err = J1939_XTP_ABORT_DUP_SEQ;
>> -		goto out_session_cancel;
> 
> We need to document old and new sanity check behavior in the commit
> message.
> 
> Before this change it protects only against a flood of CTS where the
> linux stack didn't managed to start sending the data - less probable
> scenario. And it prevents valid CTS hold support - most probable
> scenario.
> 
> After the patch it allows to sende multiple CTS including CTS(0), but
> prevents requesting the already transferred and acked packets. So, the
> kernel will abort immediately instead of going in to timeout.

Correct, and just to say it again sending a CTS while the transmission 
is ongoing is also not correct according to the standard. So a strict 
implementation would abort the session on reception of a CTS while 
transmitting. But that's a problem for a future patch.

>> +	session->last_cmd = dat[0];
>> +
>> +	if (j1939_cts_is_hold(skb)) {
>> +		if (session->transmission)
>> +			j1939_session_txtimer_cancel(session);
>> +
> 
> Here we need a comment:
> The originator should abort the session after T4 (=< 1050ms):
>   SAE J1939-21 2001 - 5.10.2.4 Connection Closure
>   a lack of a CTS for more than (T4) seconds after a CTS (0) message to
>   "hold the connection open" will all cause a connection closure to occur.
> 
> The receiver should send followup CTS not later then Th (=< 500ms):
>   SAE J1939-21 2001 - C.1 Connection Mode Data Transfer
>   The responder station then issues a TP.CM_CTS indicating that it wants
>   to hold the connection open but cannot receive any packets right now. A
>   maximum of 500 ms later it must send another TP.CM_CTS message to hold
>   the connection.
> 
> But we care only about T4 value on our side.
  Will add!>
>> +		j1939_tp_set_rxtimeout(session, J1939_CTS_HOLD_TIMEOUT_MS);
> 
> 
>> +		netdev_dbg(session->priv->ndev, "%s: 0x%p received CTS hold\n", __func__, session);
>> +		return;
>>   	}
>>   
>>   	if (session->skcb.addr.type == J1939_ETP)
>> @@ -1457,7 +1473,11 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>>   	else if (dat[1] > session->pkt.block /* 0xff for etp */)
>>   		goto out_session_cancel;
>>   
>> -	/* set packet counters only when not CTS(0) */
>> +	if (session->pkt.tx_acked >= pkt) {
>> +		err = J1939_XTP_ABORT_DUP_SEQ;
>> +		goto out_session_cancel;
>> +	}
>> +
>>   	session->pkt.tx_acked = pkt - 1;
>>   	j1939_session_skb_drop_old(session);
>>   	session->pkt.last = session->pkt.tx_acked + dat[1];
>> @@ -1467,19 +1487,13 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>>   	/* TODO: do not set tx here, do it in txtimer */
>>   	session->pkt.tx = session->pkt.tx_acked;
>>   
>> -	session->last_cmd = dat[0];
>> -	if (dat[1]) {
>> -		j1939_tp_set_rxtimeout(session, 1250);
>> -		if (session->transmission) {
>> -			if (session->pkt.tx_acked)
>> -				j1939_sk_errqueue(session,
>> -						  J1939_ERRQUEUE_TX_SCHED);
>> -			j1939_session_txtimer_cancel(session);
>> -			j1939_tp_schedule_txtimer(session, 0);
>> -		}
>> -	} else {
>> -		/* CTS(0) */
>> -		j1939_tp_set_rxtimeout(session, 550);
> 
> Now I understand where 550 comes from. It is Th + 50. But you are
> correct, we need to use T4 here.
Exactly, the Th is a receiver side performance requirement. But it is a
quite confusing standard. It's very easy to get confused.>
>> +	j1939_tp_set_rxtimeout(session, 1250);
>> +	if (session->transmission) {
>> +		if (session->pkt.tx_acked)
>> +			j1939_sk_errqueue(session,
>> +						J1939_ERRQUEUE_TX_SCHED);
>> +		j1939_session_txtimer_cancel(session);
>> +		j1939_tp_schedule_txtimer(session, 0);
>>   	}
>>   	return;
>>   
>> diff --git a/tools/testing/selftests/net/can/.gitignore b/tools/testing/selftests/net/can/.gitignore
>> +
>> +/* Segemented payload sent by the J1939 socket*/
>> +const uint8_t J1939_PAYLOAD[]	= {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09};
> 
> can you please remove tabs or extra spaces between [] and =
> const uint8_t J1939_PAYLOAD[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09};
> 
Sure!

>> +
>> +/* Expected RTS payload */
>> +const uint8_t RTS_PAYLOAD[]					= {0x10, 0x0A, 0x00, 0x02, 0x02, 0x00, 0xAB, 0x00};
>> +/* Hold payload to be sent by raw socket */
>> +const uint8_t HOLD_PAYLOAD[]				= {0x11, 0x00, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +/* CTS to send to only allow for the transmission of one data frame */
>> +const uint8_t CTS_1_FRAME_PAYLOAD[]			= {0x11, 0x01, 0x01, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +/* Resume payload to resume from connection which has been held directly after RTS*/
>> +const uint8_t RESUME_IMMEDIATE_PAYLOAD[]	= {0x11, 0x02, 0x01, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +/* Resume payload to resume session which has been held after first data frame */
>> +const uint8_t RESUME_PAYLOAD[]				= {0x11, 0x01, 0x02, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +/* Data payloads */
>> +const uint8_t DATA_1_PAYLOAD[]				= {0x01, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06};
>> +const uint8_t DATA_2_PAYLOAD[]				= {0x02, 0x07, 0x08, 0x09, 0xFF, 0xFF, 0xFF, 0xFF};
>> +
>> +/* EOMA payload to cleanup session */
>> +const uint8_t EOMA_PAYLOAD[]				= {0x13, 0x0A, 0x00, 0x02, 0xFF, 0x00, 0xAB, 0x00};
>> +
>> +/* Timeout payload sent on connection timeout */
>> +const uint8_t ABORT_TIMEOUT_PAYLOAD[]		= {0xFF, 0x03, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +char CANIF[IFNAMSIZ];
>> +
>> +static int recv_payload(int sock, const uint8_t *payload, size_t len)
>> +{
>> +	struct can_frame rx_frame = {};
>> +
>> +	if (recv(sock, &rx_frame, sizeof(rx_frame), 0) < 0) {
> 
> recv will block until we receive something, it will be not good if test fails
> and nothing is send. Probably something like this:
> 
> static int recv_payload_timeout(int sock, const uint8_t *payload, size_t len, int timeout_ms)
> {
> 	struct can_frame rx_frame = {};
> 	struct pollfd pfd = {
> 		.fd = sock,
> 		.events = POLLIN,
> 	};
> 	int ret;
> 
> 	/* Wait for data to be ready to read, up to timeout_ms */
> 	ret = poll(&pfd, 1, timeout_ms);
> 	if (ret < 0) {
> 		perror("poll failed");
> 		return 1;
> 	}
> 	
> 	if (ret == 0) {
> 		fprintf(stderr, "timeout waiting for can raw frame\n");
> 		return 1;
> 	}
> 
> 	/* Socket is readable, recv will not block */
> 	if (recv(sock, &rx_frame, sizeof(rx_frame), 0) < 0) {
> 		perror("failed to recv can raw frame");
> 		return 1;
> 	}
> 
> 	if (rx_frame.len != len) {
> 		fprintf(stderr, "received data length does not match expected value\n");
> 		return 1;
> 	}
> 
> 	if (memcmp(rx_frame.data, payload, len)) {
> 		fprintf(stderr, "received data does not match expected value\n");
> 		return 1;
> 	}
> 
> 	return 0;
> }
> 
My original way to avoid this problem was setting SO_RCVTIMEO on the raw 
socket but I can use poll instead. That would also have the advantage 
that you could change the timeout more easily. Will do!

> ....
> 
>> +/* Test timeout after not resuming hold */
>> +TEST_F(can_env, test_hold_timeout)
>> +{
>> +	struct can_frame tx_frame = {
>> +		.can_id = RECEIVER_TP_CM_ID,
>> +		.len = 8,
>> +	};
>> +
>> +	memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
>> +	int res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
>> +
>> +	ASSERT_GT(res, 0)
>> +		TH_LOG("failed to send j1939 payload: %d", errno);
>> +
>> +	res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
>> +	ASSERT_EQ(res, 0)
>> +		TH_LOG("Failed to receive RTS as expeceted");
>> +
>> +	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
>> +	ASSERT_GT(res, 0)
>> +		TH_LOG("failed to send hold with raw sock: %d", errno);
>> +
>> +	/* Wait for 1100 ms and receive the abort due to CTS hold timeout.
>> +	 * The actual timeout is 1050ms but with this test setup there is no point
>> +	 * in trying to be this exact.
>> +	 */
>> +	usleep(1100 * 1000);
>> +
>> +	res = recv_payload(self->raw_sock, ABORT_TIMEOUT_PAYLOAD, sizeof(ABORT_TIMEOUT_PAYLOAD));
>> +	ASSERT_EQ(res, 0)
>> +		TH_LOG("Failed to receive abort as expeceted");
>> +}
>   
> This test may potentially have random fails due to CI system load.
> May be:
> 
> #define DEFAULT_RECV_TIMEOUT_MS 2000
> 
> static int recv_payload(int sock, const uint8_t *payload, size_t len)
> {
> 	return recv_payload_timeout(sock, payload, len, DEFAULT_RECV_TIMEOUT_MS);
> }
> 
> /* Test timeout after not resuming hold */
> TEST_F(can_env, test_hold_timeout)
> {
> 	struct can_frame tx_frame = {
> 		.can_id = RECEIVER_TP_CM_ID,
> 		.len = 8,
> 	};
> 	struct timespec start, end;
> 	long elapsed_ms;
> 	int res;
> 
> 	memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
> 	res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
> 	ASSERT_GT(res, 0)
> 		TH_LOG("failed to send j1939 payload: %d", errno);
> 
> 	res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
> 	ASSERT_EQ(res, 0)
> 		TH_LOG("Failed to receive RTS as expected");
> 
> 	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
> 	ASSERT_GT(res, 0)
> 		TH_LOG("failed to send hold with raw sock: %d", errno);
> 
> 	/* Record start time */
> 	clock_gettime(CLOCK_MONOTONIC, &start);
> 
> 	/*
> 	 * Receive with a timeout larger than the expected 1050ms J1939 timeout.
> 	 * 2000ms provides plenty of headroom for CI without hanging indefinitely.
> 	 */
> 	res = recv_payload_timeout(self->raw_sock, ABORT_TIMEOUT_PAYLOAD,
> 				   sizeof(ABORT_TIMEOUT_PAYLOAD), 2000);
> 	
> 	ASSERT_EQ(res, 0)
> 		TH_LOG("Failed to receive abort as expected");
> 
> 	/* Record end time and calculate elapsed milliseconds */
> 	clock_gettime(CLOCK_MONOTONIC, &end);
> 	elapsed_ms = (end.tv_sec - start.tv_sec) * 1000 +
> 		     (end.tv_nsec - start.tv_nsec) / 1000000;
> 
> 	/*
> 	 * The actual timeout is 1050ms. We define an acceptable window
> 	 * to account for CI scheduling variations.
> 	 */
> 	ASSERT_GE(elapsed_ms, 1000)
> 		TH_LOG("Abort received too early: %ld ms", elapsed_ms);
> 	ASSERT_LE(elapsed_ms, 1500)
> 		TH_LOG("Abort received too late: %ld ms", elapsed_ms);
> }
> 
> 
Perfect, thank you!