Re: [PATCH net v2 2/4] net: devmem: detach TX bindings on NETDEV_UNREGISTER

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: O(N) synchronize_net() calls under rtnl_lock cause RTNL stall and IPI storm
- [High] net: devmem: NULL deref in netif_mp_close_rxq() on concurrent unregister
--

commit 92717c23489a6c952c6219d6248685cf5a364b91
Author: Mina Almasry <almasrymina@google.com>
Subject: net: devmem: detach TX bindings on NETDEV_UNREGISTER

This patch extends net_devmem_dev_unregister() to detach TX bindings when
a device is unregistered. It waits for in-flight readers via
synchronize_net() and unmaps the dma_buf to prevent dangling pointers.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 02d607cc87217..a1c029f6ae47f 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
[ ... ]
> +static void
> +net_devmem_dev_unregister_tx(struct net_devmem_dmabuf_binding *binding,
> +			     unsigned long xa_idx, struct net_device *dev)
> +{
> +	if (READ_ONCE(binding->dev) == dev) {
> +		xa_clear_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +			      NET_DEVMEM_BINDING_LIVE);
> +
> +		mutex_lock(&binding->lock);
> +		WRITE_ONCE(binding->dev, NULL);
> +		WRITE_ONCE(binding->vdev, NULL);
> +		mutex_unlock(&binding->lock);
> +
> +		/* Ensure no tx net_devmem_lookup_dmabuf() or
> +		 * validate_xmit_unreadable_skb() are in flight after detach.
> +		 */
> +		synchronize_net();

[Severity: High]
Does this O(N) synchronize_net() inside a loop cause a potential denial of
service?

Because net_devmem_dev_unregister() iterates over all bindings using
xa_for_each() and calls net_devmem_dev_unregister_tx() for each, this
synchronize_net() executes sequentially for every matching TX binding.

This executes in the NETDEV_UNREGISTER notifier chain under the global
rtnl_lock, which causes synchronize_net() to fall back to
synchronize_rcu_expedited(). For a large number of bindings, this could force
numerous consecutive expedited RCU grace periods, sending a massive amount of
IPIs system-wide and stalling the RTNL lock.

Could we lift the synchronize_net() out of the per-binding loop, batch the
pointer clearing, issue a single synchronize_net(), and then perform the
unmapping?

> +
> +		mutex_lock(&binding->lock);
> +		net_devmem_dmabuf_binding_unmap(binding);
> +		mutex_unlock(&binding->lock);
> +	} else if (READ_ONCE(binding->vdev) == dev) {
> +		mutex_lock(&binding->lock);
> +		WRITE_ONCE(binding->vdev, NULL);
> +		mutex_unlock(&binding->lock);
> +
> +		synchronize_net();
> +	}
> +}
> +
>  void net_devmem_dev_unregister(struct net_device *dev)
>  {
>  	struct net_devmem_dmabuf_binding *binding;
> @@ -201,6 +231,8 @@ void net_devmem_dev_unregister(struct net_device *dev)
>  	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
>  		if (binding->direction == DMA_FROM_DEVICE)
>  			net_devmem_dev_unregister_rx(binding, xa_idx, dev);

[Severity: High]
Is there a NULL pointer dereference risk here if net_devmem_unbind_dmabuf()
runs concurrently with device unregistration?

If unregister_netdevice_many_notify() calls dev_memory_provider_uninstall()
and then drops the lock, the following race could occur:

1. netdev_nl_sock_priv_destroy() calls net_devmem_unbind_dmabuf(), clearing
   the LIVE mark and entering synchronize_net().

2. The unregister notifier triggers net_devmem_dev_unregister_rx() here.

3. Because the LIVE mark was cleared in step 1, net_devmem_dev_unregister_rx()
   bypasses its early return and unconditionally sets binding->dev = NULL.

4. When net_devmem_unbind_dmabuf() resumes, it executes
   xa_for_each(&binding->bound_rxqs) and calls
   netif_mp_close_rxq(binding->dev) using the now NULL binding->dev.

Could this crash the kernel at dev->real_num_rx_queues in netif_mp_close_rxq()?

> +		else
> +			net_devmem_dev_unregister_tx(binding, xa_idx, dev);
>  	}
>  	mutex_unlock(&net_devmem_lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012174300.2338784-1-almasrymina@google.com?part=2