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

[email protected]
Newsgroups org.kernel.vger.linux-can
Message-ID <[email protected]>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsynchronized modification of rx_stamp and rx_ifindex allows concurrent frames to corrupt each other's metadata.
--

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

can: bcm: fix CAN frame rx/tx statistics

This commit addresses a KCSAN-detected data race in bcm_rx_handler() by
converting rx/tx statistics to atomic operations. It also simplifies
overflow handling and resolves a race condition where frames_filtered
could become larger than frames_abs.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> --- 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;

[Severity: High]
This is a pre-existing issue, but does this unsynchronized modification
allow concurrent frames to corrupt each other's metadata?

When a wildcard (ANYDEV) CAN BCM receive operation is configured,
multiple interfaces might receive matching packets simultaneously. Because
op->rx_stamp and op->rx_ifindex are updated outside of
bcm_rx_update_lock, could different CPUs concurrently overwrite these
fields?

If so, this might result in userspace receiving packets with mismatched
timestamps and source interface indices, potentially causing logic errors
in routing or packet processing.

> -	/* update statistics */
> -	op->frames_abs++;
>  
> -	/* snapshot the flag under lock: op->flags/op->frames may be updated
> -	 * concurrently by bcm_rx_setup().
> -	 */
> +	/* op->flags/op->frames may be updated concurrently by bcm_rx_setup() */
>  	spin_lock_bh(&op->bcm_rx_update_lock);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.