Re: [PATCH v6 4/9] 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 split of statistic updates across two separate spinlock critical sections in bcm_rx_handler allows frames_filtered to persistently exceed frames_abs after a counter reset, breaking their invariant and causing negative reduction percentages.

Pre-existing issues:
- [High] Concurrent, lockless updates to op->rx_stamp and op->rx_ifindex in bcm_rx_handler() cause data races and potential torn writes.
--

--- Patch [4]: [PATCH v6 4/9] can: bcm: fix CAN frame rx/tx statistics ---
commit fb8c8dc88e31fc268389c46d858c779a051a866c
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 to fix the KCSAN complaint.

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

[ ... ]

> @@ -497,12 +510,9 @@ static void bcm_rx_changed(struct bcm_op *op, struct canfd_frame *data)
>  {
>  	struct bcm_msg_head head;
>  
> -	/* update statistics */
> -	op->frames_filtered++;
> -
> -	/* prevent statistics overflow */
> -	if (op->frames_filtered > ULONG_MAX/100)
> -		op->frames_filtered = op->frames_abs = 0;
> +	/* update statistics (frames_filtered <= frames_abs) */
> +	if (atomic_long_read(&op->frames_abs))
> +		atomic_long_inc(&op->frames_filtered);

[Severity: Medium]
With the statistic updates now split across two separate lock regions in
bcm_rx_handler(), is it possible for frames_filtered to exceed frames_abs?

If Thread A calls bcm_update_stats() in the first lock region and triggers
the counter reset, then releases the lock, could Thread B interleave and
increment frames_abs to 1 before Thread A reaches the second lock region?

When Thread A subsequently calls bcm_rx_changed() in the second lock region,
it would observe frames_abs == 1 and increment frames_filtered (for the
frame that already caused the reset).

If Thread B then enters the second lock region and also increments
frames_filtered, wouldn't frames_filtered become larger than frames_abs?

When read by bcm_proc_show(), would this cause the reduction calculation
(100 - (frames_filtered * 100) / frames_abs) to yield a negative percentage?

[ ... ]

> @@ -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 since bcm_rx_handler() can be
executed concurrently on multiple CPUs for the same bcm_op, can these lockless
updates to op->rx_stamp (a 64-bit ktime_t) and op->rx_ifindex cause data races?

If so, could this lead to torn writes on 32-bit platforms, causing corrupted
timestamps and interface indices to be sent to userspace?

> +
> +	/* 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-v6-0-80710a2bc398@hartkopp.net?part=4