Skip to contentSkip to conversation

Fix two KCSAN issues

PATCH In review Oliver Hartkopp 2 participants 7 messages May 22 – Jun 10

Subsystems (2)
CAN network drivers CAN network layer
lore
Share & tools
lore

Oliver Hartkopp

author maintainer
#0 lore raw

v2 cover Fix two KCSAN issues

Patch 1 supersedes the "add locking when updating filter and timer values"
https://patchwork.kernel.org/project/linux-can/patch/[email protected]/
where the readers in softirq/timer context have not been addressed correctly and some comment was insufficient.

Patch 2 fixed an issue that has been detected by KCSAN when I was testing patch 1 ;-)

V2: Changed the commit message of patch 1 as suggested by the AI review. But the review result that "The syzbot report link could not be verified." is obviously wrong. The URL works flawlessly:
https://lore.kernel.org/linux-can/[email protected]/

Oliver Hartkopp (2):
can: bcm: add locking when updating filter and timer values
can: bcm: use atomic access in receive statistics

 net/can/bcm.c | 88 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 19 deletions(-)
Signature · 1 line
2.53.0
author maintainer
#1 lore raw

PATCH v2 1/2 can: bcm: add locking when updating filter and timer values

KCSAN detected a simultaneous access to timer values that can be overwritten in bcm_rx_setup when updating timer and filter content.

Fix the RX_SETUP update case by protecting the timer and filter updates with a spin_lock_bh().

Fixes ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Reported-by [email protected]
Closes https://lore.kernel.org/linux-can/[email protected]/ open here
Signed-off-by Oliver Hartkopp <[email protected]>
---
 net/can/bcm.c | 61 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 9 deletions(-)
net/can/bcm.c +52 -9
index a4bef2c48a55..dbdf5d4f4ed7 100644
@@ -123,10 +123,11 @@ struct bcm_op {
 	struct canfd_frame sframe;
 	struct canfd_frame last_sframe;
 	struct sock *sk;
 	struct net_device *rx_reg_dev;
 	spinlock_t bcm_tx_lock; /* protect currframe/count in runtime updates */
+	spinlock_t bcm_rx_update_lock; /* protect filter/timer data updates */
 };
 
 struct bcm_sock {
 	struct sock sk;
 	int bound;

@@ -616,10 +617,12 @@ static void bcm_rx_starttimer(struct bcm_op *op)
 static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer)
 {
 	struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
 	struct bcm_msg_head msg_head;
 
+	spin_lock_bh(&op->bcm_rx_update_lock);
+
 	/* if user wants to be informed, when cyclic CAN-Messages come back */
 	if ((op->flags & RX_ANNOUNCE_RESUME) && op->last_frames) {
 		/* clear received CAN frames to indicate 'nothing received' */
 		memset(op->last_frames, 0, op->nframes * op->cfsiz);
 	}

@@ -632,10 +635,12 @@ static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer)
 	msg_head.ival1   = op->ival1;
 	msg_head.ival2   = op->ival2;
 	msg_head.can_id  = op->can_id;
 	msg_head.nframes = 0;
 
+	spin_unlock_bh(&op->bcm_rx_update_lock);
+
 	bcm_send_to_user(op, &msg_head, NULL, 0);
 
 	return HRTIMER_NORESTART;
 }
 

@@ -680,19 +685,26 @@ static int bcm_rx_thr_flush(struct bcm_op *op)
  *                      Check for throttled data and send it to the userspace
  */
 static enum hrtimer_restart bcm_rx_thr_handler(struct hrtimer *hrtimer)
 {
 	struct bcm_op *op = container_of(hrtimer, struct bcm_op, thrtimer);
+	enum hrtimer_restart ret;
+
+	spin_lock_bh(&op->bcm_rx_update_lock);
 
 	if (bcm_rx_thr_flush(op)) {
 		hrtimer_forward_now(hrtimer, op->kt_ival2);
-		return HRTIMER_RESTART;
+		ret = HRTIMER_RESTART;
 	} else {
 		/* rearm throttle handling */
 		op->kt_lastmsg = 0;
-		return HRTIMER_NORESTART;
+		ret = HRTIMER_NORESTART;
 	}
+
+	spin_unlock_bh(&op->bcm_rx_update_lock);
+
+	return ret;
 }
 
 /*
  * bcm_rx_handler - handle a CAN frame reception
  */

@@ -737,10 +749,12 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 		traffic_flags |= RX_LOCAL;
 		if (skb->sk == op->sk)
 			traffic_flags |= RX_OWN;
 	}
 
