Re: [PATCH v3 6/7] drm/xe/mmio_gem: fix destroy flow

[Matthew Auld <matthew.auld@intel.com>]

On 23/07/2026 17:18, Ilia Levi wrote:
> xe_mmio_gem_destroy() currently frees the GEM object directly, bypassing
> reference counting.  Since existing VMAs hold a reference and the fault
> handler accesses the object through vma->vm_private_data, this is
> use-after-free.  Additionally, nothing prevents the fault handler from
> installing PTEs to the real MMIO after destroy.
> 
> Fix this with proper synchronization and refcounting. Also, do not set
> vm_pgoff to zero. Many DRM drivers do this because helpers like
> dma_mmap_pages() interpret vm_pgoff as an intra-buffer page offset;
> leaving the DRM fake offset there would break these helpers.
> Those drivers can get away with zeroing it because they map eagerly -
> all PTEs are established before mmap returns, so vm_pgoff is never
> consulted again. Our driver does not use such helpers and the newly
> introduced call to drm_vma_node_unmap() relies on vm_pgoff being untouched.
> 
> v2: (Matt Auld)
> - use dma_resv lock to serialize fault handler with destroy
> - SIGBUS on access after destroy
> 
> Fixes: 1ffcf8b8ae8a ("drm/xe: Support for mmap-ing mmio regions")
> Assisted-by: GitHub-Copilot:claude-opus-4.6
> Signed-off-by: Ilia Levi <ilia.levi@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_mmio_gem.c | 42 ++++++++++++++++++++++++++------
>   1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_mmio_gem.c b/drivers/gpu/drm/xe/xe_mmio_gem.c
> index 5bd2759fc876..ef48642a0733 100644
> --- a/drivers/gpu/drm/xe/xe_mmio_gem.c
> +++ b/drivers/gpu/drm/xe/xe_mmio_gem.c
> @@ -38,6 +38,7 @@ struct xe_mmio_gem {
>   	struct drm_gem_object base;
>   	phys_addr_t phys_addr;
>   	struct page *dummy_page; /* protected by the GEM's dma_resv */
> +	bool destroyed; /* protected by the GEM's dma_resv */
>   };
>   
>   static int xe_mmio_gem_vm_may_split(struct vm_area_struct *area, unsigned long addr)
> @@ -150,8 +151,22 @@ static void xe_mmio_gem_free(struct drm_gem_object *base)
>    */
>   void xe_mmio_gem_destroy(struct xe_mmio_gem *gem, struct drm_file *file)
>   {
> -	drm_vma_node_revoke(&gem->base.vma_node, file);
> -	xe_mmio_gem_free(&gem->base);
> +	struct drm_gem_object *base = &gem->base;
> +	struct drm_device *dev = base->dev;
> +
> +	drm_vma_node_revoke(&base->vma_node, file);
> +
> +	dma_resv_lock(base->resv, NULL);
> +	gem->destroyed = true;
> +	dma_resv_unlock(base->resv);
> +	/*
> +	 * Setting 'destroyed' under lock takes care of the subsequent faults.
> +	 * Zap the existing PTEs to cut off access to the real MMIO through
> +	 * currently mapped pages.
> +	 */
> +	drm_vma_node_unmap(&base->vma_node, dev->anon_inode->i_mapping);
> +
> +	drm_gem_object_put(base);
>   }
>   
>   static int xe_mmio_gem_mmap(struct drm_gem_object *base, struct vm_area_struct *vma)
> @@ -162,8 +177,6 @@ static int xe_mmio_gem_mmap(struct drm_gem_object *base, struct vm_area_struct *
>   	if ((vma->vm_flags & VM_SHARED) == 0)
>   		return -EINVAL;
>   
> -	/* Set vm_pgoff (used as a fake buffer offset by DRM) to 0 */
> -	vma->vm_pgoff = 0;
>   	vma->vm_page_prot = pgprot_noncached(vm_get_page_prot(vma->vm_flags));
>   	vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP |
>   		     VM_DONTCOPY | VM_NORESERVE);
> @@ -176,10 +189,9 @@ static int alloc_dummy_page_if_needed(struct drm_gem_object *base)
>   {
>   	struct xe_mmio_gem *obj = to_xe_mmio_gem(base);
>   
> -	dma_resv_lock(base->resv, NULL);
> +	dma_resv_assert_held(base->resv);
>   	if (!obj->dummy_page)
>   		obj->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> -	dma_resv_unlock(base->resv);
>   
>   	return obj->dummy_page ? 0 : -ENOMEM;
>   }
> @@ -200,7 +212,7 @@ static vm_fault_t xe_mmio_gem_vm_fault_dummy_page(struct vm_fault *vmf)
>   				   vm_get_page_prot(vma->vm_flags));
>   }
>   
> -static vm_fault_t xe_mmio_gem_vm_fault(struct vm_fault *vmf)
> +static vm_fault_t xe_mmio_gem_vm_fault_locked(struct vm_fault *vmf)
>   {
>   	struct vm_area_struct *vma = vmf->vma;
>   	struct drm_gem_object *base = vma->vm_private_data;
> @@ -210,6 +222,10 @@ static vm_fault_t xe_mmio_gem_vm_fault(struct vm_fault *vmf)
>   	unsigned long addr, pfn;
>   	int idx;
>   
> +	dma_resv_assert_held(base->resv);
> +	if (obj->destroyed)
> +		return VM_FAULT_SIGBUS;
> +
>   	if (!drm_dev_enter(dev, &idx)) {
>   		/*
>   		 * Provide a dummy page to avoid SIGBUS for events such as hot-unplug.
> @@ -232,3 +248,15 @@ static vm_fault_t xe_mmio_gem_vm_fault(struct vm_fault *vmf)
>   	drm_dev_exit(idx);
>   	return ret;
>   }
> +
> +static vm_fault_t xe_mmio_gem_vm_fault(struct vm_fault *vmf)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct drm_gem_object *base = vma->vm_private_data;
> +	vm_fault_t ret;
> +
> +	dma_resv_lock(base->resv, NULL);

Not completely sure if locking order matters here with dma-resv -> 
drm_unplug_srcu, or if that should be inverted.

Also, wondering if this patch should go earlier in the series. The 
Sashiko report is hard to read since it complains about existing issues 
earlier in the series which are fixed later in the same series (this patch).

Reviewed-by: Matthew Auld <matthew.auld@intel.com>

> +	ret = xe_mmio_gem_vm_fault_locked(vmf);
> +	dma_resv_unlock(base->resv);
> +	return ret;
> +}