Re: [PATCH net v4 1/4] net: devmem: unmap dma_buf synchronously on queue uninstall

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 10/15, Mina Almasry wrote:
> When a bound net_device is unregistered while userspace still holds a
> netlink or TCP socket reference on a devmem binding,
> mp_dmabuf_devmem_uninstall() clears binding->dev to NULL so
> netdev_nl_sock_priv_destroy() will not touch the unregistered
> net_device, but leaves the dma_buf attachment mapped until
> __net_devmem_dmabuf_binding_free() runs later on socket close.
> 
> Because dma_buf_attach() does not take a reference on dma_dev, device
> removal frees dma_dev while the attachment is still mapped. Closing the
> netlink socket afterwards triggers a slab use-after-free in
> dma_unmap_sg_attrs():
> 
>   BUG: KASAN: slab-use-after-free in dma_unmap_sg_attrs+0x76/0x2b0
>   Read of size 8 at addr ffff88810fae64b0 by task kworker/3:1/53
>   Workqueue: events __net_devmem_dmabuf_binding_free
>   Call Trace:
>    dma_unmap_sg_attrs+0x76/0x2b0
>    unmap_udmabuf+0x8e/0x100
>    dma_buf_unmap_attachment+0x145/0x330
>    dma_buf_unmap_attachment_unlocked+0x96/0x130
>    __net_devmem_dmabuf_binding_free+0x156/0x2e0
>   Freed by task 9:
>    kfree+0x1b8/0x550
>    nsim_bus_dev_release+0xe/0x60
>    device_release+0xcf/0x250
> 
> Add net_devmem_dmabuf_binding_unmap() serialized under binding->lock
> and null out binding->sgt and binding->attachment after unmapping so
> unmapping is idempotent across queue uninstall and deferred binding
> destruction. Call it synchronously when the last bound RX queue is
> uninstalled in mp_dmabuf_devmem_uninstall(), except when a leased
> queue is unleased: the physical device is still registered and, if
> restarting its queue failed, may still be using the binding.
> 
> The same use-after-free happens when the netlink socket is closed
> before the device is removed but RX tokens keep the binding alive. Once
> its queues are closed, nothing DMAs into an RX binding anymore and the
> outstanding tokens only need the net_iovs, so unmap it in
> net_devmem_unbind_dmabuf(). Make netif_mp_close_rxq() return an error
> when a queue failed to restart and may still be using the binding. Such
> a binding stays mapped and keeps binding->dev. Record a queue in
> bound_rxqs before opening it, so that a failed bind never leaves a
> queue using the binding without unbind knowing about it.
> 
> Keep bindings in net_devmem_dmabuf_bindings until they are freed and
> hide unbound ones from TX lookups with an xarray mark, so that on
> NETDEV_UNREGISTER the remaining unbound RX bindings of the device can be
> found, detached and unmapped, unless the device is a virtual device
> leasing queues, for the same reason as above. The walk runs under the
> device's instance lock and is skipped when the device only moves to
> another netns. Like netdev_nl_sock_priv_destroy(),
> __net_devmem_dmabuf_binding_free() takes the instance lock of
> binding->dev before unmapping and erasing the binding, so a binding the
> walk finds under the xarray lock stays alive while it is detached.
> 
> Fixes: f8350a4358fc ("net: page_pool: add a mp hook to unregister_netdevice*")
> Fixes: 0afc44d8cdf6 ("net: devmem: fix kernel panic when netlink socket close after module unload")
> Suggested-by: Stanislav Fomichev <sdf@fomichev.me>
> Cc: Taehee Yoo <ap420073@gmail.com>
> Cc: Tariq Toukan <tariqt@nvidia.com>
> Cc: Kaifeng Wang <kaifengw@google.com>
> Assisted-by: LLM
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
> ---
> v4:
> - Unmap RX bindings in net_devmem_unbind_dmabuf() once their queues are
>   closed, and make netif_mp_close_rxq() report queue restart failures.
>   The NETDEV_UNREGISTER walk only handles bindings whose queue failed
>   to restart (Stanislav).
> - Drop net_devmem_lock. Walk the bindings under the xarray lock, and
>   take the instance lock of binding->dev in the final free (Stanislav).
> - Record a queue in bound_rxqs before opening it, so unbind knows about
>   every queue using the binding (Sashiko).
> 
> v3:
> - Run the NETDEV_UNREGISTER walk under the netdev instance lock so it
>   cannot race with net_devmem_unbind_dmabuf() (Sashiko).
> - Skip the walk when the device only moves to another netns (Sashiko).
> 
> v2:
> - Don't unmap when a leased queue is unleased (Sashiko).
> - Unmap unbound RX bindings on NETDEV_UNREGISTER; keep bindings in
>   net_devmem_dmabuf_bindings until freed, hidden from TX lookups by
>   NET_DEVMEM_BINDING_LIVE (Sashiko).
> - Update the binding->lock and binding->ref comments (Sashiko).
> ---
>  include/net/page_pool/memory_provider.h |   4 +-
>  net/core/devmem.c                       | 137 +++++++++++++++++++-----
>  net/core/devmem.h                       |  10 +-
>  net/core/netdev-genl.c                  |   1 +
>  net/core/netdev_rx_queue.c              |  26 +++--
>  5 files changed, 140 insertions(+), 38 deletions(-)
> 
> diff --git a/include/net/page_pool/memory_provider.h b/include/net/page_pool/memory_provider.h
> index 255ce4cfd9755..a999a9b28bc70 100644
> --- a/include/net/page_pool/memory_provider.h
> +++ b/include/net/page_pool/memory_provider.h
> @@ -26,8 +26,8 @@ void net_mp_niov_clear_page_pool(struct net_iov *niov);
>  int netif_mp_open_rxq(struct net_device *dev, unsigned int rxq_idx,
>  		      const struct pp_memory_provider_params *p,
>  		      struct netlink_ext_ack *extack);
> -void netif_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
> -			const struct pp_memory_provider_params *old_p);
> +int netif_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
> +		       const struct pp_memory_provider_params *old_p);
>  
>  /**
>    * net_mp_netmem_place_in_cache() - give a netmem to a page pool
> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index f4d60654ce7fd..20d95b24d5951 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -12,6 +12,7 @@
>  #include <linux/mm.h>
>  #include <linux/netdevice.h>
>  #include <linux/types.h>
> +#include <net/netdev_lock.h>
>  #include <net/netdev_queues.h>
>  #include <net/netdev_rx_queue.h>
>  #include <net/page_pool/helpers.h>
> @@ -20,6 +21,7 @@
>  #include <net/tcp.h>
>  #include <trace/events/page_pool.h>
>  
> +#include "dev.h"
>  #include "devmem.h"
>  #include "mp_dmabuf_devmem.h"
>  #include "page_pool_priv.h"
> @@ -27,6 +29,7 @@
>  /* Device memory support */
>  
>  static DEFINE_XARRAY_FLAGS(net_devmem_dmabuf_bindings, XA_FLAGS_ALLOC1);
> +#define NET_DEVMEM_BINDING_LIVE XA_MARK_1
>  
>  static const struct memory_provider_ops dmabuf_devmem_ops;
>  
> @@ -58,9 +61,26 @@ static void net_devmem_dmabuf_binding_release(struct percpu_ref *ref)
>  	schedule_work(&binding->unbind_w);
>  }
>  
> +static void
> +net_devmem_dmabuf_binding_unmap(struct net_devmem_dmabuf_binding *binding)
> +{
> +	lockdep_assert_held(&binding->lock);
> +
> +	if (!binding->sgt)
> +		return;
> +
> +	dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt,
> +					  binding->direction);
> +	dma_buf_detach(binding->dmabuf, binding->attachment);
> +	binding->sgt = NULL;
> +	binding->attachment = NULL;
> +}
> +
>  void __net_devmem_dmabuf_binding_free(struct work_struct *wq)
>  {
>  	struct net_devmem_dmabuf_binding *binding = container_of(wq, typeof(*binding), unbind_w);
> +	netdevice_tracker dev_tracker;
> +	struct net_device *dev;
>  
>  	size_t size, avail;
>  
> @@ -74,9 +94,23 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq)
>  		  size, avail))
>  		gen_pool_destroy(binding->chunk_pool);
>  
> -	dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt,
> -					  binding->direction);
> -	dma_buf_detach(binding->dmabuf, binding->attachment);
> +	mutex_lock(&binding->lock);
> +	dev = binding->dev;
> +	if (dev)
> +		netdev_hold(dev, &dev_tracker, GFP_KERNEL);
> +	mutex_unlock(&binding->lock);
> +
> +	if (dev)
> +		netdev_lock(dev);
> +	mutex_lock(&binding->lock);
> +	net_devmem_dmabuf_binding_unmap(binding);
> +	mutex_unlock(&binding->lock);
> +	xa_erase(&net_devmem_dmabuf_bindings, binding->id);
> +	if (dev) {
> +		netdev_unlock(dev);
> +		netdev_put(dev, &dev_tracker);
> +	}
> +
>  	dma_buf_put(binding->dmabuf);
>  	xa_destroy(&binding->bound_rxqs);
>  	percpu_ref_exit(&binding->ref);
> @@ -128,11 +162,13 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
>  	struct netdev_rx_queue *rxq;
>  	unsigned long xa_idx;
>  	unsigned int rxq_idx;
> +	bool busy = false;
>  
> -	xa_erase(&net_devmem_dmabuf_bindings, binding->id);
> +	xa_clear_mark(&net_devmem_dmabuf_bindings, binding->id,
> +		      NET_DEVMEM_BINDING_LIVE);
>  
>  	/* Ensure no tx net_devmem_lookup_dmabuf() are in flight after the
> -	 * erase.
> +	 * mark is cleared.
>  	 */
>  	synchronize_net();
>  
> @@ -147,12 +183,62 @@ 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) {
> +		mutex_lock(&binding->lock);
> +		WRITE_ONCE(binding->dev, NULL);
> +		net_devmem_dmabuf_binding_unmap(binding);
> +		mutex_unlock(&binding->lock);
>  	}
>  
>  	percpu_ref_kill(&binding->ref);
>  }
>  

[..]

> +static bool
> +net_devmem_dev_unregister_rx(struct net_devmem_dmabuf_binding *binding,
> +			     unsigned long xa_idx, struct net_device *dev)
> +{
> +	return binding->direction == DMA_FROM_DEVICE &&
> +	       READ_ONCE(binding->dev) == dev &&
> +	       !xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +			    NET_DEVMEM_BINDING_LIVE);
> +}

super nit: if you happen to respin, I'd just open code this part in
net_devmem_dev_unregister, it reads funny to me idk, but maybe it's just me.
But even as is LGTM, thanks!

Acked-by: Stanislav Fomichev <sdf@fomichev.me>