[PATCH v3 1/2] Fix J1939 implementation not handling holds correctly

[PATCH v3 1/2] Fix J1939 implementation not handling holds correctly

[Alexander Hölzl]

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.

To do so the behavior of j1939_xtp_rx_cts_one was modified to allow the
handling of a hold. The previous sanity check was removed as it only
guarded against a flood of consecutive CTS, but prevented the hold
from working correctly. This patch changes this behavior to allow
for consectuive CTS to enable holds. An additional sanity check
has been added which prevents requsts of already transferred and
acked packets. In this case the kernel will abort immediately
instead of going into a timeout.

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>
---
I've integrated the comments you've send. In one of the comments you've 
referenced a wrong section of the J1939 standard. For the hold message
you've referenced SAE J1939-21 2001 - 5.10.2.4 Connection Closure,
but it should have been SAE 5.102.2.3 Data Transfer. That I have
changed. Otherwise everything should be according to your comments :)

 net/can/j1939/transport.c | 68 +++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index df93d57907da..e2c79df7b04e 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -32,6 +32,13 @@
 #define J1939_ETP_CMD_EOMA 0x17
 #define J1939_ETP_CMD_ABORT 0xff
 
+/* Time until session invalidation upon reception of a hold message.
+ * Corresponds to T4 in the specification.
+ * See ISO 11783-3 2018 - 5.10.3.5 Connection closure
+ * and SAE J1939-21 2022 - 5.10.2.4 Connection Closure
+ */
+#define J1939_CTS_HOLD_TIMEOUT_MS 1050
+
 enum j1939_xtp_abort {
 	J1939_XTP_NO_ABORT = 0,
 	J1939_XTP_ABORT_BUSY = 1,
@@ -1428,6 +1435,16 @@ j1939_xtp_rx_eoma(struct j1939_priv *priv, struct sk_buff *skb,
 	j1939_session_put(session);
 }
 
+/* See:
+ * SAE J1939-21 2022 - 5.10.2.3 Data Transfer
+ * 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 +1459,28 @@ 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;
+	session->last_cmd = dat[0];
+
+	if (j1939_cts_is_hold(skb)) {
+		/* The originator should abort the session after T4 (=< 1050ms):
+		 *   SAE J1939-21 2022 - 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.
+		 *
+		 */
+		if (session->transmission)
+			j1939_session_txtimer_cancel(session);
+
+		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 +1493,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 +1507,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);
+	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;
 
-- 
2.54.0

[PATCH v3 2/2] Add J1939 CTS hold tests

[Alexander Hölzl]

Add tests to verify the correct behavior of CTS hold messages.

The test verify that the J1939 is correctly able to restart the
transmission after the reception of a hold message and the
session is terminated if the receiver does not send a CTS to
resume from the hold.

Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
---
 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 | 359 ++++++++++++++++++
 .../selftests/net/can/test_cts_hold.sh        |  45 +++
 5 files changed, 412 insertions(+), 2 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

diff --git a/tools/testing/selftests/net/can/.gitignore b/tools/testing/selftests/net/can/.gitignore
index 764a53fc837f..96ef18ae986d 100644
--- a/tools/testing/selftests/net/can/.gitignore
+++ b/tools/testing/selftests/net/can/.gitignore
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 test_raw_filter
+test_cts_hold
\ No newline at end of file
diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile
index 5b82e60a03e7..182346682bce 100644
--- a/tools/testing/selftests/net/can/Makefile
+++ b/tools/testing/selftests/net/can/Makefile
@@ -4,8 +4,12 @@ top_srcdir = ../../../../..
 
 CFLAGS += -Wall -Wl,--no-as-needed -O2 -g -I$(top_srcdir)/usr/include $(KHDR_INCLUDES)
 
-TEST_PROGS := test_raw_filter.sh
+TEST_PROGS := \
+	test_raw_filter.sh \
+	test_cts_hold.sh \
 
-TEST_GEN_FILES := test_raw_filter
+TEST_GEN_FILES := \
+	test_raw_filter \
+	test_cts_hold \
 
 include ../../lib.mk
