can: bcm: fix CAN frame rx/tx statistics

PATCH Superseded Oliver Hartkopp 2 participants 2 messages Jun 10

Subsystems (2)

CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2026-06-10 09:34 UTC #0 lore raw

PATCH 1/1 can: bcm: fix CAN frame rx/tx statistics

KCSAN detected a data race within the bcm_rx_handler() when two CAN frames have been simultaneously received and processed in a single rx op by two different CPUs.

Use atomic operations with (signed) long data types to access the statistics in the hot path to fix the KCSAN complaint.

Additionally simplify the update and check of statistics overflow by using the atomic operations in a separate bcm_update_stats() function.

Fixes ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")

Signed-off-by Oliver Hartkopp <[email protected]>

---
 net/can/bcm.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

net/can/bcm.c +23 ~~-14~~

index 658c4a7a55df..d3d1ed7fd862 100644
@@ -106,11 +106,11 @@ struct bcm_op {
 	struct list_head list;
 	struct rcu_head rcu;
 	int ifindex;
 	canid_t can_id;
 	u32 flags;
-	unsigned long frames_abs, frames_filtered;
+	atomic_long_t frames_abs, frames_filtered;
 	struct bcm_timeval ival1, ival2;
 	struct hrtimer timer, thrtimer;
 	ktime_t rx_stamp, kt_ival1, kt_ival2, kt_lastmsg;
 	int rx_ifindex;
 	int cfsiz;

@@ -223,14 +223,17 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(op, &bo->rx_ops, list) {
 
-		unsigned long reduction;
+		long reduction, frames_filtered, frames_abs;
+
+		frames_filtered = atomic_long_read(&op->frames_filtered);
+		frames_abs = atomic_long_read(&op->frames_abs);
 
 		/* print only active entries & prevent division by zero */
-		if (!op->frames_abs)
+		if (!frames_abs)
 			continue;
 
 		seq_printf(m, "rx_op: %03X %-5s ", op->can_id,
 			   bcm_proc_getifname(net, ifname, op->ifindex));

@@ -248,13 +251,13 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 		if (op->kt_ival2)
 			seq_printf(m, "thr=%lld ",
 				   (long long)ktime_to_us(op->kt_ival2));
 
 		seq_printf(m, "# recv %ld (%ld) => reduction: ",
-			   op->frames_filtered, op->frames_abs);
+			   frames_filtered, frames_abs);
 
-		reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
+		reduction = 100 - (frames_filtered * 100) / frames_abs;
 
 		seq_printf(m, "%s%ld%%\n",
 			   (reduction == 100) ? "near " : "", reduction);
 	}

@@ -274,20 +277,30 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 
 		if (op->kt_ival2)
 			seq_printf(m, "t2=%lld ",
 				   (long long)ktime_to_us(op->kt_ival2));
 
-		seq_printf(m, "# sent %ld\n", op->frames_abs);
+		seq_printf(m, "# sent %ld\n",
+			   atomic_long_read(&op->frames_abs));
 	}
 	seq_putc(m, '\n');
 
 	rcu_read_unlock();
 
 	return 0;
 }
 #endif /* CONFIG_PROC_FS */
 
+static void bcm_update_stats(struct bcm_op *op)
+{
+	/* prevent statistics overflow */
+	if (atomic_long_inc_return(&op->frames_abs) > LONG_MAX / 100) {
+		atomic_long_set(&op->frames_filtered, 0);
+		atomic_long_set(&op->frames_abs, 0);
+	}
+}
+
 /*
  * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
  *              of the given bcm tx op
  */
 static void bcm_can_tx(struct bcm_op *op)

@@ -334,11 +347,11 @@ static void bcm_can_tx(struct bcm_op *op)
 
 	/* update currframe and count under lock protection */
 	spin_lock_bh(&op->bcm_tx_lock);
 
 	if (!err)
-		op->frames_abs++;
+		bcm_update_stats(op);
 
 	op->currframe++;
 
 	/* reached last frame? */
 	if (op->currframe >= op->nframes)

