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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Lock dropping in bcm_rx_handler() allows frames_filtered to temporarily exceed frames_abs, breaking the invariant and causing negative reduction percentage calculations.

Pre-existing issues:
- [High] bcm_tx_setup modifies ktime_t timers, flags, and frames without holding bcm_tx_lock, allowing concurrent timers to read torn or corrupted data.
- [High] bcm_rx_handler races on op->rx_stamp and op->rx_ifindex updates outside bcm_rx_update_lock.
- [High] list_add() is used instead of list_add_rcu() when adding newly initialized bcm_op elements to the RCU-protected lists bo->tx_ops and bo->rx_ops.
--

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

can: bcm: fix CAN frame rx/tx statistics

This commit fixes a KCSAN data race complaint in the broadcast manager
receive handler when two CAN frames are processed simultaneously. It
switches to atomic types for statistics and refactors overflow handling.

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

[ ... ]

> @@ -229,10 +229,13 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  
>  	list_for_each_entry_rcu(op, &bo->rx_ops, list) {

[Severity: High]
This isn't a bug introduced by this patch, but is it safe for bcm_rx_setup()
and bcm_tx_setup() to use list_add() when inserting new elements into these
lists?

Since bcm_proc_show() iterates over bo->rx_ops and bo->tx_ops using
list_for_each_entry_rcu(), using list_add() instead of list_add_rcu() during
insertion lacks the necessary memory barriers. Could this allow concurrent
RCU readers on weakly ordered architectures to observe uninitialized fields?

[ ... ]

> @@ -344,7 +366,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_tx_stats(op);
>  
>  	op->currframe++;

[Severity: High]
This is a pre-existing issue, but does bcm_tx_setup() need to hold this same
bcm_tx_lock when modifying op->frames, op->flags, and the ktime_t timers?

When a user calls bcm_sendmsg() to update an existing TX op, these values are
modified without the lock. If a concurrent timer fires bcm_tx_timeout_handler()
or transmission occurs here via bcm_can_tx(), could it result in torn reads of
64-bit ktime_t timers on 32-bit systems, or the transmission of partially
updated CAN frames?

[ ... ]

> @@ -748,16 +767,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: High]
This is a pre-existing issue, but can these unprotected assignments cause a
metadata race?

If a socket is subscribed to all interfaces (ifindex == 0) and two frames are
received simultaneously on different interfaces, couldn't different CPUs
concurrently overwrite op->rx_stamp and op->rx_ifindex outside of
bcm_rx_update_lock? This might result in corrupted stream metadata when the
frames are eventually sent to userspace.

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

[Severity: Medium]
Can dropping the lock here allow frames_filtered to exceed frames_abs?

CPU 1 could increment frames_abs past the limit (LONG_MAX / 100) and reset
both counters to 0 inside bcm_update_rx_stats(), then drop the lock.

CPU 2 could then process a packet, acquire the lock, increment frames_abs to
1, and drop the lock.

Both CPUs would then proceed to their second lock section where
bcm_rx_changed() is called. Both would increment frames_filtered because
frames_abs is non-zero, leaving frames_filtered at 2 while frames_abs is 1.
Wouldn't this break the invariant and cause a negative reduction percentage to
be displayed in procfs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-bcm_fixes-v9-0-6eebb78ccfb2@hartkopp.net?part=4