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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Fri, 2026-09-18 at 15:41 -0700, Matthew Brost wrote:
> 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?

Exactly.

> Different
> issue, right?

Yes, but if we move into GPUVM it would affect other drivers. I believe
the issue is fixable on a per-driver basis.

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

While the fix for xe is the below series, And I will respin that, If we
move more device refcounts into GPUVM, that will affect other drivers
that aren't yet fixed. Note drm_pagemap handles this with the dev
unhold work, not sure if that is applicable also with GPUVM. 

Either way, I don't want to block the series due to a fix missing. Just
want us to keep this in mind since it increases the probability of the
real bug hitting.

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

Yes, I'll respin.
Thanks,
Thomas


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