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

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

Pre-existing issues:
- [High] `bcm_rx_setup()` and `bcm_tx_setup()` use `list_add()` instead of `list_add_rcu()` when inserting into lists read via RCU.
- [High] `bcm_proc_show()` incorrectly iterates `bo->tx_ops` using the non-RCU `list_for_each_entry()` without holding the socket lock.
- [Medium] The NETDEV_DOWN handler in bcm_notify() reads bo->bound and bo->ifindex locklessly, racing with concurrent writers and invalidating the commit's concurrency claims.
--

--- Patch [2]: [PATCH v12 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit a8c67f18d60953102178fb9e6b18b46fea68d7fe
Author: Oliver Hartkopp <[email protected]>
Subject: can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

This commit addresses a race condition in bcm_sendmsg() where lockless
reads of bo->bound and bo->ifindex could lead to inconsistent states.
It moves the ifindex read and bound re-validation inside the socket
lock section to serialize with writers. It also fixes a silent failure in
bcm_rx_setup() when a target device concurrently 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: High]
This isn't a bug introduced by this patch, but do bcm_rx_setup() and 
bcm_tx_setup() use list_add() instead of list_add_rcu() when inserting into 
lists read via RCU?

net/can/bcm.c:bcm_rx_setup() {
    ...
    list_add(&op->list, &bo->rx_ops);
    ...
}

net/can/bcm.c:bcm_tx_setup() {
    ...
    list_add(&op->list, &bo->tx_ops);
    ...
}

Since /proc/net/can/bcm iterates these lists under rcu_read_lock(), could a 
concurrent reader traverse uninitialized list pointers due to the lack of 
smp_store_release() barriers?

[ ... ]

> @@ -1535,7 +1564,12 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
>  				bo->bcm_proc_read = NULL;
>  			}
>  #endif
> -			bo->bound   = 0;
> +			/* Paired with the lockless fast-path check in
> +			 * bcm_sendmsg(); bo->ifindex itself is only ever
> +			 * accessed under lock_sock() so it needs no
> +			 * annotation.
> +			 */
> +			WRITE_ONCE(bo->bound, 0);
>  			bo->ifindex = 0;
>  			notify_enodev = 1;
>  		}

[Severity: Medium]
This is a pre-existing issue, but the comment states that bo->ifindex is 
only ever accessed under lock_sock(). Does the NETDEV_DOWN handler in 
bcm_notify() read bo->bound and bo->ifindex locklessly?

net/can/bcm.c:bcm_notify() {
    ...
    case NETDEV_DOWN:
        if (bo->bound && bo->ifindex == dev->ifindex) {
    ...
}

Could a concurrent bcm_connect() or bcm_release() modify bo->bound and 
bo->ifindex under lock_sock() while the NETDEV_DOWN event triggers this 
lockless read, invalidating the comment's claims about access safety?

[Severity: High]
This is a pre-existing issue, but does bcm_proc_show() incorrectly iterate 
bo->tx_ops using the non-RCU list_for_each_entry() without holding the 
socket lock?

net/can/bcm.c:bcm_proc_show() {
    ...
    list_for_each_entry(op, &bo->tx_ops, list) {
    ...
}

If a user creates or deletes a TX operation via bcm_sendmsg() concurrently 
with another thread reading /proc/net/can/bcm, could this cause data races 
on list pointers because it lacks rcu_dereference() protection?

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