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

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

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.

> 
>>
>> 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.
>