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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent execution of bcm_rx_handler() leads to unprotected data races and potential 64-bit torn writes on op->rx_stamp and op->rx_ifindex, causing metadata corruption.
--

--- Patch [4]: [PATCH v2 4/6] can: bcm: fix CAN frame rx/tx statistics ---
commit c40daefa12b560d84d208b978162b5b62f34aade
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
CAN frames are processed concurrently. It transitions the statistics counters
to atomic long operations and consolidates overflow checking into a new
bcm_update_stats() function.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -742,7 +752,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 can concurrent execution of bcm_rx_handler()
lead to unprotected data races on op->rx_stamp and op->rx_ifindex here?

If an application creates a BCM socket listening on all interfaces (ifindex=0),
and multiple CAN frames arrive concurrently from different interfaces matching
the same bcm_op, bcm_rx_handler() can be invoked concurrently on multiple CPUs.

CPU A and CPU B could both write op->rx_stamp and op->rx_ifindex outside of
any lock. If CPU A then acquires the lock and reads these values later in
bcm_send_to_user():

net/can/bcm.c:bcm_send_to_user() {
    ...
		/* restore rx timestamp */
		skb->tstamp = op->rx_stamp;
    ...
	addr->can_ifindex = op->rx_ifindex;
    ...
}

Could CPU A pick up CPU B's ifindex or a torn timestamp? This might lead to
CAN frames being delivered to userspace with mismatched interface indices or
corrupted 64-bit timestamps on 32-bit systems, potentially breaking
time-sensitive control loops.

>  	/* update statistics */
> -	op->frames_abs++;
> +	bcm_update_stats(op);
>  
>  	/* snapshot the flag under lock: op->flags/op->frames may be updated
>  	 * concurrently by bcm_rx_setup(). Do not call bcm_can_tx() while

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v2-0-96620c534b75@hartkopp.net?part=4