+	spin_lock_bh(&op->bcm_rx_update_lock);
+
 	if (op->flags & RX_FILTER_ID) {
 		/* the easiest case */
 		bcm_rx_update_and_send(op, op->last_frames, rxframe,
 				       traffic_flags);
 		goto rx_starttimer;

@@ -772,10 +786,12 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 		}
 	}
 
 rx_starttimer:
 	bcm_rx_starttimer(op);
+
+	spin_unlock_bh(&op->bcm_rx_update_lock);
 }
 
 /*
  * helpers for bcm_op handling: find & delete bcm [rx|tx] op elements
  */

@@ -1140,10 +1156,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		return -EINVAL;
 
 	/* check the given can_id */
 	op = bcm_find_op(&bo->rx_ops, msg_head, ifindex);
 	if (op) {
+		void *new_frames = NULL;
+
 		/* update existing BCM operation */
 
 		/*
 		 * Do we need more space for the CAN frames than currently
 		 * allocated? -> This is a _really_ unusual use-case and

@@ -1151,33 +1169,53 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		 */
 		if (msg_head->nframes > op->nframes)
 			return -E2BIG;
 
 		if (msg_head->nframes) {
-			/* update CAN frames content */
-			err = memcpy_from_msg(op->frames, msg,
+			/* get new CAN frames content before locking */
+			new_frames = kmalloc(msg_head->nframes * op->cfsiz,
+					     GFP_KERNEL);
+			if (!new_frames)
+				return -ENOMEM;
+
+			err = memcpy_from_msg(new_frames, msg,
 					      msg_head->nframes * op->cfsiz);
-			if (err < 0)
+			if (err < 0) {
+				kfree(new_frames);
 				return err;
-
-			/* clear last_frames to indicate 'nothing received' */
-			memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
+			}
 		}
 
+		spin_lock_bh(&op->bcm_rx_update_lock);
 		op->nframes = msg_head->nframes;
 		op->flags = msg_head->flags;
 
+		if (msg_head->nframes) {
+			/* update CAN frames content */
+			memcpy(op->frames, new_frames,
+			       msg_head->nframes * op->cfsiz);
+
+			/* clear last_frames to indicate 'nothing received' */
+			memset(op->last_frames, 0,
+			       msg_head->nframes * op->cfsiz);
+		}
+		spin_unlock_bh(&op->bcm_rx_update_lock);
+
+		/* free temporary frames / kfree(NULL) is safe */
+		kfree(new_frames);
+
 		/* Only an update -> do not call can_rx_register() */
 		do_rx_register = 0;
 
 	} else {
 		/* insert new BCM operation for the given can_id */
 		op = kzalloc(OPSIZ, GFP_KERNEL);
 		if (!op)
 			return -ENOMEM;
 
 		spin_lock_init(&op->bcm_tx_lock);
+		spin_lock_init(&op->bcm_rx_update_lock);
 		op->can_id = msg_head->can_id;
 		op->nframes = msg_head->nframes;
 		op->cfsiz = CFSIZ(msg_head->flags);
 		op->flags = msg_head->flags;
 

@@ -1260,26 +1298,31 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
 	} else {
 		if (op->flags & SETTIMER) {
 
 			/* set timer value */
+			spin_lock_bh(&op->bcm_rx_update_lock);
 			op->ival1 = msg_head->ival1;
 			op->ival2 = msg_head->ival2;
 			op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
 			op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
+			op->kt_lastmsg = 0;
+			spin_unlock_bh(&op->bcm_rx_update_lock);
 
 			/* disable an active timer due to zero value? */
 			if (!op->kt_ival1)
 				hrtimer_cancel(&op->timer);
 
 			/*
 			 * In any case cancel the throttle timer, flush
 			 * potentially blocked msgs and reset throttle handling
 			 */
-			op->kt_lastmsg = 0;
 			hrtimer_cancel(&op->thrtimer);
+
+			spin_lock_bh(&op->bcm_rx_update_lock);
 			bcm_rx_thr_flush(op);
+			spin_unlock_bh(&op->bcm_rx_update_lock);
 		}
 
 		if ((op->flags & STARTTIMER) && op->kt_ival1)
 			hrtimer_start(&op->timer, op->kt_ival1,
 				      HRTIMER_MODE_REL_SOFT);
Signature · 1 line
2.53.0
AI review
#2 lore raw

Re: [PATCH v2 1/2] can: bcm: add locking when updating filter and timer values

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

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

Pre-existing issues:
- [High] Concurrent RX_DELETE and CAN frame reception can cause a timer to be armed after it was canceled, leading to a Use-After-Free. --

commit a1560c18e32869b3dc94d859252cf67b537b151d
Author: Oliver Hartkopp <[email protected]>

can: bcm: add locking when updating filter and timer values

KCSAN detected a simultaneous access to timer values that can be overwritten in bcm_rx_setup when updating timer and filter content.

Fix the RX_SETUP update case by protecting the timer and filter updates with a spin_lock_bh().

Link https://lore.kernel.org/linux-can/[email protected]/ open here
Quoted 4 lines
net/can/bcm.c +0 -0
index a4bef2c48a559..dbdf5d4f4ed7b 100644

[ ... ]

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

Quoted 7 lines
net/can/bcm.c +2 -0
@@ -774,6 +788,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 
 rx_starttimer:
 	bcm_rx_starttimer(op);
+
+	spin_unlock_bh(&op->bcm_rx_update_lock);
 }

[Severity: High]
This is a pre-existing issue, but does this still leave a race condition with bcm_delete_rx_op()?

Even with bcm_rx_update_lock held here, bcm_delete_rx_op() modifies op->flags without taking bcm_rx_update_lock:

net/can/bcm.c:bcm_delete_rx_op() {
	...
	/* disable automatic timer on frame reception */
	op->flags |= RX_NO_AUTOTIMER;
	...
	bcm_remove_op(op);
}

And bcm_remove_op() cancels the timers and queues the object for RCU freeing:

net/can/bcm.c:bcm_remove_op() {
	hrtimer_cancel(&op->timer);
	hrtimer_cancel(&op->thrtimer);
	call_rcu(&op->rcu, bcm_free_op_rcu);
}

If bcm_rx_starttimer() reads the old op->flags value, can it be executed after bcm_remove_op() has already canceled the timers? This would result in the timer being re-armed on a soon-to-be-freed object, potentially leading to a use-after-free when the timer fires.

Should bcm_delete_rx_op() also take bcm_rx_update_lock when updating op->flags?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer
#3 lore raw

Re: [PATCH v2 1/2] can: bcm: add locking when updating filter and timer values

The op->flags |= RX_NO_AUTOTIMER issue has been addressed by this patch which needs to be applied before evaluation:

[PATCH v3 1/1] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

https://lore.kernel.org/linux-can/[email protected]/T/#t

Quoted 65 lines · On 09.06.26 20:55, [email protected] wrote:

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

Pre-existing issues:
- [High] Concurrent RX_DELETE and CAN frame reception can cause a timer to be armed after it was canceled, leading to a Use-After-Free. --

commit a1560c18e32869b3dc94d859252cf67b537b151d
Author: Oliver Hartkopp <[email protected]>

can: bcm: add locking when updating filter and timer values

KCSAN detected a simultaneous access to timer values that can be overwritten in bcm_rx_setup when updating timer and filter content.

Fix the RX_SETUP update case by protecting the timer and filter updates with a spin_lock_bh().

Link https://lore.kernel.org/linux-can/[email protected]/ open here
Quoted 4 lines
net/can/bcm.c +0 -0
index a4bef2c48a559..dbdf5d4f4ed7b 100644

[ ... ]

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

Quoted 7 lines
net/can/bcm.c +2 -0
@@ -774,6 +788,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 
  rx_starttimer:
  	bcm_rx_starttimer(op);
+
+	spin_unlock_bh(&op->bcm_rx_update_lock);
  }

