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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] `frames_filtered` can exceed `frames_abs` when a throttle timer flushes multiple multiplex frames across a `frames_abs` reset, causing negative reduction percentages.
--

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

can: bcm: fix CAN frame rx/tx statistics

This commit fixes a KCSAN data race complaint in bcm_rx_handler() by
converting frames_abs and frames_filtered to atomic operations. It also
moves overflow updates into separate functions and aims to prevent
negative procfs reduction percentages.

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

[ ... ]

> @@ -290,6 +294,24 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  }
>  #endif /* CONFIG_PROC_FS */
>  
> +static void bcm_update_rx_stats(struct bcm_op *op)
> +{
> +	/* prevent overflow of the reduction% calculation in bcm_proc_show() */
> +	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);
> +	}
> +}
> +
> +static void bcm_update_tx_stats(struct bcm_op *op)
> +{

[ ... ]

> @@ -497,12 +519,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]
Could frames_filtered still exceed frames_abs here if a throttle timer flushes
multiple multiplex frames right after a counter reset?

If frames_abs reaches the LONG_MAX / 100 limit, bcm_update_rx_stats() resets
both counters to 0. If a subsequent frame is received, frames_abs is
incremented to 1.

If an active throttle timer then expires, bcm_rx_thr_flush() iterates and
calls bcm_rx_changed() for previously throttled frames:

net/can/bcm.c:bcm_rx_thr_flush() {
        ...
        /* for MUX filter we start at index 1 */
        for (i = 1; i < op->nframes; i++)
                updated += bcm_rx_do_flush(op, i);
        ...
}

Since frames_abs is non-zero (1), the check in bcm_rx_changed() evaluates to
true, and it will increment frames_filtered for each flushed frame.

Because bcm_rx_thr_flush() can process multiple frames at once,
frames_filtered could quickly become larger than frames_abs, which seems to
result in the negative procfs reduction percentages this patch intended to
prevent.

>  	/* this element is not throttled anymore */
>  	data->flags &= ~RX_THR;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4