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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The statistics reset logic introduces a race condition for RX operations handling RTR frames, directly contradicting the commit's claim of preventing races.

Pre-existing issues:
- [Medium] Concurrent lockless updates to `op->rx_stamp` and `op->rx_ifindex` cause data races and metadata corruption.
--

--- Patch [4]: [PATCH v7 04/10] can: bcm: fix CAN frame rx/tx statistics ---
commit df517d421b8dc860bfb7f92f0c455c27cf5d743e
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: fix CAN frame rx/tx statistics

This patch attempts to fix data races in the CAN broadcast manager's receive
and transmit statistics by using atomic operations. It introduces a helper
to update and reset statistics to prevent overflow.

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

[ ... ]

> @@ -290,6 +294,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]
If an application sets up an RX op with RX_RTR_FRAME enabled, and multiple
RTR frames arrive concurrently, could there be a race condition here?

Because bcm_update_stats() is called under bcm_tx_lock in bcm_can_tx() and
under bcm_rx_update_lock in bcm_rx_handler(), both functions can execute
for the same RX op when processing an RTR frame.

If multiple CPUs handle RTR frames concurrently, they might execute
bcm_update_stats() using different locks, causing a race on this reset
logic where valid increments are overwritten or frames_filtered temporarily
exceeds frames_abs.

>  /*
>   * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
>   *              of the given bcm tx op
> @@ -344,7 +357,7 @@ static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
>  	spin_lock_bh(&op->bcm_tx_lock);
>  
>  	if (!err)
> -		op->frames_abs++;
> +		bcm_update_stats(op);
>  
>  	op->currframe++;
>  

[ ... ]

> @@ -748,16 +758,19 @@ 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: Medium]
This isn't a bug introduced by this patch, but does this lockless update of
rx_stamp and rx_ifindex cause data races?

When multiple CAN frames are received simultaneously on different CPUs (e.g.,
listening on all interfaces), these fields are updated outside the protection
of bcm_rx_update_lock.

This can lead to torn reads of the 64-bit timestamp on 32-bit systems, or
metadata mismatches where a CAN frame is sent to userspace with a timestamp
from a different frame.

> -	/* update statistics */
> -	op->frames_abs++;
>  
> -	/* snapshot the flag under lock: op->flags/op->frames may be updated
> -	 * concurrently by bcm_rx_setup().
> +	/* snapshot RTR content under lock: op->flags/op->frames may be
> +	 * updated concurrently by bcm_rx_setup().
>  	 */
>  	spin_lock_bh(&op->bcm_rx_update_lock);
> +
> +	/* update statistics with lock analogue to bcm_rx_changed() callers */
> +	bcm_update_stats(op);
> +
>  	rtr_frame = op->flags & RX_RTR_FRAME;
>  	if (rtr_frame)
>  		memcpy(&rtrframe, op->frames, op->cfsiz);
> +
>  	spin_unlock_bh(&op->bcm_rx_update_lock);
>  
>  	if (rtr_frame) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v7-0-c78896b342e4@hartkopp.net?part=4