Re: [PATCH 1/5] drm/xe: Hold a device reference across deferred VM destruction

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Sep 18, 2026 at 09:22:53AM +0200, Thomas Hellström wrote:
> On Thu, 2026-09-17 at 20:22 -0700, Matthew Brost wrote:
> > On Wed, Sep 16, 2026 at 03:23:33PM +0530, Arvind Yadav wrote:
> > > xe_vm_free() is the drm_gpuvm vm_free callback. It hands the final
> > > teardown to vm_destroy_work_func() on a workqueue and returns.
> > > 
> > > drm_gpuvm_free() drops its device reference immediately after the
> > > callback returns:
> > > 
> > > 	gpuvm->ops->vm_free(gpuvm);
> > > 	drm_dev_put(drm);
> > > 
> > > vm_destroy_work_func() then keeps using device state:
> > > xe_pm_runtime_put()
> > > for an LR mode VM, ttm_lru_bulk_move_fini() on xe->ttm, and the
> > > tile
> > > iteration. If the freed VM held the last device reference, the work
> > > runs
> > > against a released xe_device.
> > > 
> > > Take a device reference in xe_vm_free() and drop it once
> > > vm_destroy_work_func() has finished using the device.
> > > 
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > 
> > This is a fix, IMO. Ideally, we should probably push the delayed-
> > destroy
> > semantics into gpuvm if they are really needed. I'm also questioning
> > whether the VM destroy worker is actually required. This dates back
> > to
> > the very early days of Xe, and I doubt we've ever revisited whether
> > it
> > is necessary.
> > 
> > Let's follow up with one of the following:
> > - Introduce async destroy in gpuvm and have it own the
> > drm_dev_get/put.
> > - Drop delayed destroy entirely in Xe.
> 
> We need to keep in mind that the drm file keeps a reference on the Xe
> module. So once the last close() callback has executed, the module can

Yikes, so drm_dev_get won't prevent the module from unloading? Different
issue, right?

> typically be unloaded, causing execution UAF. It's therefore not really
> recommended to keep file-related structures around with a refcount
> after close.
> 
> Device references however typically don't necessarily keep the module
> pinned. I had a series to fix this for xe only, (Got stalled) [1], but
> in general we should be careful about leaking that assumption into DRM
> code.
> 

So what is the fix here? I believe gpuvm has a drm_dev_ ref and it async
teardown which breaks drm_dev_ assumption. I don't see an issue with
this patch or my suggest follow up.

> [1] https://patchwork.freedesktop.org/series/163298/

Should we push on this? I don't really see an issue with series as long
as 'rmmod xe' works if display isn't holding a module ref.

Matt

> 
> Thanks,
> Thomas
> 
> 
> > 
> > As a temporary fix that can be backported, this looks good to me, so
> > with a Fixes tag:
> > 
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > 
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Assisted-by: Claude:claude-opus-4-8
> > > Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_vm.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > > b/drivers/gpu/drm/xe/xe_vm.c
> > > index efa5ff6cc823..264bdab75de2 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -2054,12 +2054,21 @@ static void vm_destroy_work_func(struct
> > > work_struct *w)
> > >  		xe_file_put(vm->xef);
> > >  
> > >  	kfree(vm);
> > > +
> > > +	drm_dev_put(&xe->drm);
> > >  }
> > >  
> > >  static void xe_vm_free(struct drm_gpuvm *gpuvm)
> > >  {
> > >  	struct xe_vm *vm = container_of(gpuvm, struct xe_vm,
> > > gpuvm);
> > >  
> > > +	/*
> > > +	 * drm_gpuvm drops its device reference as soon as this
> > > callback
> > > +	 * returns, but vm_destroy_work_func() still uses device
> > > state. Hold a
> > > +	 * reference across the deferred work.
> > > +	 */
> > > +	drm_dev_get(&vm->xe->drm);
> > > +
> > >  	/* To destroy the VM we need to be able to sleep */
> > >  	queue_work(system_dfl_wq, &vm->destroy_work);
> > >  }
> > > -- 
> > > 2.43.0
> > >