Re: [PATCH v8 02/10] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Data race between lockless /proc readers and socket list modification due to missing RCU protections.
--

--- Patch [2]: [PATCH v8 02/10] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 2478889b563324c073bf7ec2b2aeeb796709e2ce
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

The patch fixes a race condition where lockless reads of bo->bound and
bo->ifindex could tear against concurrent locked updates. It also fixes a
silent failure in bcm_rx_setup() when the target device disappears.

Link: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index bdf53241bd7b3..b612135b017d1 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1323,6 +1323,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  
>  				op->rx_reg_dev = dev;
>  				dev_put(dev);
> +			} else {
> +				/* the requested device is gone - do not
> +				 * silently succeed without registering
> +				 */
> +				err = -ENODEV;
>  			}

[Severity: Critical]
This is a pre-existing issue, but does this code have a data race between
lockless /proc readers and socket list modifications?

In bcm_proc_show(), bo->rx_ops and bo->tx_ops are traversed locklessly under
rcu_read_lock(). However, bcm_rx_setup() and bcm_tx_setup() use the non-RCU
list_add() function to insert elements, omitting the required memory barrier:

net/can/bcm.c:bcm_rx_setup() {
    ...
		/* add this bcm_op to the list of the rx_ops */
		list_add(&op->list, &bo->rx_ops);
    ...
}

net/can/bcm.c:bcm_tx_setup() {
    ...
		/* add this bcm_op to the list of the tx_ops */
		list_add(&op->list, &bo->tx_ops);
    ...
}

Additionally, while rx_ops is traversed with list_for_each_entry_rcu(), tx_ops
is traversed without RCU primitives entirely:

net/can/bcm.c:bcm_proc_show() {
    ...
	rcu_read_lock();

	list_for_each_entry_rcu(op, &bo->rx_ops, list) {
    ...
	list_for_each_entry(op, &bo->tx_ops, list) {
    ...
}

Can an unprivileged local user trigger a kernel panic by opening the
/proc/net/can-bcm/<inode> file while concurrently adding operations via
RX_SETUP or TX_SETUP? Should list_add_rcu() and list_for_each_entry_rcu() be
used instead to ensure list integrity?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v8-0-8c2f5f3bc212@hartkopp.net?part=2