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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Lockless sequential resetting of related statistics creates a race condition leading to invalid procfs output.

Pre-existing issues:
- [High] Missing synchronize_rcu() in bcm_delete_rx_op() causes Use-After-Free of bcm_op due to concurrent timer re-arming.
- [High] Incomplete fix for concurrent rx data races leaves CAN frame payload and timestamps vulnerable to corruption.
--

commit da9dc73034abe67929230960f0d96f1b0cc6b4c0
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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. This patch uses atomic operations with (signed) long data
types to access the statistics in the hot path.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..e7b1eb61918f5 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -249,9 +252,9 @@ 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);
> +			   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);

[Severity: Medium]
Could this reduction calculation produce a wildly incorrect negative
percentage if the counters become desynchronized?

[ ... ]
> @@ -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]
Does sequentially resetting these atomic counters introduce a race window?

If a thread is preempted between resetting frames_filtered and frames_abs,
could another thread process numerous CAN frames and increment both counters,
leaving frames_filtered at a large value when the delayed thread resumes and
resets only frames_abs?

[ ... ]
> @@ -723,7 +733,7 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	/* save originator for recvfrom() */
>  	op->rx_ifindex = skb->dev->ifindex;
>  	/* update statistics */
> -	op->frames_abs++;
> +	bcm_update_stats(op);

[Severity: High]
This isn't a bug introduced by this patch, but since bcm_rx_handler() can
execute concurrently on multiple CPUs, does this leave the rest of the
shared bcm_op state vulnerable?

Concurrent lockless writes to op->rx_stamp and op->rx_ifindex could cause
torn reads.

Additionally, bcm_rx_update_and_send() performs a non-atomic memcpy()
to shared memory:

net/can/bcm.c:bcm_rx_update_and_send() {
	...
	memcpy(lastdata, rxdata, op->cfsiz);
	lastdata->flags |= (RX_RECV|RX_THR);
	lastdata->flags |= traffic_flags;
	...
}

Can this corrupt the CAN frame payload delivered to userspace?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
RX operation removal?

When bcm_delete_rx_op() removes an RX operation, it defers subscriber
removal to an RCU callback via can_rx_unregister(). It then calls
bcm_remove_op() which cancels timers and frees the bcm_op via call_rcu()
without waiting for existing readers:

net/can/bcm.c:bcm_delete_rx_op() {
	...
	can_rx_unregister(sock_net(op->sk), NULL, op->can_id,
			  REGMASK(op->can_id), bcm_rx_handler, op);
	list_del_rcu(&op->list);
	bcm_remove_op(op);
	...
}

Could a concurrently executing bcm_rx_handler() subsequently call
bcm_rx_update_and_send(), which unconditionally executes
hrtimer_start(&op->thrtimer, ...)?

Does this re-arm the timer after it was cancelled, causing a use-after-free
when the timer fires after the bcm_op memory is freed at the end of the RCU
grace period?

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