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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] `bcm_update_stats()` can be executed concurrently from different lock domains, contradicting the commit's claim of preventing races during counter reset.
- [Medium] TX operations incorrectly inherit the RX-specific statistics cap, causing premature zeroing of the TX frame counter.

Pre-existing issues:
- [High] Unprotected modifications of `op->rx_stamp` and `op->rx_ifindex` in `bcm_rx_handler()` lead to data races and torn writes of 64-bit metadata.
--

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

can: bcm: fix CAN frame rx/tx statistics

KCSAN detected a data race within the bcm_rx_handler() when two CAN frames
have been simultaneously received and processed in a single rx op by two
different CPUs. Use atomic operations with (signed) long data types to access
the statistics in the hot path. Additionally simplify the update and check of
statistics overflow by using the atomic operations in a separate
bcm_update_stats() function and put this function under bcm_rx_update_lock
to prevent races when resetting the counters.

> 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

[ ... ]

> @@ -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);

[Severity: Medium]
Does this inadvertently inherit the RX-specific limit for the TX path?

TX operations don't use frames_filtered or calculate a reduction percentage,
so the LONG_MAX / 100 limit enforced inside bcm_update_stats() seems logically
unnecessary here. On 32-bit systems, does this artificially cap the TX sent
counter to 21.4 million frames before abruptly resetting to zero, whereas
prior to this patch it safely counted up to ULONG_MAX?

[Severity: Medium]
The commit message mentions putting bcm_update_stats() under
bcm_rx_update_lock to prevent races when resetting the counters. However,
is it possible for bcm_update_stats() to be called concurrently from
different lock domains?

If an RX operation is configured with RX_RTR_FRAME, it can receive multiple
RTR frames concurrently. bcm_rx_handler() calls bcm_update_stats() while
holding bcm_rx_update_lock. It then invokes bcm_can_tx() to send the reply,
which calls bcm_update_stats() right here while holding a DIFFERENT lock,
bcm_tx_lock:

bcm_rx_handler()
    spin_lock_bh(&op->bcm_rx_update_lock);
    bcm_update_stats(op);
    spin_unlock_bh(&op->bcm_rx_update_lock);
    ...
    bcm_can_tx()
        spin_lock_bh(&op->bcm_tx_lock);
        bcm_update_stats(op);

Could this allow the reset logic inside bcm_update_stats() to run
concurrently across two CPUs, leading to an inconsistent counter reset?

[ ... ]

> @@ -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;
> -	/* 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);

[Severity: High]
This isn't a bug introduced by this patch, but while fixing the KCSAN data
race in bcm_rx_handler(), are op->rx_stamp and op->rx_ifindex still
vulnerable to concurrent torn writes?

These fields are written without any lock protection just before
bcm_rx_update_lock is acquired. If multiple CAN frames for the same RX
operation are received simultaneously on different CPUs, could the 64-bit
ktime_t timestamp tear on 32-bit systems? This might result in a wildly
incorrect timestamp value being passed to userspace via bcm_send_to_user()
along with misattributed network interfaces in the metadata.

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