Re: [PATCH v13 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:

Pre-existing issues:
- [High] Missing RCU list primitives lead to potential NULL pointer dereference or memory corruption during lockless /proc/net/can-bcm/... traversal.
--

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

    can: bcm: fix CAN frame rx/tx statistics

This commit fixes a data race detected by KCSAN when two CAN frames are
simultaneously received and processed by different CPUs. It transitions
the statistics counters to use atomic operations to prevent races.
Additionally, it simplifies overflow checks and consolidates rx statistics
updates into a single critical section.

> 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
> @@ -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) {
>  
> -		unsigned long reduction;
> +		long reduction, frames_filtered, frames_abs;
> +

[ ... ]

> @@ -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 while reviewing this code we noticed a
potential missing RCU primitive issue.

In bcm_proc_show(), the code traverses the bo->tx_ops list (which contains
the loop printing the tx stats above) using list_for_each_entry() instead
of list_for_each_entry_rcu(). This omits the READ_ONCE() dependency ordering
required for safe lockless traversal under rcu_read_lock().

Additionally, when adding to these lists in bcm_tx_setup() and
bcm_rx_setup(), the code uses the non-RCU list_add() primitive instead of
list_add_rcu(), which omits the required smp_store_release() semantics.

Since a user can repeatedly read the procfs file for a CAN_BCM socket while
another thread concurrently adds filters on the same socket, could this
expose lockless readers to uninitialized memory and lead to memory corruption
or wild pointer dereferences?

>  	seq_putc(m, '\n');

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