diff --git a/tools/testing/selftests/net/can/config b/tools/testing/selftests/net/can/config
index 188f79796670..cb538ed93ae4 100644
--- a/tools/testing/selftests/net/can/config
+++ b/tools/testing/selftests/net/can/config
@@ -1,3 +1,4 @@
 CONFIG_CAN=m
 CONFIG_CAN_DEV=m
 CONFIG_CAN_VCAN=m
+CONFIG_CAN_J1939=m
\ No newline at end of file
diff --git a/tools/testing/selftests/net/can/test_cts_hold.c b/tools/testing/selftests/net/can/test_cts_hold.c
new file mode 100644
index 000000000000..4fe4b97d6206
--- /dev/null
+++ b/tools/testing/selftests/net/can/test_cts_hold.c
@@ -0,0 +1,359 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <time.h>
+
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/ioctl.h>
+#include <net/if.h>
+#include <linux/if.h>
+
+#include <linux/can.h>
+#include <linux/can/raw.h>
+#include <linux/can/j1939.h>
+
+
+#include "kselftest_harness.h"
+
+
+#define SENDER_ADDR		0x10
+#define RECEIVER_ADDR	0x20
+
+#define TEST_PGN 0xAB00
+#define SENDER_TP_CM_ID		(0x18EC2010 | CAN_EFF_FLAG)
+#define RECEIVER_TP_CM_ID	(0x18EC1020 | CAN_EFF_FLAG)
+
+#define DEFAULT_RECV_TIMEOUT_MS 2000
+
+/* Segemented payload sent by the J1939 socket*/
+const uint8_t J1939_PAYLOAD[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09};
+
+/* 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_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;
+}
+
+static int recv_payload(int sock, const uint8_t *payload, size_t len)
+{
+	return recv_payload_timeout(sock, payload, len, DEFAULT_RECV_TIMEOUT_MS);
+}
+
+
+FIXTURE(can_env)
+{
+	int j1939_sock;
+	int raw_sock;
+};
+
+FIXTURE_SETUP(can_env)
+{
+	struct sockaddr_can addr = {};
+	struct ifreq ifr = {};
+	int ret;
+
+	self->raw_sock = -1;
+	self->j1939_sock = -1;
+
+	self->raw_sock = socket(PF_CAN, SOCK_RAW, CAN_RAW);
+	ASSERT_GE(self->raw_sock, 0)
+		TH_LOG("failed to create CAN_RAW socket: %d", errno);
+
+	strncpy(ifr.ifr_name, CANIF, sizeof(ifr.ifr_name));
+	ret = ioctl(self->raw_sock, SIOCGIFINDEX, &ifr);
+	ASSERT_GE(ret, 0)
+		TH_LOG("failed SIOCGIFINDEX: %d", errno);
+
+
+	addr.can_family = AF_CAN;
+	addr.can_ifindex = ifr.ifr_ifindex;
+
+	ret = bind(self->raw_sock, (struct sockaddr *)&addr, sizeof(addr));
+	ASSERT_EQ(ret, 0)
+		TH_LOG("failed bind CAN_RAW socket: %d", errno);
+
+	self->j1939_sock = socket(PF_CAN, SOCK_DGRAM, CAN_J1939);
+	ASSERT_GE(self->j1939_sock, 0)
+		TH_LOG("failed to create CAN_J1939 socket: %d", errno);
+
+	addr.can_addr.j1939.addr = SENDER_ADDR;
+	addr.can_addr.j1939.name = J1939_NO_NAME;
+	addr.can_addr.j1939.pgn = J1939_NO_PGN;
+
+	ret = bind(self->j1939_sock, (struct sockaddr *)&addr, sizeof(addr));
+	ASSERT_EQ(ret, 0)
+		TH_LOG("failed bind CAN_J1939 socket: %d", errno);
+
+	addr.can_addr.j1939.addr = RECEIVER_ADDR;
+	addr.can_addr.j1939.pgn = TEST_PGN;
+	ret = connect(self->j1939_sock, (struct sockaddr *)&addr, sizeof(addr));
+	ASSERT_EQ(ret, 0)
+		TH_LOG("failed connect CAN_J1939 socket: %d", errno);
+}
+
+FIXTURE_TEARDOWN(can_env)
+{
+	if (self->j1939_sock != -1)
+		close(self->j1939_sock);
+
+	if (self->raw_sock != -1)
+		close(self->raw_sock);
+}
+
+/* Test RTS/CTS transport without hold as baseline */
+TEST_F(can_env, test_no_hold)
+{
+	struct can_frame tx_frame = {
+		.can_id = RECEIVER_TP_CM_ID,
+		.len = 8,
+	};
+
+	memcpy(tx_frame.data, RESUME_IMMEDIATE_PAYLOAD, sizeof(RESUME_IMMEDIATE_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);
+
+	res = recv_payload(self->raw_sock, DATA_1_PAYLOAD, sizeof(DATA_1_PAYLOAD));
+	ASSERT_EQ(res, 0)
+		TH_LOG("Failed to receive DATA 1 as expeceted");
+
+	res = recv_payload(self->raw_sock, DATA_2_PAYLOAD, sizeof(DATA_2_PAYLOAD));
+	ASSERT_EQ(res, 0)
+		TH_LOG("Failed to receive DATA 2 as expeceted");
+
+	memcpy(tx_frame.data, EOMA_PAYLOAD, sizeof(EOMA_PAYLOAD));
+	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+	ASSERT_GT(res, 0)
+		TH_LOG("failed to send EOMA with raw sock: %d", errno);
+}
+
+/* Test holding RTS/CTS transport on first frame and resuming immediatley */
+TEST_F(can_env, test_hold_resume_immediate)
+{
+	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 300ms before sending the resume */
+	usleep(300000);
+
+	memcpy(tx_frame.data, RESUME_IMMEDIATE_PAYLOAD, sizeof(RESUME_IMMEDIATE_PAYLOAD));
+	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+	ASSERT_GT(res, 0)
+		TH_LOG("failed to send resume with raw sock: %d", errno);
+
+	res = recv_payload(self->raw_sock, DATA_1_PAYLOAD, sizeof(DATA_1_PAYLOAD));
+	ASSERT_EQ(res, 0)
+		TH_LOG("Failed to receive DATA 1 as expeceted");
+
+	res = recv_payload(self->raw_sock, DATA_2_PAYLOAD, sizeof(DATA_2_PAYLOAD));
+	ASSERT_EQ(res, 0)
+		TH_LOG("Failed to receive DATA 2 as expeceted");
+
+	memcpy(tx_frame.data, EOMA_PAYLOAD, sizeof(EOMA_PAYLOAD));
+	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+	ASSERT_GT(res, 0)
+		TH_LOG("failed to send EOMA with raw sock: %d", errno);
+}
+
+/* Test send hold in transport session and resuming */
+TEST_F(can_env, test_hold_resume)
+{
+	struct can_frame tx_frame = {
+		.can_id = RECEIVER_TP_CM_ID,
+		.len = 8,
+	};
+
+	memcpy(tx_frame.data, CTS_1_FRAME_PAYLOAD, sizeof(CTS_1_FRAME_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 cts(1) with raw sock: %d", errno);
+
+	res = recv_payload(self->raw_sock, DATA_1_PAYLOAD, sizeof(DATA_1_PAYLOAD));
+	ASSERT_EQ(res, 0)
+		TH_LOG("Failed to receive RTS as expeceted");
+
+	memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
+	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 300ms before sending the resume */
+	usleep(300000);
+
+	memcpy(tx_frame.data, RESUME_PAYLOAD, sizeof(RESUME_PAYLOAD));
+	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+	ASSERT_GT(res, 0)
+		TH_LOG("failed to send resume with raw sock: %d", errno);
+
+	res = recv_payload(self->raw_sock, DATA_2_PAYLOAD, sizeof(DATA_2_PAYLOAD));
+	ASSERT_EQ(res, 0)
+		TH_LOG("Failed to receive DATA 2 as expeceted");
+
+	memcpy(tx_frame.data, EOMA_PAYLOAD, sizeof(EOMA_PAYLOAD));
+	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+	ASSERT_GT(res, 0)
+		TH_LOG("failed to send EOMA with raw sock: %d", errno);
+}
+
+/* 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);
+}
+
+int main(int argc, char **argv)
+{
+	char *ifname = getenv("CANIF");
+
+	if (!ifname) {
+		printf("CANIF environment variable must contain the test interface\n");
+		return KSFT_FAIL;
+	}
+
+	strncpy(CANIF, ifname, sizeof(CANIF) - 1);
+
+	return test_harness_run(argc, argv);
+}
diff --git a/tools/testing/selftests/net/can/test_cts_hold.sh b/tools/testing/selftests/net/can/test_cts_hold.sh
new file mode 100755
index 000000000000..e69e9109245c
--- /dev/null
+++ b/tools/testing/selftests/net/can/test_cts_hold.sh
@@ -0,0 +1,45 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="
+	test_cts_hold
+"
+
+net_dir=$(dirname $0)/..
+source $net_dir/lib.sh
+
+export CANIF=${CANIF:-"vcan0"}
+BITRATE=${BITRATE:-500000}
+
+setup()
+{
+	if [[ $CANIF == vcan* ]]; then
+		ip link add name $CANIF type vcan || exit $ksft_skip
+	else
+		ip link set dev $CANIF type can bitrate $BITRATE || exit $ksft_skip
+	fi
+	ip link set dev $CANIF up
+	pwd
+}
+
+cleanup()
+{
+	ip link set dev $CANIF down
+	if [[ $CANIF == vcan* ]]; then
+		ip link delete $CANIF
+	fi
+}
+
+test_cts_hold()
+{
+	./test_cts_hold
+	check_err $?
+	log_test "test_cts_hold"
+}
+
+trap cleanup EXIT
+setup
+
+tests_run
+
+exit $EXIT_STATUS
-- 
2.54.0

Re: [PATCH v3 1/2] Fix J1939 implementation not handling holds correctly

[Oleksij Rempel]

Hi Alexander,

Sorry I lost the track of this patches.

Can you please take a look here:
https://sashiko.dev/#/patchset/20260525190948.42461-1-alexander.hoelzl%40gmx.net

You can ignore the warning in net/can/j1939/transport.c
I guess it is protocol specific issue (potentially can be commented in
the source code), if you have other opinion, please share :)

There are some typos in the tests, can you please address them.

On Mon, May 25, 2026 at 09:08:48PM +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.
> 
> To do so the behavior of j1939_xtp_rx_cts_one was modified to allow the
> handling of a hold. The previous sanity check was removed as it only
> guarded against a flood of consecutive CTS, but prevented the hold
> from working correctly. This patch changes this behavior to allow
> for consectuive CTS to enable holds. An additional sanity check
> has been added which prevents requsts of already transferred and
> acked packets. In this case the kernel will abort immediately
> instead of going into a timeout.
> 
> 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>
> ---
> I've integrated the comments you've send. In one of the comments you've 
> referenced a wrong section of the J1939 standard. For the hold message
> you've referenced SAE J1939-21 2001 - 5.10.2.4 Connection Closure,
> but it should have been SAE 5.102.2.3 Data Transfer. That I have
> changed. Otherwise everything should be according to your comments :)
> 
>  net/can/j1939/transport.c | 68 +++++++++++++++++++++++++++++----------
>  1 file changed, 51 insertions(+), 17 deletions(-)
> 
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index df93d57907da..e2c79df7b04e 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -32,6 +32,13 @@
>  #define J1939_ETP_CMD_EOMA 0x17
>  #define J1939_ETP_CMD_ABORT 0xff
>  
> +/* Time until session invalidation upon reception of a hold message.
> + * Corresponds to T4 in the specification.
> + * See ISO 11783-3 2018 - 5.10.3.5 Connection closure
> + * and SAE J1939-21 2022 - 5.10.2.4 Connection Closure
> + */
> +#define J1939_CTS_HOLD_TIMEOUT_MS 1050
> +
>  enum j1939_xtp_abort {
>  	J1939_XTP_NO_ABORT = 0,
>  	J1939_XTP_ABORT_BUSY = 1,
> @@ -1428,6 +1435,16 @@ j1939_xtp_rx_eoma(struct j1939_priv *priv, struct sk_buff *skb,
>  	j1939_session_put(session);
>  }
>  
> +/* See:
> + * SAE J1939-21 2022 - 5.10.2.3 Data Transfer
> + * 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 +1459,28 @@ 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;
> +	session->last_cmd = dat[0];
> +
> +	if (j1939_cts_is_hold(skb)) {
> +		/* The originator should abort the session after T4 (=< 1050ms):
> +		 *   SAE J1939-21 2022 - 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.
> +		 *
> +		 */
> +		if (session->transmission)
> +			j1939_session_txtimer_cancel(session);
> +
> +		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 +1493,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 +1507,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);
> +	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;
>  
> -- 
> 2.54.0
> 
> 
> 

-- 
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 v3 1/2] Fix J1939 implementation not handling holds correctly

[Hölzl, Alexander]

Hi Oleksij,

Am 10.06.2026 um 11:42 schrieb Oleksij Rempel:
> Hi Alexander,
> 
> Sorry I lost the track of this patches.

No worries!

> Can you please take a look here:
> https://sashiko.dev/#/patchset/20260525190948.42461-1-alexander.hoelzl%40gmx.net
> 
> You can ignore the warning in net/can/j1939/transport.c
> I guess it is protocol specific issue (potentially can be commented in
> the source code), if you have other opinion, please share :)
> 

The bot is right and after looking through the specs once again
there are requirements mentioned regarding retransmission
requests. In 5.10.3.2 Connection Mode Clear to Send (CTS):
...
When the CTS message is used to request the retransmission of data 
packet(s), it is recommended not to use more than two retransmit 
requests. When this limit is reached, a connection abort with abort 
reason 5 from Table 6 shall be sent.
...

This paragraph to me sounds more like a requirement for the responder to
stop requesting retransmissions.


Second there is also this:
5.12.3 Device Response Time and Timeout Defaults
...
Number of request retries = 2 (3 requests total); this includes the 
situation where the CTS is used to request the retransmission of data 
packet(s). If the retransmit request limit is reached, then the 
connection abort shall be sent with abort reason 5 from Table 6.
...
This sounds a bit more generic and not related specifically to responder 
or originator. I did not see a mention of in any of those requirements
in the compliance spec J1939-82 however...

Do you think I should add a counter for retransmit requests?
If yes should it also apply to holds?

> There are some typos in the tests, can you please address them.
> 
Sure!> On Mon, May 25, 2026 at 09:08:48PM +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.
>>
>> To do so the behavior of j1939_xtp_rx_cts_one was modified to allow the
>> handling of a hold. The previous sanity check was removed as it only
>> guarded against a flood of consecutive CTS, but prevented the hold
>> from working correctly. This patch changes this behavior to allow
>> for consectuive CTS to enable holds. An additional sanity check
>> has been added which prevents requsts of already transferred and
>> acked packets. In this case the kernel will abort immediately
>> instead of going into a timeout.
>>
>> Fix J1939 RTS/CTS session not being able to resume from hold.
>> Replace hardcoded timeout with define.
>> Add CTS hold behavior tests.

...
In addition just want to mention this check I've introduced, which 
prevents requesting packets which the responder has already acknowledged 
in a previous CTS:>> -	/* set packet counters only when not CTS(0) */
>> +	if (session->pkt.tx_acked >= pkt) {
>> +		err = J1939_XTP_ABORT_DUP_SEQ;
>> +		goto out_session_cancel;
>> +	}
>> +
I couldn't find this requirement in J1939-21 but the compliance testing 
spec J1939-82 mentions it in table A7 row 6:

	Verify DUT transmits a TP.Conn_Abort when 'Next packet number
	to be sent' in TP.CM_CTS message:
	- is less than the 'Next packet number to be sent' in 	
	  previous TP.CM_CTS

Should I add this as a comment as well?

Regards,
Alex

Re: [PATCH v3 1/2] Fix J1939 implementation not handling holds correctly

[Oleksij Rempel]

Hi Alexander,

On Fri, Jun 19, 2026 at 01:13:00PM +0200, Hölzl, Alexander wrote:
> Hi Oleksij,
> 
> Am 10.06.2026 um 11:42 schrieb Oleksij Rempel:
> > Hi Alexander,
> > 
> > Sorry I lost the track of this patches.
> 
> No worries!
> 
> > Can you please take a look here:
> > https://sashiko.dev/#/patchset/20260525190948.42461-1-alexander.hoelzl%40gmx.net
> > 
> > You can ignore the warning in net/can/j1939/transport.c
> > I guess it is protocol specific issue (potentially can be commented in
> > the source code), if you have other opinion, please share :)
> > 
> 
> The bot is right and after looking through the specs once again
> there are requirements mentioned regarding retransmission
> requests. In 5.10.3.2 Connection Mode Clear to Send (CTS):
> ...
> When the CTS message is used to request the retransmission of data
> packet(s), it is recommended not to use more than two retransmit requests.
> When this limit is reached, a connection abort with abort reason 5 from
> Table 6 shall be sent.
> ...
> 
> This paragraph to me sounds more like a requirement for the responder to
> stop requesting retransmissions.
> 
> 
> Second there is also this:
> 5.12.3 Device Response Time and Timeout Defaults
> ...
> Number of request retries = 2 (3 requests total); this includes the
> situation where the CTS is used to request the retransmission of data
> packet(s). If the retransmit request limit is reached, then the connection
> abort shall be sent with abort reason 5 from Table 6.
> ...
> This sounds a bit more generic and not related specifically to responder or
> originator. I did not see a mention of in any of those requirements
> in the compliance spec J1939-82 however...
> 
> Do you think I should add a counter for retransmit requests?
> If yes should it also apply to holds?

