Re: [PATCH v11 11/11] can: bcm: track a single source interface for ANYDEV timeout/throttle ops

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Race condition between interface unregistration and in-flight packets permanently breaks ANYDEV sockets.
- [Medium] Data race and torn read on 64-bit ktime_t variables bypassing ANYDEV concurrency protection.
--

--- Patch [11]: [PATCH v11 11/11] can: bcm: track a single source interface for ANYDEV timeout/throttle ops ---
commit ed6fe9660233297822929009c213120acb00229f
Author: Oliver Hartkopp <[email protected]>

can: bcm: track a single source interface for ANYDEV timeout/throttle ops

An ANYDEV rx op with an active RX timeout/throttle timer has no defined
semantics when matching frames arrive from multiple interfaces, leading to
data corruption. This patch adds op->if_detected to track the first interface
that delivers a matching frame, rejecting frames from other interfaces.
The claim is released on NETDEV_UNREGISTER or timer reconfiguration.

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 10a883b50fc63..8cc0d2267582f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -776,6 +777,28 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  			return;
>  	}
>  
> +	/* An ANYDEV op with an active RX timeout and/or throttle timer
> +	 * tracks a single source interface: claim the first interface that
> +	 * delivers a matching frame and reject frames from any other one,
> +	 * before hrtimer_cancel() below can touch op->timer - this avoids
> +	 * racing bcm_rx_timeout_handler() across concurrent interfaces.
> +	 * RX_RTR_FRAME ops are excluded, as kt_ival1/kt_ival2 may briefly
> +	 * hold a stale value from an earlier non-RTR configuration.
> +	 */
> +	if (!op->ifindex && !(op->flags & RX_RTR_FRAME) &&
> +	    (op->kt_ival1 || op->kt_ival2)) {

[Severity: Medium]
Can the lockless read of these 64-bit ktime_t variables cause a torn read on
32-bit platforms?

If the torn read evaluates to zero while the timers are actually configured,
wouldn't it bypass the interface-tracking logic and allow concurrent frames
to clobber shared metadata?

> +		bool foreign;
> +
> +		spin_lock_bh(&op->bcm_rx_update_lock);
> +		if (!op->if_detected)
> +			op->if_detected = skb->dev->ifindex;

[Severity: High]
Can an in-flight packet spuriously re-claim the stale ifindex here?

In bcm_notify(), the claim is released when an interface goes down. Since
bcm_notify() executes before synchronize_net(), could an already dequeued
packet arrive here, see op->if_detected == 0, and re-claim the dead
interface's index?

This seems like it would permanently block future packets from other valid
interfaces.

> +		foreign = op->if_detected != skb->dev->ifindex;
> +		spin_unlock_bh(&op->bcm_rx_update_lock);
> +
> +		if (foreign)
> +			return;
> +	}
> +

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