Re: [PATCH v3 4/7] 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] A Time-Of-Check to Time-Of-Use (TOCTOU) race during statistics reset can permanently break the invariant `frames_filtered <= frames_abs`.

Pre-existing issues:
- [High] Missing RCU publish/subscribe barriers for lockless list operations, risking uninitialized memory reads in procfs.
--

--- Patch [4]: [PATCH v3 4/7] can: bcm: fix CAN frame rx/tx statistics ---
commit faeade145d2ba64627aca2f6af5f2b0b8ad0d933
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.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index fd548c6eaa913..0378e31ea10ed 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -280,7 +283,8 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  			seq_printf(m, "t2=%lld ",
>  				   (long long)ktime_to_us(op->kt_ival2));
>  
> -		seq_printf(m, "# sent %ld\n", op->frames_abs);
> +		seq_printf(m, "# sent %ld\n",
> +			   atomic_long_read(&op->frames_abs));
>  	}

[Severity: High]
This is a pre-existing issue, but does this function safely traverse
the lockless tx_ops list?

bcm_proc_show() traverses bo->tx_ops via list_for_each_entry() instead of
list_for_each_entry_rcu(), which lacks the necessary subscribe barriers.

Additionally, neither bcm_tx_setup() nor bcm_rx_setup() use list_add_rcu()
when inserting into these lists:

bcm_tx_setup()
    list_add(&op->list, &bo->tx_ops);

bcm_rx_setup()
    list_add(&op->list, &bo->rx_ops);

Could reading /proc/net/can/bcm/<inode> while another thread concurrently
sets up a BCM message on the same socket result in traversing uninitialized
memory due to missing RCU publish/subscribe barriers?

>  	seq_putc(m, '\n');
>  
> @@ -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]
Could this non-atomic reset allow other threads to interleave their
increments and permanently break the statistic invariant?

If Thread A hits the LONG_MAX / 100 threshold and begins resetting the stats:

Thread A
bcm_update_stats()
    atomic_long_set(&op->frames_filtered, 0);
    atomic_long_set(&op->frames_abs, 0);

And Thread B is concurrently processing bcm_rx_changed():

Thread B
bcm_rx_changed()
    if (atomic_long_read(&op->frames_abs))
        atomic_long_inc(&op->frames_filtered);

If Thread B reads a non-zero frames_abs, then Thread A resets both to 0,
and finally Thread B increments frames_filtered, wouldn't frames_filtered
become 1 while frames_abs is 0?

This would leave frames_filtered greater than frames_abs, causing the procfs
output to display negative reduction percentages.

> +
>  /*
>   * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
>   *              of the given bcm tx op
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v3-0-84600ca6d889@hartkopp.net?part=4