Re: [PATCH v11 09/11] can: bcm: fix stale rx/tx ops after device removal

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

Pre-existing issues:
- [High] Data race on op->rx_stamp and op->rx_ifindex in bcm_rx_handler causes incorrect metadata to be delivered to userspace.
--

--- Patch [9]: [PATCH v11 09/11] can: bcm: fix stale rx/tx ops after device removal ---
commit ef4eaedce01b041fb9a737217fc61cc4150a0e37
Author: Oliver Hartkopp <[email protected]>

can: bcm: fix stale rx/tx ops after device removal

This commit fixes issues with stale rx and tx operations after a CAN device
is removed. It ensures rx operations are re-registered if they were torn
down by a concurrent NETDEV_UNREGISTER, and cancels the hrtimer for tx
operations to prevent runaway timers.

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 5660165cdf0eb..fbf1cbb7c99fe 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1714,11 +1735,19 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
>  	case NETDEV_UNREGISTER:
>  		lock_sock(sk);
>  
> -		/* remove device specific receive entries */
> +		/* rx_ops: remove device specific receive entries */
>  		list_for_each_entry(op, &bo->rx_ops, list)
>  			if (op->rx_reg_dev == dev)
>  				bcm_rx_unreg(dev, op);

[Severity: High]
This is a pre-existing issue, but is there a data race on op->rx_stamp and
op->rx_ifindex in bcm_rx_handler() that causes incorrect metadata to be
delivered to userspace?

If a CAN frame matching the same ANYDEV filter is received concurrently on
different interfaces, bcm_rx_handler() can execute simultaneously on
different CPUs. Could both CPUs write to the shared op->rx_stamp and
op->rx_ifindex without holding a lock?

net/can/bcm.c:bcm_rx_handler() {
	/* 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);
}

When CPU 1 subsequently acquires op->bcm_rx_update_lock and calls
bcm_send_to_user(), will it read the values overwritten by CPU 2?

net/can/bcm.c:bcm_send_to_user() {
	if (has_timestamp) {
		/* restore rx timestamp */
		skb->tstamp = op->rx_stamp;
	}

	/* ... */

	addr = (struct sockaddr_can *)skb->cb;
	memset(addr, 0, sizeof(*addr));
	addr->can_family  = AF_CAN;
	addr->can_ifindex = op->rx_ifindex;
}

For ANYDEV sockets, the ifindex returned in sockaddr_can is the sole source
of truth for identifying the origin bus, and delivering the wrong ifindex
can cause userspace gateway applications to misroute CAN frames.

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