Re: [PATCH] can: remove CAN filters independent from namespace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Permanent struct net_device reference leak on error path in bcm_connect().
- [Medium] Cross-namespace data corruption in BCM ANYDEV receive operations.
- [Medium] Timer cancellation regression leading to leaked BCM TX timers.
- [Medium] Per-namespace CAN receive list statistics leak in isotp_release().
--

commit 510b2a0288244edb9bcc3905c8c1576ae80db01f
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: remove CAN filters independent from namespace

This patch removes the namespace equality check in the netdev notifiers for
BCM, ISOTP and RAW sockets to ensure CAN filters are removed even when the
device's namespace changes. It also introduces tracked netdevice pointers
for BCM and converts isotp.c to use the device's namespace during
unregistration.

Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 3d637a1e0ac1a..42851d37cc9ae 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1223,6 +1246,12 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		op->sk = sk;
>  		sock_hold(sk);
>  		op->ifindex = ifindex;
> +		tx_dev = dev_get_by_index(sock_net(sk), ifindex);
> +		if (tx_dev) {
> +			op->tx_dev = tx_dev;
> +			netdev_hold(tx_dev, &op->tx_dev_tracker, GFP_KERNEL);
> +			dev_put(tx_dev);
> +		}
>  
>  		/* initialize uninitialized (kzalloc) structure */
>  		hrtimer_setup(&op->timer, bcm_tx_timeout_handler, CLOCK_MONOTONIC,

[Severity: Medium]
If dev_get_by_index() returns NULL here because the interface does not exist
yet, op->tx_dev remains NULL but the timer is still armed below.

If the device is created later, bcm_can_tx() dynamically resolves it and
succeeds. However, if the device is then unregistered, bcm_notify() skips
cancelling the timer because it strictly checks op->tx_dev == dev:

    list_for_each_entry(op, &bo->tx_ops, list)
        if (op->tx_dev == dev) {
            hrtimer_cancel(&op->timer);
            ...
        }

Will this leave the hrtimer permanently leaked and firing indefinitely,
repeatedly failing device lookups?

[ ... ]
> @@ -1794,9 +1823,6 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
>  	struct bcm_op *op;
>  	int notify_enodev = 0;
>  
> -	if (!net_eq(dev_net(dev), sock_net(sk)))
> -		return;
> -
>  	switch (msg) {
>  
>  	case NETDEV_UNREGISTER:

[Severity: Medium]
Since this namespace check was removed, bcm_notify() now processes events
across all network namespaces.

For ANYDEV sockets, the code later in this function handles NETDEV_UNREGISTER
like this:

    if (!op->ifindex) {
        spin_lock_bh(&op->bcm_rx_update_lock);
        if (op->if_detected == dev->ifindex)
            op->if_detected = 0;
        spin_unlock_bh(&op->bcm_rx_update_lock);
    }

Because ifindexes are only unique per-namespace, deleting a CAN interface
in one namespace triggers this globally.

Can this cause an ifindex collision that spuriously clears the if_detected
state of an ANYDEV socket operating in a completely different namespace,
causing it to drop active frame assembly or mismanage timers?

[ ... ]
> @@ -2031,11 +2066,14 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
>  		}
>  
>  		bo->ifindex = dev->ifindex;
> +		bo->dev = dev;
> +		netdev_hold(dev, &bo->dev_tracker, GFP_KERNEL);
>  		dev_put(dev);
>  
>  	} else {
>  		/* no interface reference for ifindex = 0 ('any' CAN device) */
>  		bo->ifindex = 0;
> +		bo->dev = NULL;
>  	}

[Severity: High]
Does this error path leak the acquired net_device reference?

If the subsequent proc_create_net_single() allocation fails, the function
jumps to the fail label and returns without calling netdev_put():

    if (!bo->bcm_proc_read) {
        ret = -ENOMEM;
        goto fail;
    }

The cleanup also skips setting WRITE_ONCE(bo->bound, 1). When the socket
is later closed, bcm_release() skips the release because bo->bound is 0:

    if (bo->bound) {
        WRITE_ONCE(bo->bound, 0);
        bo->ifindex = 0;
        if (bo->dev) {
            netdev_put(bo->dev, &bo->dev_tracker);
            bo->dev = NULL;
        }
    }

Could this permanently leak the net_device and tracker, preventing the
interface from ever being cleanly unregistered?

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 155530aedce2e..0835a4758a72f 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1492,11 +1492,11 @@ static int isotp_release(struct socket *sock)
>  	 */
>  	if (so->bound && so->dev) {
>  		if (isotp_register_rxid(so))
> -			can_rx_unregister(net, so->dev, so->rxid,
> +			can_rx_unregister(dev_net(so->dev), so->dev, so->rxid,
>  					  SINGLE_MASK(so->rxid),
>  					  isotp_rcv, sk);
>  
> -		can_rx_unregister(net, so->dev, so->txid,
> +		can_rx_unregister(dev_net(so->dev), so->dev, so->txid,
>  				  SINGLE_MASK(so->txid),
>  				  isotp_rcv_echo, sk);
>  		netdev_put(so->dev, &so->dev_tracker);

[Severity: Medium]
Does this leak the per-namespace receive list statistics in the socket's
original namespace?

If the device was moved to a different network namespace, this now passes
the new namespace (dev_net(so->dev)) to can_rx_unregister().

When can_rx_unregister() runs, it decrements the statistic in the
provided namespace:

    if (rcv_lists_stats->rcv_entries > 0)
        rcv_lists_stats->rcv_entries--;

Because can_rx_register() incremented the counter in the socket's original
namespace (sock_net(sk)), does decrementing it in the new namespace permanently
leak the count in the old one, while inappropriately decrementing the new one?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902182915.19486-1-socketcan@hartkopp.net?part=1