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

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

[Oliver Hartkopp]

bcm_release() removes the socket from bcm_notifier_list before it takes
any lock that serializes against the netdev notifier chain, which runs
under RTNL. A socket that leaves the list in this window is skipped by
bcm_notify() and has to remove its CAN filters itself.

This is the same problem as reported for isotp_release() and fixed in a
separate patch: if the bound device changes its namespace in this
window, can_rx_unregister() can see a different netns than the one
passed in by bcm_rx_unreg() and silently skips the removal. The op is
freed anyway and the remaining receiver leads to a use-after-free in
bcm_rx_handler().

Take rtnl_lock() before removing the socket from the notifier list, so
that bcm_release() and bcm_notify() can not both skip the removal and
dev_net(dev) is stable while the filters are removed. RTNL is dropped
before synchronize_rcu(). As bcm_busy_notifier can not point to the
released socket under RTNL, replace the busy-wait loop with a
WARN_ON_ONCE().

Fixes: 8e8cda6d737d ("can: initial support for network namespaces")
Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
Cc: Norbert Szetei <norbert@doyensec.com>
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3d637a1e0ac1..4765c14947fc 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1924,16 +1924,18 @@ static int bcm_release(struct socket *sock)
 	net = sock_net(sk);
 	bo = bcm_sk(sk);
 
 	/* remove bcm_ops, timer, rx_unregister(), etc. */
 
+	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);
-	while (bcm_busy_notifier == bo) {
-		spin_unlock(&bcm_notifier_lock);
-		schedule_timeout_uninterruptible(1);
-		spin_lock(&bcm_notifier_lock);
-	}
+	WARN_ON_ONCE(bcm_busy_notifier == bo);
 	list_del(&bo->notifier);
 	spin_unlock(&bcm_notifier_lock);
 
 	lock_sock(sk);
 
@@ -1971,10 +1973,12 @@ static int bcm_release(struct socket *sock)
 					  REGMASK(op->can_id),
 					  bcm_rx_handler, op);
 
 	}
 
+	rtnl_unlock();
+
 	synchronize_rcu();
 
 	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
 		list_del_rcu(&op->list);
 		bcm_remove_op(op);
-- 
2.53.0

Re: [PATCH] can: bcm: fix filter removal race between release and netns change

[netdev-bot+sashiko]

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

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

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

- [High] bcm_release() now holds RTNL across remove_proc_entry() for the
  per-socket /proc/net/can-bcm/<ino> file and across the whole tx_ops…

--

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

This patch takes rtnl_lock() in bcm_release() before the socket is removed
from bcm_notifier_list. That way bcm_release() and bcm_notify() can't both
skip removing the CAN filters while the bound device changes namespace.
RTNL is dropped before synchronize_rcu(), and the busy-wait loop on
bcm_busy_notifier is replaced with a WARN_ON_ONCE().

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..4765c14947fc3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1926,12 +1926,14 @@ static int bcm_release(struct socket *sock)
>  
>  	/* remove bcm_ops, timer, rx_unregister(), etc. */
>  
> +	rtnl_lock();
> +

[Severity: High]
Once rtnl_lock() is taken here, RTNL stays held across lock_sock(), the
procfs removal and the tx_ops teardown. It is only released by
rtnl_unlock() after the rx_ops unregistration loop:

net/can/bcm.c:bcm_release() {
    ...
	lock_sock(sk);

#if IS_ENABLED(CONFIG_PROC_FS)
	/* remove procfs entry */
	if (net->can.bcmproc_dir && bo->bcm_proc_read)
		remove_proc_entry(bo->procname, net->can.bcmproc_dir);
#endif /* CONFIG_PROC_FS */

	list_for_each_entry_safe(op, next, &bo->tx_ops, list) {
		list_del_rcu(&op->list);
		bcm_remove_op(op);
	}
    ...
}

Can remove_proc_entry() block forever here while RTNL is held?

remove_proc_entry() calls proc_entry_rundown(), which waits for every
in-flight user of the entry:

fs/proc/inode.c:proc_entry_rundown() {
    ...
	if (atomic_add_return(BIAS, &de->in_use) != BIAS)
		wait_for_completion(&c);
    ...
}

bcm_connect() creates the per-socket entry with proc_create_net_single()
and mode 0644, so any local user can open it. The entry is not permanent,
so proc_reg_read() holds use_pde() across seq_read(), and that includes
the copy to the user buffer:

fs/proc/inode.c:proc_reg_read() {
    ...
	} else if (use_pde(pde)) {
		rv = pde_read(pde, file, buf, count, ppos);
		unuse_pde(pde);
	}
    ...
}

A user can stall that copy, for example by reading into a FUSE-backed
mmap buffer. The sequence would then be:

socket(PF_CAN, SOCK_DGRAM, CAN_BCM)
connect()     /* ifindex 0 is accepted, proc entry is created */
read() of /proc/net/can-bcm/<ino> into the stalled buffer
close()
  bcm_release()
    rtnl_lock()
    remove_proc_entry()
      proc_entry_rundown()
        wait_for_completion()   /* RTNL still held */

Would every other rtnl_lock() user on the system then hang behind this
close()? That includes ip link, netns creation and teardown, and device
registration.

