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 <[email protected]> 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/[email protected]?part=2
