Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] net: devmem: leased RX bindings never unmapped before dma_dev is freed --
commit b250f1e4ffd826ff4e6f5c24e078e06c36365baf Author: Mina Almasry <[email protected]> net: devmem: unmap dma_buf synchronously on queue uninstall This patch adds synchronous dma_buf unmapping when RX queues are closed, uninstalled, or when the bound net_device is unregistered, to avoid a slab use-after-free in dma_unmap_sg_attrs() if dma_dev is freed while the devmem binding is still alive. > diff --git a/net/core/devmem.c b/net/core/devmem.c > index 32fafbfa1359e..4610de805ef08 100644 > --- a/net/core/devmem.c > +++ b/net/core/devmem.c [ ... ] > @@ -143,12 +228,45 @@ void net_devmem_unbind_dmabuf(struct > net_devmem_dmabuf_binding *binding) > > rxq_idx = get_netdev_rx_queue_index(rxq); > > - netif_mp_close_rxq(binding->dev, rxq_idx, &mp_params); > + if (netif_mp_close_rxq(binding->dev, rxq_idx, &mp_params)) > + busy = true; > + } > + > + if (binding->direction == DMA_FROM_DEVICE && binding->dev && !busy) { > + net_devmem_dmabuf_binding_unmap(binding); > + WRITE_ONCE(binding->dev, NULL); > } > > percpu_ref_kill(&binding->ref); > } > > +void net_devmem_dev_unregister(struct net_device *dev) > +{ > + struct net_devmem_dmabuf_binding *binding; > + unsigned long xa_idx; > + > + netdev_assert_locked(dev); > + > + if (dev->reg_state != NETREG_UNREGISTERING) > + return; > + > + xa_lock(&net_devmem_dmabuf_bindings); > + xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) { > + if (binding->direction != DMA_FROM_DEVICE || > + READ_ONCE(binding->dev) != dev || > + xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx, > + NET_DEVMEM_BINDING_LIVE)) > + continue; > + > + xa_unlock(&net_devmem_dmabuf_bindings); > + if (!netif_is_queue_leasee(dev)) > + net_devmem_dmabuf_binding_unmap(binding); > + xa_lock(&net_devmem_dmabuf_bindings); > + WRITE_ONCE(binding->dev, NULL); > + } > + xa_unlock(&net_devmem_dmabuf_bindings); > +} [ ... ] > @@ -551,16 +669,17 @@ static void mp_dmabuf_devmem_uninstall(void *mp_priv, > unsigned long xa_idx; > > xa_for_each(&binding->bound_rxqs, xa_idx, bound_rxq) { > - if (bound_rxq == rxq) { > - xa_erase(&binding->bound_rxqs, xa_idx); > - if (xa_empty(&binding->bound_rxqs)) { > - mutex_lock(&binding->lock); > - ASSERT_EXCLUSIVE_WRITER(binding->dev); > - WRITE_ONCE(binding->dev, NULL); > - mutex_unlock(&binding->lock); > - } > - break; > + if (bound_rxq != rxq) > + continue; > + > + xa_erase(&binding->bound_rxqs, xa_idx); > + if (xa_empty(&binding->bound_rxqs)) { > + if (!rxq->lease) > + net_devmem_dmabuf_binding_unmap(binding); > + ASSERT_EXCLUSIVE_WRITER(binding->dev); > + WRITE_ONCE(binding->dev, NULL); > } > + break; > } > } [Severity: High] As reported by Sashiko on v2 [1], for an RX binding on a leased queue (for example via netkit), can the binding still be left mapped after the backing physical device is unregistered and its dma_dev is freed? When netdev_nl_bind_rx_doit() binds to a virtual queue-leasee device, the dmabuf is attached to the physical device's dma_dev, but binding->dev is set to the virtual device and net_devmem_bind_dmabuf_to_queue() records the virtual rxq in binding->bound_rxqs while netif_mp_open_rxq() installs mp_params on the physical rxq. When the virtual device unregisters (either directly or via netkit_check_lease_unregister() when the physical device unregisters), ndo_uninit runs: netkit_uninit() netkit_queue_unlease() netdev_rx_queue_unlease(virt_rxq, phys_rxq) netif_rxq_cleanup_unlease(phys_rxq, virt_rxq) __netif_mp_close_rxq(phys_rxq->dev, rxq_idx, &p) __netif_mp_uninstall_rxq(virt_rxq, &p) mp_dmabuf_devmem_uninstall(binding, virt_rxq) Because netdev_rx_queue_unlease() only clears rxq_src->lease and rxq_dst->lease after netif_rxq_cleanup_unlease() returns: net/core/netdev_rx_queue.c:netdev_rx_queue_unlease() { ... netif_rxq_cleanup_unlease(rxq_src, rxq_dst); WRITE_ONCE(rxq_src->lease, NULL); WRITE_ONCE(rxq_dst->lease, NULL); ... } rxq->lease is still non-NULL in mp_dmabuf_devmem_uninstall(), so it skips net_devmem_dmabuf_binding_unmap(binding) while emptying binding->bound_rxqs and setting binding->dev to NULL. If the netlink socket is closed afterwards, net_devmem_unbind_dmabuf() sees binding->dev == NULL and also skips unmapping. Similarly, if the netlink socket was closed earlier and netif_mp_close_rxq() failed (busy == true), net_devmem_dev_unregister() on the virtual device skips net_devmem_dmabuf_binding_unmap(binding) because netif_is_queue_leasee(dev) is true, and then clears binding->dev to NULL. In both cases, when the physical device finishes unregistering, dev_memory_provider_uninstall() and net_devmem_dev_unregister() on the physical device cannot find or unmap the binding because binding->bound_rxqs is empty and binding->dev was the virtual device (and is now NULL). Once the physical device frees its dma_dev, won't __net_devmem_dmabuf_binding_free() later hit the same slab use-after-free in dma_unmap_sg_attrs()? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
