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 <[email protected]> > > > > 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 <[email protected]> > > > > > Cc: Thomas Hellström <[email protected]> > > > Cc: Himal Prasad Ghimiray <[email protected]> > > > Cc: Rodrigo Vivi <[email protected]> > > > Assisted-by: Claude:claude-opus-4-8 > > > Signed-off-by: Arvind Yadav <[email protected]> > > > --- > > > 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 > > >
