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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Spurious RX_TIMEOUT notification and state corruption due to a race between concurrent bcm_rx_handler execution and op->timer expiration.
- [High] Throttled multiplex frames share a single metadata store, leading to incorrect interface index (rx_ifindex) and timestamps being sent to userspace.
--

--- Patch [10]: [PATCH v10 10/10] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() ---
commit d4e0952720604b6c20cb44a753876e69fcdf61ec
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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

For an rx op subscribed on all interfaces (ifindex == 0), the same op
is registered once in the shared per-netns wildcard filter list, so
bcm_rx_handler() can run concurrently on different CPUs for frames
arriving on different net devices.

Link: https://lore.kernel.org/linux-can/20260707145135.5BC831F00A3A@smtp.kernel.org/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index fbf1cbb7c99fe..10a883b50fc63 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -779,11 +779,6 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	/* disable timeout */
>  	hrtimer_cancel(&op->timer);

[Severity: High]
This is a pre-existing issue, but could this lockless cancellation cause
spurious RX_TIMEOUT notifications and state corruption?

If a wildcard receive operation receives two frames concurrently on different
CPUs:

CPU1 and CPU2 execute bcm_rx_handler() and both call hrtimer_cancel() outside
the lock. The timer is not yet running, so both return immediately.

CPU1 acquires op->bcm_rx_update_lock, handles its frame, calls
bcm_rx_starttimer() to arm the timer, and releases the lock.

CPU2 acquires op->bcm_rx_update_lock.

The timer expires and bcm_rx_timeout_handler() starts on CPU3, blocking on
the lock.

CPU2 finishes handling its frame, calls bcm_rx_starttimer() to re-arm, and
releases the lock.

CPU3 acquires the lock, clears op->last_frames via memset(), and
unconditionally sends an RX_TIMEOUT, despite CPU2 having just successfully
received a frame?

> -	/* save rx timestamp */
> -	op->rx_stamp = skb->tstamp;
> -	/* save originator for recvfrom() */
> -	op->rx_ifindex = skb->dev->ifindex;
> -
>  	/* op->flags/op->frames may be updated concurrently by bcm_rx_setup() */
>  	spin_lock_bh(&op->bcm_rx_update_lock);

[ ... ]

> @@ -815,6 +810,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 does sharing a single metadata
store for throttled multiplex frames lead to incorrect interface indexes and
timestamps being sent to userspace?

If an RX_SETUP operation is created on all interfaces with multiplex filtering
and throttling:

A frame matching multiplex index 1 arrives on can0. op->rx_ifindex becomes
can0 and the throttle timer starts.

A frame matching multiplex index 2 arrives on can1. op->rx_ifindex is
unconditionally overwritten with can1 here.

When the throttle timer expires, bcm_rx_thr_flush() sends both buffered
frames to userspace:

    updated += bcm_rx_do_flush(op, i);

Are both frames then delivered with can_ifindex set to can1 by
bcm_send_to_user(), destroying the origin information of the first frame?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-bcm_fixes-v10-0-57fff38e34ce@hartkopp.net?part=10