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

On 14.07.26 12:33, [email protected] wrote:
> 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.
> --

Fixed in patch 5

> 
> --- 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');
>