Re: [PATCH v6 2/9] 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 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless, non-RCU iteration of the tx_ops and rx_ops lists in bcm_proc_show() races with concurrent list mutations, leading to list corruption and crashes.
- [High] bcm_release() leaks CAN receiver hooks and causes a Use-After-Free if the network device is unregistered concurrently.
- [Medium] bcm_rx_setup() silently fails to register the CAN RX hook when updating an existing operation for a recreated device.
- [Medium] bcm_notify() accesses bo->ifindex locklessly during NETDEV_DOWN, contradicting the patch's assumption and risking missed notifications.
--

--- Patch [2]: [PATCH v6 2/9] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 216efa99603464dc19876367ec3eb7d6e1414515
Author: Oliver Hartkopp <socketcan@hartkopp.net>
Date:   Wed Jul 8 09:22:28 2026 +0000

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

This commit fixes a lockless race condition between bcm_sendmsg() and
functions that mutate bo->bound and bo->ifindex by re-validating the
state under the socket lock. It also addresses a silent failure in
bcm_rx_setup() when the target device is removed concurrently.

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;
>  			}
>  
>  		} else

[Severity: Medium]
This is a pre-existing issue, but can bcm_rx_setup() silently fail to
register the CAN RX hook when updating an existing operation?

If a socket is bound to 'any' interface and a user issues an RX_SETUP
for a specific device that was previously removed and recreated, it
appears do_rx_register remains 0:

bcm_rx_setup() {
    ...
    if (op) {
        ...
        /* Only an update -> do not call can_rx_register() */
        do_rx_register = 0;
    }
}

Since bcm_notify() unregisters the hook and sets op->rx_reg_dev = NULL
when the device is removed, updating the dormant bcm_op without
registering the hook again leaves the socket unable to receive frames.

[ ... ]

> @@ -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 new comment states bo->ifindex is
only ever accessed under lock_sock(). Is this accurate?

Looking at NETDEV_DOWN handling in bcm_notify(), it accesses bo->ifindex
locklessly:

bcm_notify() {
    ...
    case NETDEV_DOWN:
        if (bo->bound && bo->ifindex == dev->ifindex) {
            sk->sk_err = ENETDOWN;
            ...
        }
}

Could a concurrent bcm_connect() cause this to read a stale bo->ifindex
and miss sending the ENETDOWN notification?

[ ... ]

> @@ -1676,7 +1710,7 @@ static int bcm_release(struct socket *sock)
>  
>  	/* remove device reference */
>  	if (bo->bound) {
> -		bo->bound   = 0;
> +		WRITE_ONCE(bo->bound, 0);
>  		bo->ifindex = 0;
>  	}

[Severity: High]
This is a pre-existing issue, but does bcm_release() risk leaking CAN
receiver hooks if the network device is unregistered concurrently?

When a CAN socket is closed, bcm_release() relies on dev_get_by_index()
to find the device and unregister the hook:

bcm_release() {
    ...
    dev = dev_get_by_index(net, op->ifindex);
    if (dev) {
        bcm_rx_unreg(dev, op);
        dev_put(dev);
    }
}

If the device is being unregistered concurrently, it is removed from the
network namespace index by unlist_netdevice(), so dev_get_by_index()
returns NULL. This causes bcm_release() to skip calling bcm_rx_unreg(),
leaving a dangling pointer in the CAN core's dev_rcv_lists that can lead
to a use-after-free.

[ ... ]

> @@ -1746,7 +1780,10 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
>  	}
>  #endif /* CONFIG_PROC_FS */
>  
> -	bo->bound = 1;
> +	/* bo->ifindex above is fully assigned before this point; pairs
> +	 * with the lockless fast-path check in bcm_sendmsg()
> +	 */
> +	WRITE_ONCE(bo->bound, 1);
>  
>  fail:
>  	release_sock(sk);

[Severity: Medium]
This is a pre-existing issue, but does this lack the necessary memory
barriers to enforce ordering?

Because bcm_notify() can read these fields locklessly during a NETDEV_DOWN
event, doesn't WRITE_ONCE() alone fail to guarantee that the store to
bo->ifindex is visible to other CPUs before bo->bound = 1 without an
explicit smp_wmb()?

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
between lockless list iteration in bcm_proc_show() and list mutations?

When a user reads /proc/net/can-bcm/*, bcm_proc_show() iterates over
bo->tx_ops and bo->rx_ops holding only the rcu_read_lock():

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

However, when adding elements, the writers appear to use list_add() instead
of list_add_rcu(), which fails to emit the necessary smp_store_release()
barriers:

bcm_tx_setup() {
    ...
    list_add(&op->list, &bo->tx_ops);
}

Can weakly-ordered CPUs make the new node's next pointer visible to the
lockless reader before the node itself is fully initialized, causing a
crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=2