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

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

Pre-existing issues:
- [High] Concurrent updates to `op->rx_stamp` and `op->rx_ifindex` outside of `bcm_rx_update_lock` lead to torn writes and mismatched metadata being delivered to user space.
--

--- Patch [4]: [PATCH v14 04/11] can: bcm: fix CAN frame rx/tx statistics ---
commit 1507f4edb289e7819b879095be4c4c84a3b1f750
Author: Oliver Hartkopp <[email protected]>

can: bcm: fix CAN frame rx/tx statistics

This commit addresses KCSAN-detected data races in bcm_rx_handler() by
using atomic operations for rx/tx statistics. It also restructures the
critical sections under bcm_rx_update_lock to prevent frames_filtered from
ever incorrectly exceeding frames_abs during concurrent updates.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index af4bb4182bc29..38c3f690c1cd0 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -748,24 +767,30 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	op->rx_stamp = skb->tstamp;
>  	/* save originator for recvfrom() */
>  	op->rx_ifindex = skb->dev->ifindex;
> -	/* update statistics */
> -	op->frames_abs++;
>  
> -	/* snapshot the flag under lock: op->flags/op->frames may be updated
> -	 * concurrently by bcm_rx_setup().
> -	 */
>  	spin_lock_bh(&op->bcm_rx_update_lock);

[Severity: High]
This is a pre-existing issue, but since this patch focuses on fixing
concurrent data races in bcm_rx_handler(), should these metadata fields
be moved inside the bcm_rx_update_lock as well?

If two CAN frames matching the same ANYDEV filter arrive concurrently on
different interfaces, could CPU2 overwrite op->rx_ifindex before CPU1
acquires the lock?

If CPU1 subsequently reads op->rx_ifindex inside bcm_send_to_user(), would
it attach CPU2's interface index to CPU1's CAN frame, delivering mismatched
metadata to user space?

Additionally, on 32-bit systems, could concurrent unprotected writes to the
64-bit ktime_t op->rx_stamp result in torn writes and corrupted timestamps?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4