[PATCH v4 1/3] j1939: restrict amount of consecutive retransmission requests

[PATCH v4 1/3] j1939: restrict amount of consecutive retransmission requests

[Alexander Hölzl]

In J1939 segmented messages the receiver of a segmented message
can request the retranmission of sent data frames. In the current
implementation there was no limit on the amount of consecutive
retransmission requests which were allowed. The standard states
that after two retransmission requests (so three transmissions
in total) the connection should be aborted with reason 5,
'maximum retransmit request limit reached'.
See SAE J1939-22 2025: 5.10.3.2 Connection Mode Clear to Send
and 5.12.3 Device Response Time and Timeout Defaults.

This commit introduces a retransmit counter and aborts the segmented
data transfer if the retransmit limit is reached.

Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
---
Hello,
sorry I was also a bit slow, I survived the heatwave now I can do
something again :)

I've added an additional commit to introduce the retransmit counting
as discussed in the previous review. I've added additional tests to
test the retransmit abort as well as the abort on requesting an 
already acked frame.
I've also renamed the tests from cts_hold to 
rx_cts as they now also test other parts of the rx_cts path so I though
they should have a more generic name.

I've added the comments and fixed the spelling mistakes, I hope I did
not add any new ones.

 net/can/j1939/j1939-priv.h |  2 ++
 net/can/j1939/transport.c  | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index 81f58924b4ac..16867d803092 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -285,6 +285,8 @@ struct j1939_session {
 		unsigned int block;
 		/* dpo - ETP.CM_DPO, Data Packet Offset */
 		unsigned int dpo;
+		/* retransmits - amount of received retransmit requests (including holds) */
+		unsigned int retransmits;
 	} pkt;
 	struct hrtimer txtimer, rxtimer;
 };
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index df93d57907da..6f999b18bbca 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -32,6 +32,14 @@
 #define J1939_ETP_CMD_EOMA 0x17
 #define J1939_ETP_CMD_ABORT 0xff
 
