Re: [PATCH net 15/19] 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] Unconditional update of `op->rx_stamp` and `op->rx_ifindex` corrupts metadata for throttled or multiplexed CAN frames.
--

--- Patch [15]: [PATCH net 15/19] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() ---
commit e05fd3b3c775fc8bd3bab9bce2bc9f1a23e5fc07
Author: Oliver Hartkopp <[email protected]>

can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler()

This patch fixes a data race on op->rx_stamp and op->rx_ifindex in
bcm_rx_handler() by moving their assignments into the bcm_rx_update_lock
critical section. This ensures the CAN content and metadata are bundled
correctly without torn stores on 32-bit platforms. As a side effect,
RTR-request frames no longer update these fields since only the notification
path needs them.

Link: https://patch.msgid.link/[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 is a pre-existing issue, but since the commit message states the goal
is to report the timestamp/ifindex of the very same frame whose content it
is delivering, does the unconditional assignment here still leave the
metadata vulnerable to being corrupted by subsequent frames?

If a CAN frame matches a BCM filter and is throttled, it is queued and the
throttle timer starts. Before the timer expires, if another CAN frame with
the same CAN ID arrives, it will unconditionally overwrite op->rx_stamp and
op->rx_ifindex here.

If this new frame is discarded by multiplex filters, won't
bcm_rx_thr_flush() end up sending the previously throttled frame to
userspace using the overwritten metadata of the discarded frame?

>  	if (op->flags & RX_FILTER_ID) {
>  		/* the easiest case */
>  		bcm_rx_update_and_send(op, op->last_frames, rxframe,

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