Re: [PATCH v13 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] Unprotected concurrent access to op->rx_stamp and op->rx_ifindex in bcm_rx_handler causes data races and torn 64-bit reads on 32-bit architectures.
--

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

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

This commit fixes issues where an RX_SETUP update could silently skip
registering if a concurrent NETDEV_UNREGISTER occurred. It also ensures
that cyclic transmissions are stopped when a device is removed.

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..10dd99ecce70e 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1483,18 +1495,32 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  				err = -ENODEV;
>  			}
>  
> -		} else
> +		} else {
>  			err = can_rx_register(sock_net(sk), NULL, op->can_id,
>  					      REGMASK(op->can_id),
>  					      bcm_rx_handler, op, "bcm", sk);
> +		}

[Severity: High]
This is a pre-existing issue, but is there a data race when bcm_rx_handler()
accesses op->rx_stamp and op->rx_ifindex?

When a BCM socket is bound to ifindex 0 (all CAN devices), multiple CAN
frames arriving simultaneously on different CAN interfaces trigger
bcm_rx_handler() concurrently. The handler unconditionally writes to
op->rx_stamp and op->rx_ifindex outside of op->bcm_rx_update_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;
    ...
}

These fields are then read in bcm_send_to_user(), sometimes locklessly (e.g.
when called from bcm_rx_timeout_handler()):

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

Does this cause torn 64-bit reads of op->rx_stamp on 32-bit architectures,
and can the interface index be logically swapped with that of a concurrent
frame?

[ ... ]

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