Re: [PATCH net-next 08/37] can: bcm: fix filter removal race between release and netns change

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit moves remove_proc_entry() ahead of rtnl_lock() so
  that procfs readers cannot stall RTNL from close().
- [Medium] bcm_release() now holds the global rtnl_mutex across the whole
  rx_ops unregistration loop (rtnl_lock() at net/can/bcm.c:1951,…
- [Low] Stale comment in bcm_release().

--

can: bcm: fix filter removal race between release and netns change

bcm_release() now takes rtnl_lock() before it removes the socket from
bcm_notifier_list. It holds RTNL across the rx_ops unregistration, so
bcm_release() and bcm_notify() can no longer both skip filter removal
while the bound device moves to another netns. The bcm_busy_notifier
busy-wait becomes a WARN_ON_ONCE(), and the procfs entry and tx_ops are
now removed before RTNL is taken.

Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
Link: https://patch.msgid.link/20261014142114.280279-3-socketcan@hartkopp.net

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 6759662e024c..301920011a30 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1926,21 +1926,19 @@ static int bcm_release(struct socket *sock)
>  
>  	/* remove bcm_ops, timer, rx_unregister(), etc. */
>  
> -	spin_lock(&bcm_notifier_lock);
> -	while (bcm_busy_notifier == bo) {
> -		spin_unlock(&bcm_notifier_lock);
> -		schedule_timeout_uninterruptible(1);
> -		spin_lock(&bcm_notifier_lock);
> -	}
> -	list_del(&bo->notifier);
> -	spin_unlock(&bcm_notifier_lock);
> -
> +	/* Remove the procfs entry and the tx_ops before taking RTNL:
> +	 * remove_proc_entry() waits for concurrent readers of the entry and
> +	 * must not hold up RTNL for them. Clear bo->bcm_proc_read so that
> +	 * bcm_notify() does not remove the entry a second time.
> +	 */
>  	lock_sock(sk);
>  
>  #if IS_ENABLED(CONFIG_PROC_FS)
>  	/* remove procfs entry */
> -	if (net->can.bcmproc_dir && bo->bcm_proc_read)
> +	if (net->can.bcmproc_dir && bo->bcm_proc_read) {
>  		remove_proc_entry(bo->procname, net->can.bcmproc_dir);
> +		bo->bcm_proc_read = NULL;
> +	}
>  #endif /* CONFIG_PROC_FS */

[Severity: Medium]
list_del(&bo->notifier) now runs after rtnl_lock(). That means this
first lock_sock() section runs while the socket is still on
bcm_notifier_list. Can a procfs reader still stall RTNL here, just
indirectly through the socket lock?

Consider this interleaving:

CPU1 (close)
bcm_release()
  lock_sock(sk)
  remove_proc_entry()
    proc_entry_rundown()
      wait_for_completion()      <- waits for the reader on CPU2

CPU2 (reader of /proc/net/can-bcm/<ino>)
proc_reg_read()
  use_pde()
  pde_read()                     <- copy to a FUSE or userfaultfd
                                    backed buffer blocks

CPU3 (NETDEV_UNREGISTER, RTNL held)
bcm_notifier()
  bcm_notify(bo, NETDEV_UNREGISTER, dev)
    lock_sock(sk)                <- blocks behind CPU1

bcm_notify() takes lock_sock(sk) before it checks whether any op or the
binding uses dev. So deleting or moving any CAN device in the socket's
netns would be enough.

Before this patch, the socket was unlinked first and the busy-wait made
sure bcm_notify() was not running for it, so this interleaving was not
possible.

The procfs entry is mode 0644 and is created on any connect(), including
can_ifindex = 0. With unprivileged user namespaces, a vcan device in a
user-owned netns can trigger the notifier, and RTNL is global.

Does this mean the commit message goal ("which could otherwise stall
RTNL from a plain close()") is only partly met?

There are similar RTNL stall paths already. bcm_sendmsg() holds
lock_sock() across memcpy_from_msg() in bcm_tx_setup() and
bcm_rx_setup(), and bcm_notify() itself calls remove_proc_entry() under
RTNL. This particular ordering is new with this patch, though.

[ ... ]

> @@ -1948,6 +1946,21 @@ static int bcm_release(struct socket *sock)
>  		bcm_remove_op(op);
>  	}
>  
> +	release_sock(sk);
> +
> +	rtnl_lock();
> +
> +	/* The netdev notifier chain runs under RTNL, so bcm_notifier() can't
> +	 * be walking bcm_notifier_list and point to this socket. The spinlock
> +	 * is still needed as bcm_init() adds sockets without holding RTNL.
> +	 */
> +	spin_lock(&bcm_notifier_lock);
> +	WARN_ON_ONCE(bcm_busy_notifier == bo);
> +	list_del(&bo->notifier);
> +	spin_unlock(&bcm_notifier_lock);
> +
> +	lock_sock(sk);
> +
>  	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
>  		/*
>  		 * Don't care if we're bound or not (due to netdev problems)

[Severity: Low]
This isn't a functional problem, but the comment a bit further down in
this rx_ops loop in bcm_release() still describes a concurrent
unregister:

			 * 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.

This loop now runs between rtnl_lock() and rtnl_unlock(), and the socket
stays on bcm_notifier_list until RTNL is taken. A NETDEV_UNREGISTER has
either already finished, in which case bcm_notify()->bcm_rx_unreg()
cleared op->rx_reg_dev, or it runs after this loop, when the socket is
no longer on the list.

Can a concurrent NETDEV_UNREGISTER still happen at this point? The old
comment seems to contradict the new comment added above it.

The same comment in bcm_delete_rx_op() is still accurate, because that
path does not hold RTNL. Should the copy in bcm_release() be updated?

> @@ -1973,6 +1986,8 @@ static int bcm_release(struct socket *sock)
>  
>  	}
>  
> +	rtnl_unlock();
> +
>  	synchronize_rcu();

[Severity: Medium]
The global rtnl_mutex is now held across the whole rx_ops
unregistration loop, and userspace decides how long that loop is. Is
that intended?

bcm_rx_setup() allocates one op per distinct (can_id, ifindex) with
kzalloc(OPSIZ, GFP_KERNEL). That allocation is not memcg accounted, and
there is no per-socket limit. bcm_connect() accepts can_ifindex == 0
without any device, and ANYDEV ops register on the all-devices list
through:

	err = can_rx_register(sock_net(sk), NULL, op->can_id,

So an unprivileged user with no CAN hardware can create a very large
number of ops.

On close(), each op calls can_rx_unregister(). That takes
spin_lock_bh(&net->can.rcvlists_lock) and walks the receiver hlist
linearly until it finds the match.

A second socket can put M newer entries in front of the victim's
entries in the same bucket. It can do this with EFF ids that fold to
the same bucket in effhash(), or with the same SFF id. RTNL would then
be held for roughly K * M list steps.

Before this patch, the same loop ran only under lock_sock(sk), so only
the closing task paid for it. Now it would block netlink and netdev
configuration in every network namespace.

The loop without cond_resched() already existed, and the attacker needs
a lot of kernel memory. What is new is that RTNL covers the whole loop.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015015336.816543-2-mkl%40pengutronix.de