public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Fix J1939 implementation not handling holds correctly
@ 2026-05-04 22:18 Alexander Hölzl
  2026-05-06 13:21 ` Oleksij Rempel
  0 siblings, 1 reply; 2+ messages in thread
From: Alexander Hölzl @ 2026-05-04 22:18 UTC (permalink / raw)
  To: o.rempel; +Cc: robin, linux-kernel, kernel, linux-can, 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.
Additionally the incorrect timeout for a held open connection was
changed to 1050ms.
Some simple tests for the general protocol behavior have been added.

This is just an initial draft I'm just looking for some feedback
if the general setup is acceptable.

Also I still have some points I'm not sure about.
1. There is no check for the reception of a CTS during an ongoing data
   transfer. According to the standard it is illegal, but I dont think
   the implementation cares about it. J1939_XTP_ABORT_GENERIC is never
   referenced and the previous check I removed only really prevented
   consecutive CTS which are legal if they are holds. Should I add a
   check for this? This seems to be a slightly different problem so I
   did not address it.

2. Should userspace be notified about holds? I don't think the standard
   gives a maximum 'hold open time' so in theory it could be held
   indefinetly. This might be a situation a user could be intrested in
   even though it is rather theoretical.

Please tell me what you think.

Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
---
 net/can/j1939/transport.c                     |  50 ++--
 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 | 269 ++++++++++++++++++
 .../selftests/net/can/test_cts_hold.sh        |  45 +++
 6 files changed, 355 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

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index df93d57907da..e53a9fbe82a9 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1442,9 +1442,27 @@ 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];
+
+	/* TODO
+	 *according to the standard it is illegal to send a CTS while data transfer is ongoing.
+	 * If a CTS is sent while data transfer is ongoing an abort with code 4 should be sent.
+	 * The previous check I removed was simply (session->last_cmd == dat[0]).
+	 * This did not check for a invalid CTS after some data frames had already been
+	 * sent but before all of them were sent.
+	 * So CTS(0), CTS(5) was illegal. CTS(5), DAT, CTS(5) was legal,
+	 * even though only one data frame has been sent instead of 5.
+	 * Should a check for this also be added?
+	 */
+	if (!dat[1]) {
+		/* CTS(0) */
+		if (session->transmission) {
+			/* TODO notify error queue about hold messages? */
+			j1939_session_txtimer_cancel(session);
+		}
+		j1939_tp_set_rxtimeout(session, 1050);
+		netdev_dbg(session->priv->ndev, "%s: 0x%p received CTS hold\n", __func__, session);
+		return;
 	}
 
 	if (session->skcb.addr.type == J1939_ETP)
@@ -1457,7 +1475,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 +1489,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;
 
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..a36efb099522
--- /dev/null
+++ b/tools/testing/selftests/net/can/test_cts_hold.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/ioctl.h>
+#include <sys/time.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)
+
+//10 millisecond timeout for raw socket
+#define RAW_RCVTIMEOUT_US 10000
+
+/* 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};
+
+/* 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) {
+		perror("failed to recv can raw frame");
+		return 1;
+	}
+
+	if (rx_frame.len != len) {
+		printf("received data length does not match expected value\n");
+		return 1;
+	}
+
+	if (memcmp(rx_frame.data, payload, len)) {
+		printf("received data does not match expected value\n");
+		return 1;
+	}
+
+	return 0;
+}
+
+
+FIXTURE(can_env)
+{
+	int j1939_sock;
+	int raw_sock;
+};
+
+FIXTURE_SETUP(can_env)
+{
+	struct sockaddr_can addr;
+	struct ifreq ifr;
+	struct timeval raw_rcvtimeout = {.tv_sec = 0, .tv_usec = RAW_RCVTIMEOUT_US};
+	int s, ret;
+
+	s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
+	ASSERT_GE(s, 0)
+		TH_LOG("failed to create CAN_RAW socket: %d", errno);
+
+	strncpy(ifr.ifr_name, CANIF, sizeof(ifr.ifr_name));
+	ret = ioctl(s, 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(s, (struct sockaddr *)&addr, sizeof(addr));
+	ASSERT_EQ(ret, 0)
+		TH_LOG("failed bind CAN_RAW socket: %d", errno);
+
+	self->raw_sock = s;
+
+	s = socket(PF_CAN, SOCK_DGRAM, CAN_J1939);
+	ASSERT_GE(s, 0)
+		TH_LOG("failed to create CAN_J1939 socket: %d", errno);
+
+
+	ret = setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &raw_rcvtimeout, sizeof(raw_rcvtimeout));
+
+	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(s, (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(s, (struct sockaddr *)&addr, sizeof(addr));
+	ASSERT_EQ(ret, 0)
+		TH_LOG("failed connect CAN_J1939 socket: %d", errno);
+
+	self->j1939_sock = s;
+}
+
+FIXTURE_TEARDOWN(can_env)
+{
+	close(self->j1939_sock);
+	close(self->raw_sock);
+}
+
+/* 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");
+}
+
+/* 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_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_2_PAYLOAD, sizeof(DATA_2_PAYLOAD));
+	ASSERT_EQ(res, 0)
+		TH_LOG("Failed to receive DATA 2 as expeceted");
+
+}
+
+/* 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 1100ms before sending the resume */
+	usleep(1050);
+
+	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");
+
+
+}
+
+
+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


^ permalink raw reply related	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-05-06 13:21 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-04 22:18 [PATCH] Fix J1939 implementation not handling holds correctly Alexander Hölzl
2026-05-06 13:21 ` Oleksij Rempel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox