Re: [PATCH net 09/19] can: bcm: fix CAN frame rx/tx statistics

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsynchronized concurrent updates to rx_stamp and rx_ifindex in bcm_rx_handler()
- [High] Missing RCU memory barriers for BCM operations lists (rx_ops and tx_ops)
--

--- Patch [9]: [PATCH net 09/19] can: bcm: fix CAN frame rx/tx statistics ---
commit d8d928a86f45beee426bc4e5b96d4a141f67ddbb
Author: Oliver Hartkopp <[email protected]>
Subject: 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. This patch uses atomic operations with (signed) long data types
to access the statistics in the hot path.

Link: https://patch.msgid.link/[email protected]

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 1e5f8d65d351b..03c98e4cc677d 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;
> +
> +		frames_filtered = atomic_long_read(&op->frames_filtered);
> +		frames_abs = atomic_long_read(&op->frames_abs);

[Severity: High]
This isn't a bug introduced by this patch, but while we're in bcm_proc_show(),
does this function safely traverse both operation lists?

Looking further down in bcm_proc_show(), tx_ops is traversed holding only
rcu_read_lock():

	list_for_each_entry(op, &bo->tx_ops, list) {

However, this uses list_for_each_entry() instead of list_for_each_entry_rcu(),
which misses the necessary load-acquire operations.

Additionally, in bcm_tx_setup() and bcm_rx_setup(), new BCM operations are
added to these lists without RCU barriers:

net/can/bcm.c:bcm_tx_setup() {
    ...
	/* add this bcm_op to the list of the tx_ops */
	list_add(&op->list, &bo->tx_ops);
    ...
}

and:

net/can/bcm.c:bcm_rx_setup() {
    ...
	/* add this bcm_op to the list of the rx_ops */
	list_add(&op->list, &bo->rx_ops);
    ...
}

Could using list_add() instead of list_add_rcu() miss the store-release memory
barriers required to safely publish the newly initialized node to concurrent
RCU readers? This could allow the procfs reader to observe uninitialized
or partially initialized list nodes.

[ ... ]
> @@ -756,24 +775,30 @@ 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().
> -	 */
> +	/* op->flags/op->frames may be updated concurrently by bcm_rx_setup() */
>  	spin_lock_bh(&op->bcm_rx_update_lock);

[Severity: High]
This is a pre-existing issue, but while the statistics updates are moved into
the locked section, rx_stamp and rx_ifindex appear to remain unprotected.

For an RX operation subscribed to all interfaces (ifindex == 0),
bcm_rx_handler() can execute concurrently on different CPUs for the same
op.

Because op->rx_stamp and op->rx_ifindex are updated before acquiring
bcm_rx_update_lock, if two frames arrive concurrently on different CPUs,
could the first CPU waiting for the lock have its metadata overwritten by
the second CPU?

The first CPU would then process its payload but bundle it with the timestamp
and interface index of the second CPU's frame, returning corrupted metadata
to userspace. On 32-bit systems, could this concurrent unsynchronized write to
the 64-bit ktime_t also cause a torn read?

Should these metadata assignments be moved inside the spin_lock_bh() section?

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