Re: [PATCH v15 10/11] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler()

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

Pre-existing issues:
- [High] Unmatched or multiplexed frames overwrite global metadata (rx_stamp/rx_ifindex), corrupting subsequent throttled frame notifications.
--

--- Patch [10]: [PATCH v15 10/11] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() ---
commit 192a4832f1f8f3051e71e66a1a0dc3b893e46cc8
Author: Oliver Hartkopp <[email protected]>
can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler()

This commit addresses a data race on op->rx_stamp and op->rx_ifindex in
bcm_rx_handler(). It moves the assignment of these fields into the same
spin_lock_bh() critical section as the content comparison, preventing torn
stores on 32-bit platforms and ensuring CAN frame metadata is correctly
bundled when processing concurrently on different CPUs.

Link: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a53dba6ab8b89..f213a0b377915 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -836,6 +831,14 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  			traffic_flags |= RX_OWN;
>  	}
>  
> +	/* save rx timestamp and originator for recvfrom() under lock.
> +	 * For an op subscribed on all interfaces (ifindex == 0)
> +	 * bcm_rx_handler() can run concurrently on different CPUs so
> +	 * the CAN content and the meta data must be bundled correctly.
> +	 */
> +	op->rx_stamp = skb->tstamp;
> +	op->rx_ifindex = skb->dev->ifindex;
> +

[Severity: High]
This isn't a bug introduced by this patch, but do unmatched or multiplexed
frames overwrite global metadata here in bcm_rx_handler(), corrupting subsequent
throttled frame notifications?

Since the assignments to op->rx_stamp and op->rx_ifindex happen unconditionally
before the multiplex filter checks, a frame that matches the CAN ID but fails
the multiplex mask check will still update the metadata.

If a previous frame matched a multiplex mask and was queued for throttle
delivery, could it be flushed to userspace with the timestamp and ifindex of the
dropped frame instead?

Additionally, if an operation tracks multiple indices (nframes > 1), would
concurrently throttled frames sharing the single global op->rx_stamp inevitably
overwrite each other's metadata?

>  	if (op->flags & RX_FILTER_ID) {
>  		/* the easiest case */
>  		bcm_rx_update_and_send(op, op->last_frames, rxframe,
>  				       traffic_flags);
>  		goto rx_starttimer;
>  	}
>  
>  	if (op->nframes == 1) {
>  		/* simple compare with index 0 */
>  		bcm_rx_cmp_to_index(op, 0, rxframe, traffic_flags);
>  		goto rx_starttimer;
>  	}
>  
>  	if (op->nframes > 1) {
>  		/*
>  		 * multiplex compare
>  		 *
>  		 * find the first multiplex mask that fits.
>  		 * Remark: The MUX-mask is stored in index 0 - but only the
>  		 * first 64 bits of the frame data[] are relevant (CAN FD)
>  		 */
>  
>  		for (i = 1; i < op->nframes; i++) {
>  			if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) ==
>  			    (get_u64(op->frames, 0) &
>  			     get_u64(op->frames + op->cfsiz * i, 0))) {
>  				bcm_rx_cmp_to_index(op, i, rxframe,
>  						    traffic_flags);
>  				break;
>  			}
>  		}
>  	}

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