@@ -487,16 +500,12 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
  */
 static void bcm_rx_changed(struct bcm_op *op, struct canfd_frame *data)
 {
 	struct bcm_msg_head head;
 
-	/* update statistics */
-	op->frames_filtered++;
-
-	/* prevent statistics overflow */
-	if (op->frames_filtered > ULONG_MAX/100)
-		op->frames_filtered = op->frames_abs = 0;
+	/* update statistics (frames_filtered <= frames_abs) */
+	atomic_long_inc(&op->frames_filtered);
 
 	/* this element is not throttled anymore */
 	data->flags &= ~RX_THR;
 
 	memset(&head, 0, sizeof(head));

@@ -733,11 +742,11 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 	/* save rx timestamp */
 	op->rx_stamp = skb->tstamp;
 	/* save originator for recvfrom() */
 	op->rx_ifindex = skb->dev->ifindex;
 	/* update statistics */
-	op->frames_abs++;
+	bcm_update_stats(op);
 
 	if (op->flags & RX_RTR_FRAME) {
 		/* send reply for RTR-request (placed in op->frames[0]) */
 		bcm_can_tx(op);
 		return;

Signature · 1 line

2.53.0

[email protected]

AI review

2026-06-10 09:46 UTC #1 lore raw

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

New issues:
- [Medium] Logic flaw in statistics overflow handling breaks invariant and causes negative reduction percentages.

Pre-existing issues:
- [High] Data race on CAN message payloads, timestamps, and interface indices during concurrent frame reception.
- [High] Use-After-Free due to a timer teardown race during BCM RX op deletion.
- [High] Missing RCU memory barriers when modifying operation lists lead to potential uninitialized memory accesses. --

commit 6493cba39cc176386238cda012e3d42c9808c07e
Author: Oliver Hartkopp <[email protected]>

can: bcm: fix CAN frame rx/tx statistics

This commit fixes a KCSAN-detected data race in bcm_rx_handler() when multiple CPUs process CAN frames simultaneously. It converts the statistics to atomic types and centralizes the overflow handling.

Quoted 4 lines

net/can/bcm.c +0 -0

index a4bef2c48a559..a8b914e862798 100644

[ ... ]

net/can/bcm.c static int bcm_proc_show(struct seq_file *m, void *v) · line 227

Quoted 6 lines

net/can/bcm.c +1 -1

@@ -224,10 +224,13 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 
 	list_for_each_entry_rcu(op, &bo->rx_ops, list) {
 
-		unsigned long reduction;
+		long reduction, frames_filtered, frames_abs;

[Severity: High]
This is a pre-existing issue, but does bcm_rx_setup() use list_add() instead of list_add_rcu() when inserting into bo->rx_ops?

If so, could lockless readers like bcm_proc_show() observe uninitialized memory due to missing store-release memory barriers?

Similarly, does bcm_proc_show() traverse bo->tx_ops locklessly while bcm_tx_setup() inserts using list_add()?

[ ... ]

net/can/bcm.c static int bcm_proc_show(struct seq_file *m, void *v) · line 300

Quoted 13 lines

net/can/bcm.c +9 -0

@@ -285,6 +289,15 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 }
 #endif /* CONFIG_PROC_FS */
 
+static void bcm_update_stats(struct bcm_op *op)
+{
+	/* prevent statistics overflow */
+	if (atomic_long_inc_return(&op->frames_abs) > LONG_MAX / 100) {
+		atomic_long_set(&op->frames_filtered, 0);
+		atomic_long_set(&op->frames_abs, 0);
+	}
+}
+

[Severity: Medium]
When bcm_rx_handler() calls bcm_update_stats(), frames_abs is incremented first. If it overflows, both frames_abs and frames_filtered are reset to 0.

If the packet passes the filter, bcm_rx_changed() is called next and increments frames_filtered:

net/can/bcm.c:bcm_rx_changed() {
    ...
    atomic_long_inc(&op->frames_filtered);
    ...
}

Could this order cause frames_filtered to become 1 while frames_abs is 0?

Since frames_abs increments by 1 per packet, would frames_filtered remain strictly greater than frames_abs, causing the reduction calculation in bcm_proc_show() to persistently yield a negative percentage?

[ ... ]

net/can/bcm.c static void bcm_rx_handler(struct sk_buff *skb, void *data) · line 733

Quoted 3 lines

net/can/bcm.c +0 -0

@@ -723,7 +732,7 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 	/* save originator for recvfrom() */
 	op->rx_ifindex = skb->dev->ifindex;

[Severity: High]
This is a pre-existing issue, but since the patch addresses data races in concurrent frame reception, does this concurrent execution also race on writing to op->rx_stamp and op->rx_ifindex here?

More significantly, could multiple CPUs concurrently execute memcpy() on the shared op->last_frames array in bcm_rx_update_and_send():

net/can/bcm.c:bcm_rx_update_and_send() {
    ...
    memcpy(lastdata, rxdata, op->cfsiz);
    ...
}

Could this non-atomic memory copy corrupt the CAN frame payload sent to userspace?

[Severity: High]
This is a pre-existing issue, but does unregistering the handler in bcm_delete_rx_op() properly synchronize with a concurrent bcm_rx_handler()?

If bcm_delete_rx_op() unregisters the handler and cancels timers:

net/can/bcm.c:bcm_delete_rx_op() {
    ...
    can_rx_unregister(sock_net(op->sk), NULL, op->can_id, ...);
    ...
    hrtimer_cancel(&op->thrtimer);
}

Could a concurrent bcm_rx_handler() still be executing and call hrtimer_start(&op->thrtimer) in bcm_rx_update_and_send() after the cancel?

Since op is freed after an RCU grace period, could this re-armed timer fire later and cause a use-after-free?

Quoted 6 lines

net/can/bcm.c +1 -1

 	/* update statistics */
-	op->frames_abs++;
+	bcm_update_stats(op);
 
 	if (op->flags & RX_RTR_FRAME) {
 		/* send reply for RTR-request (placed in op->frames[0]) */

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1