[Severity: High]
This is a pre-existing issue, but does this still leave a race condition with bcm_delete_rx_op()?

Even with bcm_rx_update_lock held here, bcm_delete_rx_op() modifies op->flags without taking bcm_rx_update_lock:

net/can/bcm.c:bcm_delete_rx_op() {
	...
	/* disable automatic timer on frame reception */
	op->flags |= RX_NO_AUTOTIMER;
	...
	bcm_remove_op(op);
}

And bcm_remove_op() cancels the timers and queues the object for RCU freeing:

net/can/bcm.c:bcm_remove_op() {
	hrtimer_cancel(&op->timer);
	hrtimer_cancel(&op->thrtimer);
	call_rcu(&op->rcu, bcm_free_op_rcu);
}

If bcm_rx_starttimer() reads the old op->flags value, can it be executed after bcm_remove_op() has already canceled the timers? This would result in the timer being re-armed on a soon-to-be-freed object, potentially leading to a use-after-free when the timer fires.

Should bcm_delete_rx_op() also take bcm_rx_update_lock when updating op->flags?

author maintainer
#4 lore raw

PATCH v2 2/2 can: bcm: use atomic access in receive statistics

In reply to 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 to access the statistics in the hot path to fix the KCSAN complaint.

Fixes ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by Oliver Hartkopp <[email protected]>
---
 net/can/bcm.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)