+/* Maximum amount of transmission attempts for a given packet number to
+ * be sent. According to SAE J1939-21 2022 - 5.12.3 Device Response Time and
+ * Timeout Defaults there should be no more than 2 retries (3 requests in total)
+ * before the connection is aborted with reason 5 which corresponds to
+ * J1939_XTP_ABORT_FAULT in this implementation.
+ */
+#define J1939_CTS_MAX_NUM_TRANSMITS 3
+
 enum j1939_xtp_abort {
 	J1939_XTP_NO_ABORT = 0,
 	J1939_XTP_ABORT_BUSY = 1,
@@ -1457,6 +1465,19 @@ 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;
 
+	/* If the 'next packet number to be sent' in the CTS is smaller or
+	 * equal to an already sent packet it is a retransmit request.
+	 */
+	if (session->pkt.tx >= pkt) {
+		session->pkt.retransmits++;
+		if (session->pkt.retransmits >= J1939_CTS_MAX_NUM_TRANSMITS) {
+			err = J1939_XTP_ABORT_FAULT;
+			goto out_session_cancel;
+		}
+	} else {
+		session->pkt.retransmits = 0;
+	}
+
 	/* set packet counters only when not CTS(0) */
 	session->pkt.tx_acked = pkt - 1;
 	j1939_session_skb_drop_old(session);
@@ -1669,6 +1690,7 @@ j1939_session *j1939_xtp_rx_rts_session_new(struct j1939_priv *priv,
 
 	session->pkt.rx = 0;
 	session->pkt.tx = 0;
+	session->pkt.retransmits = 0;
 
 	session->tskey = priv->rx_tskey++;
 	j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_RTS);
-- 
2.55.0

[PATCH v4 2/3] j1939: fix 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>
---
 net/can/j1939/transport.c | 70 +++++++++++++++++++++++++++++----------
 1 file changed, 53 insertions(+), 17 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 6f999b18bbca..ae0dc0a0168a 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -39,6 +39,12 @@
  * J1939_XTP_ABORT_FAULT in this implementation.
  */
 #define J1939_CTS_MAX_NUM_TRANSMITS 3
+/* 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,
@@ -1436,6 +1442,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)
 {
@@ -1450,9 +1466,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];
+
+	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)
@@ -1465,10 +1499,18 @@ 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;
 
+	/* According to J1939-82 table A7 row 6 the connection should be aborted
+	 * if the 'next packet number to be sent' in the CTS message is less than
+	 * the 'next packet number to be sent' in the previous message.
+	 */
+	if (session->pkt.tx_acked >= pkt) {
+		err = J1939_XTP_ABORT_DUP_SEQ;
+		goto out_session_cancel;
+	}
 	/* If the 'next packet number to be sent' in the CTS is smaller or
 	 * equal to an already sent packet it is a retransmit request.
 	 */
-	if (session->pkt.tx >= pkt) {
+	else if (session->pkt.tx >= pkt) {
 		session->pkt.retransmits++;
 		if (session->pkt.retransmits >= J1939_CTS_MAX_NUM_TRANSMITS) {
 			err = J1939_XTP_ABORT_FAULT;
@@ -1488,19 +1530,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.55.0

[PATCH v4 3/3] j1939: add J1939 rx CTS tests

[Alexander Hölzl]

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

The tests verify that the J1939 is able to handle received CTS messages
correctly. The following scenarios are tested:

1. Normal transmission of a segmented message
2. Transmission can be resumed 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.
3. Transmission is aborted upon request of already acked message
4. Transmission is aborted if too many retransmit requests are sent

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 +
 tools/testing/selftests/net/can/test_rx_cts.c | 450 ++++++++++++++++++
 .../testing/selftests/net/can/test_rx_cts.sh  |  45 ++
 5 files changed, 503 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/net/can/test_rx_cts.c
 create mode 100755 tools/testing/selftests/net/can/test_rx_cts.sh

diff --git a/tools/testing/selftests/net/can/.gitignore b/tools/testing/selftests/net/can/.gitignore
index 764a53fc837f..e42de065854b 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_rx_cts
\ No newline at end of file
diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile
index 5b82e60a03e7..df0f4389c974 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_rx_cts.sh \
 
-TEST_GEN_FILES := test_raw_filter
+TEST_GEN_FILES := \
+	test_raw_filter \
+	test_rx_cts \
 
 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_rx_cts.c b/tools/testing/selftests/net/can/test_rx_cts.c
new file mode 100644
index 000000000000..1b437792d677
--- /dev/null
+++ b/tools/testing/selftests/net/can/test_rx_cts.c
@@ -0,0 +1,450 @@
+// 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 <poll.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
+
+/* Segmented 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};
+
+/* Connection abort payload sent on connection timeout */
+const uint8_t ABORT_TIMEOUT_PAYLOAD[] = {0xFF, 0x03, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
+/* Connection abort payload sent on reaching retransmit request limit */
+const uint8_t ABORT_RETRANSMIT_PAYLOAD[] = {0xFF, 0x05, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
+/* Connection abort payload sent due to duplicate sequence number */
+const uint8_t ABORT_DUP_SEQ_PAYLOAD[] = {0xFF, 0x08, 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 normal RTS/CTS transport 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 expected");
+
+	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+	ASSERT_GT(res, 0)
+		TH_LOG("failed to CTS 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 expected");
+
+	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 expected");
+
+	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 immediately */
+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 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);
+
+	/* 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 expected");
+
+	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 expected");
+
+	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 expected");
+
+	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 DATA 1 as expected");
+
+	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 expected");
+
+	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);
+}
+
+/* Test that the connection is aborted after the retransmission of
+ * an already acked frame is requested
+ */
+TEST_F(can_env, test_abort_after_invalid_retransmit_request)
+{
+	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 expected");
+
+	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 DATA 1 as expected");
+
+	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 expected");
+
+	/* Now send a CTS for an already acked frame */
+	memcpy(tx_frame.data, CTS_1_FRAME_PAYLOAD, sizeof(CTS_1_FRAME_PAYLOAD));
+	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+	ASSERT_GT(res, 0)
+		TH_LOG("failed to send second cts(1) with raw sock: %d", errno);
+
+	res = recv_payload(self->raw_sock, ABORT_DUP_SEQ_PAYLOAD, sizeof(DATA_2_PAYLOAD));
+	ASSERT_EQ(res, 0)
+		TH_LOG("Failed to receive abort due to duplicate sequence number as expected");
+}
+
+TEST_F(can_env, test_abort_after_too_many_retransmits)
+{
+	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 expected");
+
+	/* Send the valid retransmit requests */
+	for (int i = 0; i < 3; i++) {
+		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 DATA 1 as expected");
+	}
+
+	/* This is one retransmit request too many */
+	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, ABORT_RETRANSMIT_PAYLOAD, sizeof(ABORT_RETRANSMIT_PAYLOAD));
+	ASSERT_EQ(res, 0)
+		TH_LOG("Failed to receive abort too many retransmit requests as expected");
+}
+
+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_rx_cts.sh b/tools/testing/selftests/net/can/test_rx_cts.sh
new file mode 100755
index 000000000000..b2064ea7851e
--- /dev/null
+++ b/tools/testing/selftests/net/can/test_rx_cts.sh
@@ -0,0 +1,45 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="
+	test_rx_cts
+"
+
+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_rx_cts()
+{
+	./test_rx_cts
+	check_err $?
+	log_test "test_rx_cts
+}
+
+trap cleanup EXIT
+setup
+
+tests_run
+
+exit $EXIT_STATUS
-- 
2.55.0

Re: [PATCH v4 3/3] j1939: add J1939 rx CTS tests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing closing quote in bash script `test_rx_cts.sh` causing unexpected EOF syntax error, breaking the test.
- [Low] Incorrect `sizeof()` argument used when validating the received abort payload.
- [Low] Strict timing assertions might cause flaky test failures on heavily loaded CI systems.
--

--- Patch [3]: [PATCH v4 3/3] j1939: add J1939 rx CTS tests ---
commit 21e063d80602f46f314c362808e394f235e646f9
Author: Alexander Hölzl <alexander.hoelzl@gmx.net>

j1939: add J1939 rx CTS tests

This commit adds tests to verify the correct behavior of received J1939
CTS messages. It covers normal segmented transmission, transmission resuming
after a hold, aborting on requested already acked messages, and aborting on
too many retransmit requests.

> diff --git a/tools/testing/selftests/net/can/test_rx_cts.c b/tools/testing/selftests/net/can/test_rx_cts.c
> new file mode 100644
> index 0000000000000..1b437792d6772
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_rx_cts.c
[ ... ]
> +	/*
> +	 * 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);

[Severity: Low]
This isn't a bug, but could this strict timing assertion cause flaky test
failures?

An upper bound of 1500ms provides only a 450ms window for the 1050ms kernel
timeout, which might be easily exceeded due to thread scheduling delays under
heavy load on oversubscribed or virtualized CI runners.

[ ... ]
> +	/* Now send a CTS for an already acked frame */
> +	memcpy(tx_frame.data, CTS_1_FRAME_PAYLOAD, sizeof(CTS_1_FRAME_PAYLOAD));
> +	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
> +	ASSERT_GT(res, 0)
> +		TH_LOG("failed to send second cts(1) with raw sock: %d", errno);
> +
> +	res = recv_payload(self->raw_sock, ABORT_DUP_SEQ_PAYLOAD, sizeof(DATA_2_PAYLOAD));

[Severity: Low]
Is this the correct size constant to use here?

It appears sizeof(ABORT_DUP_SEQ_PAYLOAD) should be used instead of
sizeof(DATA_2_PAYLOAD), even though both evaluate to 8 bytes.

[ ... ]
> diff --git a/tools/testing/selftests/net/can/test_rx_cts.sh b/tools/testing/selftests/net/can/test_rx_cts.sh
> new file mode 100755
> index 0000000000000..b2064ea7851e2
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_rx_cts.sh
[ ... ]
> +test_rx_cts()
> +{
> +	./test_rx_cts
> +	check_err $?
> +	log_test "test_rx_cts

[Severity: Medium]
Does this missing closing double quote cause an unexpected EOF syntax error
during shell parsing?

This would completely prevent the test script from running.

> +}
> +
> +trap cleanup EXIT

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

Re: [PATCH v4 2/3] j1939: fix implementation not handling holds correctly

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unprotected concurrent access and modification of J1939 session state variables (`pkt.tx`) between the RX softirq and the TX timer softirq, leading to potential silent data corruption.
--

--- Patch [2]: [PATCH v4 2/3] j1939: fix implementation not handling holds correctly ---
commit eff7a7e71fc39cb6fe94b38a7c7c2264d3d9f7e4
Author: Alexander Hölzl <alexander.hoelzl@gmx.net>

j1939: fix implementation not handling holds correctly

This commit modifies the behavior of j1939_xtp_rx_cts_one to allow the
handling of a hold message. It cancels the TX timer and sets the RX
timeout properly while adding a check against requests of already
transferred and acked packets.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 6f999b18bbca5..ae0dc0a0168aa 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -1488,19 +1530,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;

[Severity: High]
This is a pre-existing issue, but I have a question about the concurrent
access to session->pkt.tx.

Can modifying session->pkt.tx here in the RX softirq context race with
readers in the TX timer softirq context?

If the TX timer executes j1939_session_tx_dat() concurrently, it reads
session->pkt.tx multiple times within its transmission loop without locking
or READ_ONCE():

    dat[0] = session->pkt.tx - session->pkt.dpo + 1;
    offset = (session->pkt.tx * 7) - se_skcb->offset;

Because these softirqs can run concurrently on different CPUs, might the
compiler reload session->pkt.tx between the two statements? 

If the RX path overwrites pkt.tx in that microscopic window, could
j1939_session_tx_dat() transmit a frame containing the payload data of one
packet but the sequence number of another, leading to silent data corruption
on the CAN network?

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

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

Re: [PATCH v4 1/3] j1939: restrict amount of consecutive retransmission requests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Flawed retransmit tracking logic causes both spurious connection aborts and counter resets.
- [High] CTS hold messages bypass the retransmit limit, enabling indefinite connection persistence (DoS).
--

--- Patch [1]: [PATCH v4 1/3] j1939: restrict amount of consecutive retransmission requests ---
commit e2e0a35fca4d64ebcdd174aa2094ae405f7ecced
Author: Alexander Hölzl <alexander.hoelzl@gmx.net>

j1939: restrict amount of consecutive retransmission requests

This commit introduces a retransmit counter and aborts the segmented
data transfer if the retransmit limit is reached.

> diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
> index 81f58924b4acd..16867d8030922 100644
> --- a/net/can/j1939/j1939-priv.h
> +++ b/net/can/j1939/j1939-priv.h
[ ... ]
> @@ -285,6 +285,8 @@ struct j1939_session {
>  		unsigned int block;
>  		/* dpo - ETP.CM_DPO, Data Packet Offset */
>  		unsigned int dpo;
> +		/* retransmits - amount of received retransmit requests (including holds) */
> +		unsigned int retransmits;

[Severity: High]
If this counter is intended to include holds, will they actually be counted?

Looking at j1939_xtp_rx_cts_one() in transport.c, if j1939_cts_is_hold(skb)
is true, the function sets the hold timeout and returns early:

	if (j1939_cts_is_hold(skb)) {
		...
		j1939_tp_set_rxtimeout(session, J1939_CTS_HOLD_TIMEOUT_MS);
		return;
	}

Because this early return bypasses the new retransmits tracking logic,
could a peer keep a session open indefinitely by continuously sending
CTS hold messages?

>  	} pkt;
>  	struct hrtimer txtimer, rxtimer;
>  };
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index df93d57907da7..6f999b18bbca5 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
[ ... ]
> @@ -1457,6 +1465,19 @@ 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;
>  
> +	/* If the 'next packet number to be sent' in the CTS is smaller or
> +	 * equal to an already sent packet it is a retransmit request.
> +	 */
> +	if (session->pkt.tx >= pkt) {
> +		session->pkt.retransmits++;

[Severity: High]
Does this logic reliably track retransmits, given that session->pkt.tx
is updated asynchronously by txtimer?

If a peer drops a packet and requests the next one (forward progress),
but txtimer has already sent past that requested packet number, wouldn't
session->pkt.tx >= pkt evaluate to true? This seems like it could
spuriously increment the counter and abort valid transfers over lossy links.

Conversely, if a peer sends a duplicate CTS request before txtimer has sent
the newly requested packet, session->pkt.tx would still be pkt - 1. Wouldn't
that evaluate to false and cause the else block to reset the retransmits
counter to 0?

> +		if (session->pkt.retransmits >= J1939_CTS_MAX_NUM_TRANSMITS) {
> +			err = J1939_XTP_ABORT_FAULT;
> +			goto out_session_cancel;
> +		}
> +	} else {
> +		session->pkt.retransmits = 0;
> +	}
> +

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