Re: [PATCH v2 5/5] drm/xe/mmio_gem: fix destroy flow

["Levi, Ilia" <ilia.levi@intel.com>]

On 21-Jul-26 12:15, Matthew Auld wrote:
> On 16/07/2026 16:22, Matthew Auld wrote:
>> On 26/05/2026 13:51, 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.
>>>
>>> Use SRCU to ensure the fault handler sees the 'destroyed' flag
>>> (mirroring the drm_dev_enter/exit pattern for hot-unplug), then zap
>>> existing PTEs to prevent continued access to the real MMIO. Use
>>> drm_gem_object_put() to respect the reference count.
>>
>> Couple questions here:
>>
>> 1) Can we not just use the dma-resv for synchronisation? We wrap the mmio_gem with a gem buffer, so the dma-resv is already there. This has the added benefit of looking more similar to the normal bo fault path. Also same question for dummy_page_lock in the previous patch.
>>
>> 2) Should we not just SIGBUG, if something faults on this post destroy? Is this not a userspace issue? Re-routing to the dummy page on unplug makes sense, since userspace did nothing wrong, so we want to give it a chance to recover.
>
> For 2) other option is maybe just to drop the gem->destroyed handling for now. For PCI_BARRIER and anything else tied to the xe_file, there shouldn't be any weird lifetime issues, so it should be impossible to see something "destroyed" in the fault handler. It's otherwise hard to judge without seeing a real user for this special "destroyed" flow with the re-routing to a dummy page.
>

The contract I envisioned for xe_mmio_gem_destroy() is immediate access cut-off,
so we're not dependent on userspace releasing it with munmap. For example,
suppose the exposed MMIO region is a per-exec-queue resource (i.e.
xe_mmio_gem_create called from xe_exec_queue_create and
xe_mmio_gem_destroy called from xe_exec_queue_destroy).
In such case, the same MMIO region could be recycled and reassigned to another
client - hence the need for the "destroyed" flag and zapping. You're probably
right about SIGBUS being the better choice in this case than a dummy page though.

Regarding dma_resv lock instead of srcu and dummy_page_lock - interesting idea,
let me check it out.

- Ilia

>>
>>>
>>> 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 | 27 ++++++++++++++++++++++++++-
>>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_mmio_gem.c b/drivers/gpu/drm/xe/ xe_mmio_gem.c
>>> index 2f2ebc4fd901..b91704c51a93 100644
>>> --- a/drivers/gpu/drm/xe/xe_mmio_gem.c
>>> +++ b/drivers/gpu/drm/xe/xe_mmio_gem.c
>>> @@ -5,11 +5,15 @@
>>>   #include "xe_mmio_gem.h"
>>> +#include <linux/srcu.h>
>>> +
>>>   #include <drm/drm_drv.h>
>>>   #include <drm/drm_gem.h>
>>>   #include "xe_device_types.h"
>>> +DEFINE_STATIC_SRCU(xe_mmio_gem_srcu);
>>> +
>>>   /**
>>>    * DOC: Exposing MMIO regions to userspace
>>>    *
>>> @@ -39,6 +43,7 @@ struct xe_mmio_gem {
>>>       unsigned long pgoff;
>>>       struct mutex dummy_page_lock; /* protects dummy page allocation */
>>>       struct page *dummy_page;
>>> +    bool destroyed;
>>>   };
>>>   static const struct vm_operations_struct vm_ops = {
>>> @@ -145,8 +150,23 @@ 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)
>>>   {
>>> +    struct drm_gem_object *base = &gem->base;
>>> +    struct drm_device *dev = base->dev;
>>> +
>>>       drm_vma_node_revoke(&gem->base.vma_node, file);
>>> -    xe_mmio_gem_free(&gem->base);
>>> +
>>> +    gem->destroyed = true;
>>> +    synchronize_srcu(&xe_mmio_gem_srcu);
>>> +
>>> +    /*
>>> +     * At this point every subsequent fault handler will see that the
>>> +     * object has been destroyed and provide the dummy page.
>>> +     * Now just zap existing PTEs to prevent continued access to the real
>>> +     * MMIO.
>>> +     */
>>> +    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)
>>> @@ -196,6 +216,11 @@ static vm_fault_t xe_mmio_gem_vm_fault(struct vm_fault *vmf)
>>>       unsigned long pgoff;
>>>       int idx;
>>> +    guard(srcu)(&xe_mmio_gem_srcu);
>>> +
>>> +    if (obj->destroyed)
>>> +        return xe_mmio_gem_vm_fault_dummy_page(vmf);
>>> +
>>>       if (!drm_dev_enter(dev, &idx)) {
>>>           /*
>>>            * Provide a dummy page to avoid SIGBUS for events such as hot-unplug.
>>
>