net/can/bcm.c +17 -10
index dbdf5d4f4ed7..8a2cb766603d 100644
@@ -106,11 +106,12 @@ 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;
+	atomic_long_t 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;

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

@@ -248,13 +249,16 @@ 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);
+			   atomic_long_read(&op->frames_filtered),
+			   atomic_long_read(&op->frames_abs));
 
-		reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
+		reduction = 100 -
+			(atomic_long_read(&op->frames_filtered) * 100) /
+			atomic_long_read(&op->frames_abs);
 
 		seq_printf(m, "%s%ld%%\n",
 			   (reduction == 100) ? "near " : "", reduction);
 	}
 

@@ -274,11 +278,12 @@ 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();
 

@@ -334,11 +339,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++;
+		atomic_long_inc(&op->frames_abs);
 
 	op->currframe++;
 
 	/* reached last frame? */
 	if (op->currframe >= op->nframes)

@@ -488,15 +493,17 @@ 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++;
+	atomic_long_inc(&op->frames_filtered);
 
 	/* prevent statistics overflow */
-	if (op->frames_filtered > ULONG_MAX/100)
-		op->frames_filtered = op->frames_abs = 0;
+	if (atomic_long_read(&op->frames_filtered) > ULONG_MAX / 100) {
+		atomic_long_set(&op->frames_filtered, 0);
+		atomic_long_set(&op->frames_abs, 0);
+	}
 
 	/* this element is not throttled anymore */
 	data->flags &= ~RX_THR;
 
 	memset(&head, 0, sizeof(head));

@@ -733,11 +740,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++;
+	atomic_long_inc(&op->frames_abs);
 
 	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
AI review
#5 lore raw

Re: [PATCH v2 2/2] can: bcm: use atomic access in receive statistics

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

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

New issues:
- [Medium] Converting frames_filtered to atomic_long_t introduces a signed integer overflow on 32-bit systems during procfs statistics calculation, producing bogus corrupted percentages.

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in bcm_proc_show() can cause a division by zero and kernel panic due to multiple lockless reads of op->frames_abs. --

commit 6749d4fcb2595ae8bc232f3dd041a194ebd9a9f4
Author: Oliver Hartkopp <[email protected]>

can: bcm: use atomic access in receive statistics

This commit modifies the CAN broadcast manager (bcm) protocol to use atomic operations for rx statistics. This prevents a data race detected by KCSAN when multiple CAN frames are processed simultaneously by different CPUs.

Quoted 13 lines
net/can/bcm.c +1 -1
index dbdf5d4f4ed7b..8a2cb766603dc 100644
@@ -228,7 +229,7 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 		unsigned long reduction;
 
 		/* print only active entries & prevent division by zero */
-		if (!op->frames_abs)
+		if (!atomic_long_read(&op->frames_abs))
 			continue;
 
 		seq_printf(m, "rx_op: %03X %-5s ", op->can_id,

[ ... ]

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

Quoted 12 lines
net/can/bcm.c +5 -2
@@ -250,9 +251,12 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 				   (long long)ktime_to_us(op->kt_ival2));
 
 		seq_printf(m, "# recv %ld (%ld) => reduction: ",
-			   op->frames_filtered, op->frames_abs);
+			   atomic_long_read(&op->frames_filtered),
+			   atomic_long_read(&op->frames_abs));
 
-		reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
+		reduction = 100 -
+			(atomic_long_read(&op->frames_filtered) * 100) /
+			atomic_long_read(&op->frames_abs);

[Severity: High]
This isn't a bug introduced by this patch, but does reading op->frames_abs multiple times introduce a Time-of-Check to Time-of-Use race here?

bcm_proc_show() in net/can/bcm.c first ensures op->frames_abs is not zero. However, concurrently, bcm_rx_changed() can hit the overflow threshold and reset it:

net/can/bcm.c:bcm_rx_changed() {
    ...
	if (atomic_long_read(&op->frames_filtered) > ULONG_MAX / 100) {
		atomic_long_set(&op->frames_filtered, 0);
		atomic_long_set(&op->frames_abs, 0);
	}
    ...
}

If this reset occurs precisely between the initial non-zero check and the division above, could the denominator evaluate to 0 and cause a divide-by-zero kernel panic?

Would it be safer to read the atomic values exactly once into local variables at the beginning of the iteration?

