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

[sashiko-bot@kernel.org]

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