Before this patch, the same wait ran under lock_sock() only and affected
just this socket. bcm_notify() already calls remove_proc_entry() under
RTNL, but only on NETDEV_UNREGISTER of the bound device. This path can be
reached from a plain close().

Even with no stalled reader, the tx_ops loop runs bcm_remove_op() under
RTNL, with two hrtimer_cancel() calls per op. The RTNL hold time therefore
grows with the number of tx ops.

RTNL only seems to be needed for the bcm_notifier_list removal and the
rx_ops can_rx_unregister()/bcm_rx_unreg() loop. Could the procfs removal
and the tx_ops teardown be moved out of the RTNL section, either before
rtnl_lock() or after rtnl_unlock()?

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013113628.104020-1-socketcan%40hartkopp.net

Re: [PATCH] can: bcm: fix filter removal race between release and netns change

[Oliver Hartkopp]

This is an old feedback.

Corrected patch based on this sashiko review is already on its way:

https://lore.kernel.org/linux-can/20261014142114.280279-3-socketcan@hartkopp.net/


On 15.10.26 02:53, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> 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).
> 
> - [High] bcm_release() now holds RTNL across remove_proc_entry() for the
>    per-socket /proc/net/can-bcm/<ino> file and across the whole tx_ops…
> 
> --
> 
> can: bcm: fix filter removal race between release and netns change
> 
> This patch takes rtnl_lock() in bcm_release() before the socket is removed
> from bcm_notifier_list. That way bcm_release() and bcm_notify() can't both
> skip removing the CAN filters while the bound device changes namespace.
> RTNL is dropped before synchronize_rcu(), and the busy-wait loop on
> bcm_busy_notifier is replaced with a WARN_ON_ONCE().
> 
> 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..4765c14947fc3 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -1926,12 +1926,14 @@ static int bcm_release(struct socket *sock)
>>   
>>   	/* remove bcm_ops, timer, rx_unregister(), etc. */
>>   
>> +	rtnl_lock();
>> +
> 
> [Severity: High]
> Once rtnl_lock() is taken here, RTNL stays held across lock_sock(), the
> procfs removal and the tx_ops teardown. It is only released by
> rtnl_unlock() after the rx_ops unregistration loop:
> 
> net/can/bcm.c:bcm_release() {
>      ...
> 	lock_sock(sk);
> 
> #if IS_ENABLED(CONFIG_PROC_FS)
> 	/* remove procfs entry */
> 	if (net->can.bcmproc_dir && bo->bcm_proc_read)
> 		remove_proc_entry(bo->procname, net->can.bcmproc_dir);
> #endif /* CONFIG_PROC_FS */
> 
> 	list_for_each_entry_safe(op, next, &bo->tx_ops, list) {
> 		list_del_rcu(&op->list);
> 		bcm_remove_op(op);
> 	}
>      ...
> }
> 
> Can remove_proc_entry() block forever here while RTNL is held?
> 
> remove_proc_entry() calls proc_entry_rundown(), which waits for every
> in-flight user of the entry:
> 
> fs/proc/inode.c:proc_entry_rundown() {
>      ...
> 	if (atomic_add_return(BIAS, &de->in_use) != BIAS)
> 		wait_for_completion(&c);
>      ...
> }
> 
> bcm_connect() creates the per-socket entry with proc_create_net_single()
> and mode 0644, so any local user can open it. The entry is not permanent,
> so proc_reg_read() holds use_pde() across seq_read(), and that includes
> the copy to the user buffer:
> 
> fs/proc/inode.c:proc_reg_read() {
>      ...
> 	} else if (use_pde(pde)) {
> 		rv = pde_read(pde, file, buf, count, ppos);
> 		unuse_pde(pde);
> 	}
>      ...
> }
> 
> A user can stall that copy, for example by reading into a FUSE-backed
> mmap buffer. The sequence would then be:
> 
> socket(PF_CAN, SOCK_DGRAM, CAN_BCM)
> connect()     /* ifindex 0 is accepted, proc entry is created */
> read() of /proc/net/can-bcm/<ino> into the stalled buffer
> close()
>    bcm_release()
>      rtnl_lock()
>      remove_proc_entry()
>        proc_entry_rundown()
>          wait_for_completion()   /* RTNL still held */
> 
> Would every other rtnl_lock() user on the system then hang behind this
> close()? That includes ip link, netns creation and teardown, and device
> registration.
> 
> Before this patch, the same wait ran under lock_sock() only and affected
> just this socket. bcm_notify() already calls remove_proc_entry() under
> RTNL, but only on NETDEV_UNREGISTER of the bound device. This path can be
> reached from a plain close().
> 
> Even with no stalled reader, the tx_ops loop runs bcm_remove_op() under
> RTNL, with two hrtimer_cancel() calls per op. The RTNL hold time therefore
> grows with the number of tx ops.
> 
> RTNL only seems to be needed for the bcm_notifier_list removal and the
> rx_ops can_rx_unregister()/bcm_rx_unreg() loop. Could the procfs removal
> and the tx_ops teardown be moved out of the RTNL section, either before
> rtnl_lock() or after rtnl_unlock()?
> 
>> +	/* 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.
>> +	 */
> 
> [ ... ]
>