[Severity: Medium]
Does changing the statistics variables to atomic_long_t introduce a signed integer overflow on 32-bit systems?

atomic_long_read() returns a signed long. The expression atomic_long_read(&op->frames_filtered) * 100 in bcm_proc_show() now evaluates using signed 32-bit integer arithmetic on 32-bit systems.

While the overflow prevention in bcm_rx_changed() caps frames_filtered at ULONG_MAX / 100, multiplying by 100 when frames_filtered exceeds LONG_MAX / 100 will overflow the maximum positive value for a signed 32-bit integer.

Dividing this negative intermediate result by frames_abs yields a negative number, which corrupts the final reduction percentage.

Should the reads be cast to unsigned long before the calculation?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
author maintainer
#6 lore raw

Re: [PATCH v2 2/2] can: bcm: use atomic access in receive statistics

Quoted 9 lines · On 09.06.26 20:45, [email protected] wrote:

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

New issues:
- [Medium] Converting frames_filtered to atomic_long_t introduces a signed integer overflow on 32-bit systems during procfs statistics calculation, producing bogus corrupted percentages.

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in bcm_proc_show() can cause a division by zero and kernel panic due to multiple lockless reads of op->frames_abs. --

Correct!

This patch is now superseded by:

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

https://lore.kernel.org/linux-can/[email protected]/

which addresses the found issues.

Many thanks,
Oliver

Quoted 79 lines

commit 6749d4fcb2595ae8bc232f3dd041a194ebd9a9f4
Author: Oliver Hartkopp <[email protected]>

can: bcm: use atomic access in receive statistics

This commit modifies the CAN broadcast manager (bcm) protocol to use atomic operations for rx statistics. This prevents a data race detected by KCSAN when multiple CAN frames are processed simultaneously by different CPUs.

Quoted 13 lines
net/can/bcm.c +1 -1
index dbdf5d4f4ed7b..8a2cb766603dc 100644
@@ -228,7 +229,7 @@ static int bcm_proc_show(struct seq_file *m, void *v)
  		unsigned long reduction;
 
  		/* print only active entries & prevent division by zero */
-		if (!op->frames_abs)
+		if (!atomic_long_read(&op->frames_abs))
  			continue;
 
  		seq_printf(m, "rx_op: %03X %-5s ", op->can_id,

[ ... ]

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

Quoted 12 lines
net/can/bcm.c +5 -2
@@ -250,9 +251,12 @@ static int bcm_proc_show(struct seq_file *m, void *v)
  				   (long long)ktime_to_us(op->kt_ival2));
 
  		seq_printf(m, "# recv %ld (%ld) => reduction: ",
-			   op->frames_filtered, op->frames_abs);
+			   atomic_long_read(&op->frames_filtered),
+			   atomic_long_read(&op->frames_abs));
 
-		reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
+		reduction = 100 -
+			(atomic_long_read(&op->frames_filtered) * 100) /
+			atomic_long_read(&op->frames_abs);

[Severity: High]
This isn't a bug introduced by this patch, but does reading op->frames_abs multiple times introduce a Time-of-Check to Time-of-Use race here?

bcm_proc_show() in net/can/bcm.c first ensures op->frames_abs is not zero. However, concurrently, bcm_rx_changed() can hit the overflow threshold and reset it:

net/can/bcm.c:bcm_rx_changed() {
     ...
	if (atomic_long_read(&op->frames_filtered) > ULONG_MAX / 100) {
		atomic_long_set(&op->frames_filtered, 0);
		atomic_long_set(&op->frames_abs, 0);
	}
     ...
}

If this reset occurs precisely between the initial non-zero check and the division above, could the denominator evaluate to 0 and cause a divide-by-zero kernel panic?

Would it be safer to read the atomic values exactly once into local variables at the beginning of the iteration?

[Severity: Medium]
Does changing the statistics variables to atomic_long_t introduce a signed integer overflow on 32-bit systems?

atomic_long_read() returns a signed long. The expression atomic_long_read(&op->frames_filtered) * 100 in bcm_proc_show() now evaluates using signed 32-bit integer arithmetic on 32-bit systems.

While the overflow prevention in bcm_rx_changed() caps frames_filtered at ULONG_MAX / 100, multiplying by 100 when frames_filtered exceeds LONG_MAX / 100 will overflow the maximum positive value for a signed 32-bit integer.

Dividing this negative intermediate result by frames_abs yields a negative number, which corrupts the final reduction percentage.

Should the reads be cast to unsigned long before the calculation?