Re: [PATCH v6 8/9] can: bcm: add missing device refcount for CAN filter removal

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] BCM TX operations (tx_ops) are not cleaned up during NETDEV_UNREGISTER, causing continuous timer firing and potential cross-device traffic injection upon ifindex reuse.
- [Medium] Updating an existing BCM RX filter after device recreation silently fails to re-register the CAN receiver.
--

--- Patch [8]: [PATCH v6 8/9] can: bcm: add missing device refcount for CAN filter removal ---
commit e5a4c52c71d52a19d27614862f641c4332b115e5
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add missing device refcount for CAN filter removal

This patch fixes a CAN filter removal issue where a concurrent 
NETDEV_UNREGISTER can cause a lookup failure for a device by ifindex.
It prevents skipping can_rx_unregister() by holding a netdev reference on
op->rx_reg_dev from filter registration until unregistration, rather than
looking up the device by index again.

Link: https://sashiko.dev/#/patchset/20260707094716.63578-1-socketcan@hartkopp.net

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 300f1f88c4d2c..4bdca72f9327f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1455,7 +1454,17 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  						      bcm_rx_handler, op,
>  						      "bcm", sk);
>  
> -				op->rx_reg_dev = dev;
> +				/* keep a tracked reference so that a later
> +				 * unregister can safely reach the device even
> +				 * if a concurrent NETDEV_UNREGISTER has
> +				 * already unlisted it by ifindex
> +				 */
> +				if (!err) {
> +					op->rx_reg_dev = dev;
> +					netdev_hold(dev,
> +						    &op->rx_reg_dev_tracker,
> +						    GFP_KERNEL);
> +				}
>  				dev_put(dev);

[Severity: Medium]
This is a pre-existing issue, but does updating an existing BCM RX filter
after a device recreation silently fail to re-register the CAN receiver?

Looking at bcm_rx_setup():

    op = bcm_find_op(&bo->rx_ops, msg_head, ifindex);
    if (op) {
        ...
        /* Only an update -> do not call can_rx_register() */
        do_rx_register = 0;

Updates to existing ops unconditionally skip CAN registration, even if the
previous registration was torn down by a NETDEV_UNREGISTER event leaving
op->rx_reg_dev as NULL.

Could this cause the application to silently drop incoming frames?

[ ... ]
> @@ -1832,16 +1841,14 @@ static int bcm_release(struct socket *sock)
>  			 * Only remove subscriptions that had not
>  			 * been removed due to NETDEV_UNREGISTER
>  			 * in bcm_notifier()
> +			 *
> +			 * op->rx_reg_dev is a tracked reference taken
> +			 * when the subscription was registered, so it
> +			 * stays valid here even if a concurrent
> +			 * NETDEV_UNREGISTER already unlisted the device.
>  			 */
> -			if (op->rx_reg_dev) {
> -				struct net_device *dev;
> -
> -				dev = dev_get_by_index(net, op->ifindex);
> -				if (dev) {
> -					bcm_rx_unreg(dev, op);
> -					dev_put(dev);
> -				}
> -			}
> +			if (op->rx_reg_dev)
> +				bcm_rx_unreg(op->rx_reg_dev, op);

[Severity: High]
This isn't a bug introduced by this patch, but are TX operations properly
cleaned up during NETDEV_UNREGISTER?

When looking at bcm_notify():

    case NETDEV_UNREGISTER:
        lock_sock(sk);

        /* remove device specific receive entries */
        list_for_each_entry(op, &bo->rx_ops, list)
            if (op->rx_reg_dev == dev)
                bcm_rx_unreg(dev, op);

Only rx_ops are cleaned up upon NETDEV_UNREGISTER. The tx_ops appear to be
completely ignored, and op->ifindex is never cleared to halt transmissions.

This seems to allow the timer to fire endlessly in bcm_tx_timeout_handler():

    } else if (tx_ival2) {
        bcm_can_tx(op, NULL);
    }

    return bcm_tx_set_expiry(op, &op->timer) ?
        HRTIMER_RESTART : HRTIMER_NORESTART;

The timer handler continues to restart the timer even when the underlying
device has been destroyed. In bcm_can_tx(), dev_get_by_index() silently fails,
allowing the loop to continue endlessly.

Could this lead to cross-device traffic injection if the removed ifindex is
later reused by a new CAN interface?

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