Re: [PATCH net-next 08/37] can: bcm: fix filter removal race between release and netns change
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-16 02:37:06
Also in:
linux-can
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/ (local) Link: https://patch.msgid.link/20261014142114.280279-3-socketcan@hartkopp.net (local)
quoted hunk ↗ jump to hunk
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.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -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