[PATCH] can: bcm: fix CAN frame rx/tx statistics

[PATCH] can: bcm: fix CAN frame rx/tx statistics

[Oliver Hartkopp]

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 <socketcan@hartkopp.net>
---
 net/can/bcm.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 658c4a7a55df..d3d1ed7fd862 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -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;
-- 
2.53.0

Re: [PATCH] can: bcm: fix CAN frame rx/tx statistics

[sashiko-bot]

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 <socketcan@hartkopp.net>

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.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..a8b914e862798 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -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()?

[ ... ]
> @@ -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?

[ ... ]
> @@ -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?

>  	/* 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]) */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610093422.39847-1-socketcan@hartkopp.net?part=1