Re: [PATCH v7 09/10] can: bcm: fix stale rx/tx ops after device removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Setting `op->ifindex = 0` for `tx_ops` on NETDEV_UNREGISTER orphans the operations, preventing reconfiguration and causing memory leaks.
- [Medium] `bcm_rx_setup()` silently ignores registration failures for existing operations, falsely reporting success to userspace.
--

--- Patch [9]: [PATCH v7 09/10] can: bcm: fix stale rx/tx ops after device removal ---
commit 685050073b3ab1afd17b250b634cb2dfd5941250
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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

This commit modifies bcm_rx_setup() to correctly re-register an RX operation 
when a concurrent NETDEV_UNREGISTER has torn down its previous registration. 
It also updates bcm_notify() to stop the TX timer and clear op->ifindex 
for affected tx_ops when a device is unregistered, preventing cyclic frames 
from being transmitted to unrelated devices.

Link: https://lore.kernel.org/linux-can/20260708094536.DDF821F00A3A@smtp.kernel.org/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 4bdca72f9327f..9d01931708081 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1473,18 +1485,21 @@ 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);
> -		if (err) {
> +		}
> +
> +		if (err && new_op) {

[Severity: Medium]
Does adding the new_op check here silently swallow registration errors for existing
operations?

If an existing operation (new_op == 0) attempts to re-register after being unregistered,
and dev_get_by_index() or can_rx_register() fails, err will be set (e.g., -ENODEV).
Because of the && new_op condition, the error is ignored and the function will return 
success to userspace, despite the comment above indicating "do not silently succeed 
without registering".

>  			/* this bcm rx op is broken -> remove it */
>  			bcm_remove_op(op);
>  			return err;
>  		}

[ ... ]

> @@ -1704,11 +1719,25 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
>  	case NETDEV_UNREGISTER:
>  		lock_sock(sk);
>  
> -		/* remove device specific receive entries */
> +		/* rx_ops: 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);
>  
> +		/* tx_ops: stop device specific cyclic transmissions on the
> +		 * vanishing ifindex and invalidate the op->ifindex.
> +		 */
> +		list_for_each_entry(op, &bo->tx_ops, list) {
> +			if (op->ifindex != dev->ifindex)
> +				continue;
> +
> +			spin_lock_bh(&op->bcm_tx_lock);
> +			op->ifindex = 0;

[Severity: High]
Does setting op->ifindex = 0 permanently orphan the operation and cause a memory leak?

The commit message states the index is cleared "so that a later TX_SETUP update can 
reconfigure it". However, bcm_tx_setup() rejects an ifindex of 0 with -ENODEV:

bcm_tx_setup() {
    ...
	if (!ifindex)
		return -ENODEV;
    ...
}

Furthermore, bcm_find_op() strictly requires an exact match on ifindex. 
Since userspace can't pass 0, and passing the old valid index will no longer match 
the zeroed op->ifindex, any subsequent TX_SETUP will fail to find it and will allocate a 
duplicate bcm_op. A TX_DELETE will similarly fail to find and remove the operation, 
leaving it as a zombie.

> +			spin_unlock_bh(&op->bcm_tx_lock);
> +
> +			hrtimer_cancel(&op->timer);
> +		}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v7-0-c78896b342e4@hartkopp.net?part=9