Re: [PATCH v9 03/10] can: bcm: add locking when updating filter and timer values

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Lockless updates of RX metadata (`rx_stamp`, `rx_ifindex`, `frames_abs`) lead to torn reads, lost increments, and corrupted timestamps.
--

--- Patch [3]: [PATCH v9 03/10] can: bcm: add locking when updating filter and timer values ---
commit 10b4ca432b89b6bdfb7aab7948a13f81a6bc41ed
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add locking when updating filter and timer values

This commit introduces bcm_rx_update_lock to protect the filter and timer
configurations of a bcm_op structure from concurrent accesses. It ensures
that updates via bcm_rx_setup and usages in Rx and timeout handlers do not
race.

Link: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/

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

[ ... ]

> @@ -729,9 +751,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	/* update statistics */
>  	op->frames_abs++;
>  
> -	if (op->flags & RX_RTR_FRAME) {
> +	/* snapshot the flag under lock: op->flags/op->frames may be updated
> +	 * concurrently by bcm_rx_setup().
> +	 */
> +	spin_lock_bh(&op->bcm_rx_update_lock);

[Severity: Medium]
This is a pre-existing issue, but should the update to op->frames_abs be
protected by the new bcm_rx_update_lock?

If a bcm_op is configured to listen on all interfaces (ifindex == 0) and
receives frames concurrently on multiple CAN interfaces, bcm_rx_handler()
can execute simultaneously on different CPUs.

Because op->frames_abs is updated outside the lock, concurrent writes can
occur, potentially leading to lost statistical increments. Additionally, on
32-bit systems, concurrent writes to 64-bit metadata fields like
op->rx_stamp could lead to torn reads when userspace retrieves them.

Could these metadata fields be moved inside the lock to prevent data
races?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-bcm_fixes-v9-0-6eebb78ccfb2@hartkopp.net?part=3