Yes, otherwise it seems to bind system resources for no good reason.
Please also add comments in the code explaining this decision.

> > There are some typos in the tests, can you please address them.
> > 
> Sure!> On Mon, May 25, 2026 at 09:08:48PM +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.
> > > 
> > > To do so the behavior of j1939_xtp_rx_cts_one was modified to allow the
> > > handling of a hold. The previous sanity check was removed as it only
> > > guarded against a flood of consecutive CTS, but prevented the hold
> > > from working correctly. This patch changes this behavior to allow
> > > for consectuive CTS to enable holds. An additional sanity check
> > > has been added which prevents requsts of already transferred and
> > > acked packets. In this case the kernel will abort immediately
> > > instead of going into a timeout.
> > > 
> > > Fix J1939 RTS/CTS session not being able to resume from hold.
> > > Replace hardcoded timeout with define.
> > > Add CTS hold behavior tests.
> 
> ...
> In addition just want to mention this check I've introduced, which prevents
> requesting packets which the responder has already acknowledged in a
> previous CTS:>> -	/* set packet counters only when not CTS(0) */
> > > +	if (session->pkt.tx_acked >= pkt) {
> > > +		err = J1939_XTP_ABORT_DUP_SEQ;
> > > +		goto out_session_cancel;
> > > +	}
> > > +
> I couldn't find this requirement in J1939-21 but the compliance testing spec
> J1939-82 mentions it in table A7 row 6:
> 
> 	Verify DUT transmits a TP.Conn_Abort when 'Next packet number
> 	to be sent' in TP.CM_CTS message:
> 	- is less than the 'Next packet number to be sent' in 	
> 	  previous TP.CM_CTS
> 
> Should I add this as a comment as well?

Yes please. 

Thank you for your work!

Best Regards,
Oleksij
-- 
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 |