[PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

[PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

[Thomas Hellström]

Driver and shared DRM helper code is increasingly relying on bare
drm_device references (drm_dev_get()/drm_dev_put()) to keep a device's
software state around, without also pairing that with a reference on
the owning kernel module. Xe itself does this in several places, and
so does drm_gpuvm for the lifetime of a GPU VM. None of these
references currently prevent the owning module from being unloaded
while they, or the teardown work they can still trigger, are
outstanding, meaning driver code can end up executing after its own
module's text has already been freed.

This series closes that gap for xe:

- Patch 1 adds core DRM infrastructure allowing a driver to wait for
  its outstanding device-release callbacks to finish before
  proceeding with module unload.

- Patch 2 makes xe use this infrastructure to hold up module unload
  until every xe_device instance has actually been released, rather
  than only until the module's own refcount happens to reach zero,
  with a diagnostic if this ends up taking an unexpectedly long time.

- Patch 3 fixes a related, previously unprotected case where the
  teardown of a GPU VM or its address space mappings can be deferred
  to run at an arbitrary later time, including after module unload has
  already completed.

Together, these changes ensure `rmmod xe` cannot free the module's
memory while any of its devices, or asynchronous work stemming from
them, might still be executing.

v2:
- Use plain WARN_ON_ONCE() instead of drm_WARN_ON_ONCE(NULL, ...) in
  drm_dev_release_barrier(), fixing a NULL pointer dereference on the
  warning path itself (patch 1, sashiko)
- Updated the commit message of patch 2 to describe the actual
  implementation (a single 20s wait_var_event_timeout() followed by
  one pr_warn() and an unbounded wait_var_event(), rather than a loop
  retrying with a diagnostic every 10s) and its uninterruptible-sleep
  tradeoff (sashiko)

v3:
- drm_dev_release_barrier() now uses a single, global SRCU domain
  shared by all drivers instead of requiring each driver to register
  its own via a new &drm_driver.release_srcu field, accepting that a
  driver's call may then occasionally block on unrelated drivers'
  release callbacks (patch 1, patch 2, Christian König)

Thomas Hellström (3):
  drm: Provide a drm_dev_release_barrier() function to wait for device
    release callbacks
  drm/xe: Don't unload the driver until all drm devices are freed
  drm/xe: Route deferred xe_vma/xe_vm teardown off system_dfl_wq

 drivers/gpu/drm/drm_drv.c      | 61 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_device.c | 32 ++++++++++++++++++
 drivers/gpu/drm/xe/xe_device.h |  2 ++
 drivers/gpu/drm/xe/xe_module.c | 24 +++++++++++--
 drivers/gpu/drm/xe/xe_vm.c     |  5 +--
 include/drm/drm_drv.h          |  1 +
 6 files changed, 120 insertions(+), 5 deletions(-)

-- 
2.55.0

[PATCH v3 1/3] drm: Provide a drm_dev_release_barrier() function to wait for device release callbacks

[Thomas Hellström]

Driver and drm helper code is increasingly relying on holding a bare
&drm_device reference (drm_dev_get()) without also holding a module
reference on the module that created the device.

For example drm_gpuvm_init() takes a drm_dev_get() reference on the
&drm_gpuvm's behalf with no accompanying module reference at all, and
drm_gpuvm_free() later calls the driver-supplied gpuvm->ops->vm_free()
callback, which lives in the driver module, before dropping that
reference. xe also takes bare drm_device references itself from
several asynchronous contexts, such as GuC submission fence workers,
EU stall, OA and PMU sampling code, relying only on those references
being dropped before the underlying xe_device, and eventually the
driver module, can be torn down.

If the module that created such a device is unloaded while one of
these bare references is still outstanding, and the corresponding
drm_dev_put() only completes after the module has already been
removed, the driver's ->release() callback, any drm managed release
actions, or a driver callback such as gpuvm->ops->vm_free(), all of
which live in that module's, by then freed, code, can end up being
invoked out of memory that no longer contains valid code.

Requiring every one of these bare drm_device references to also take a
module reference, as drm_pagemap does today via try_module_get(),
doesn't scale to shared helpers and driver-internal code with many
call sites, and is easy to get wrong.

Fix this properly by letting drivers keep a drm device-count and
ensure the module isn't unloaded until that count has dropped to zero
and until any release callback that had already started executing has
also finished executing.

To help with the latter, add a drm_dev_release_barrier() function.
The function ensures that any caller that has started executing
device release callbacks has also finished executing them.

Use SRCU for the implementation, with a single, global SRCU domain
shared by all drivers. This means a driver's call to
drm_dev_release_barrier() may occasionally end up waiting for an
unrelated driver's release callback to finish, but release callbacks
are expected to run quickly, and sharing one domain avoids the
bookkeeping that a per-driver SRCU domain would require, such as
adding a new &drm_driver field that every existing struct drm_driver
instance in the tree would need to be audited for, and initializing
and cleaning up that domain around driver (un)registration.

v2:
- Use plain WARN_ON_ONCE() instead of drm_WARN_ON_ONCE(NULL, ...) in
  drm_dev_release_barrier(), since passing a NULL drm_device caused
  the warning path itself to dereference that NULL pointer inside
  dev_driver_string()/dev_name() (sashiko)

v3:
- Use a single, global SRCU domain shared by all drivers instead of
  requiring each driver to supply its own via a new
  &drm_driver.release_srcu field, accepting that a driver's call to
  drm_dev_release_barrier() may then occasionally block on unrelated
  drivers' release callbacks (Christian König)

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Assisted-by: LLM
---
 drivers/gpu/drm/drm_drv.c | 61 +++++++++++++++++++++++++++++++++++++++
 include/drm/drm_drv.h     |  1 +
 2 files changed, 62 insertions(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 0cdc606af8d1..0641e61ee931 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -918,21 +918,65 @@ struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
 }
 EXPORT_SYMBOL(drm_dev_alloc);
 
+/*
+ * Single, global SRCU domain used to synchronize completion of every
+ * driver's @release callback and drm managed release actions with
+ * drm_dev_release_barrier(). Sharing one domain across all drivers means a
+ * driver's call to drm_dev_release_barrier() may occasionally have to wait
+ * for unrelated drivers' release callbacks to finish, but that's a
+ * reasonable trade-off given that release callbacks are expected to run
+ * quickly, and it avoids the bookkeeping of a per-driver SRCU domain.
+ */
+DEFINE_STATIC_SRCU(drm_release_srcu);
+
 static void drm_dev_release(struct kref *ref)
 {
 	struct drm_device *dev = container_of(ref, struct drm_device, ref);
+	int idx;
 
 	/* Just in case register/unregister was never called */
 	drm_debugfs_dev_fini(dev);
 
+	idx = srcu_read_lock(&drm_release_srcu);
+
 	if (dev->driver->release)
 		dev->driver->release(dev);
 
 	drm_managed_release(dev);
 
+	srcu_read_unlock(&drm_release_srcu, idx);
+
 	kfree(dev->managed.final_kfree);
 }
 
+/**
+ * drm_dev_release_barrier() - Ensure drm device release callbacks are finished
+ *
+ * If a device release method or any of the drm managed release callbacks
+ * have been called for any drm_device, wait until all of them have finished
+ * executing. This function can be used to help determine whether it's safe
+ * to unload a driver module.
+ *
+ * Assume for example a driver maintains a device count which is decremented
+ * using a drmm callback or a device release callback. From a drm device
+ * lifetime POV, it's then safe to unload the driver when that device-count
+ * has reached zero and drm_dev_release_barrier() has been called.
+ *
+ * Since a single, global SRCU domain is used for all drivers, this function
+ * may also end up waiting for unrelated drivers' release callbacks to
+ * complete.
+ *
+ * This function only waits for the &drm_driver.release callback and drm
+ * managed release actions to finish. It does not, by itself, guarantee that
+ * whoever called drm_dev_put() to drop the reference triggering that release
+ * has itself finished running. See drm_dev_put() for that invariant.
+ */
+void drm_dev_release_barrier(void)
+{
+	synchronize_srcu(&drm_release_srcu);
+}
+EXPORT_SYMBOL(drm_dev_release_barrier);
+
 /**
  * drm_dev_get - Take reference of a DRM device
  * @dev: device to take reference of or NULL
@@ -958,6 +1002,23 @@ EXPORT_SYMBOL(drm_dev_get);
  *
  * This decreases the ref-count of @dev by one. The device is destroyed if the
  * ref-count drops to zero.
+ *
+ * If this call may drop the last reference, the calling code itself is
+ * responsible for ensuring it isn't unloaded (for example as part of a
+ * module) before this call has returned. This matters in particular for
+ * drivers relying on drm_dev_release_barrier() to determine when it's safe to
+ * unload, since that function only waits for the &drm_driver.release
+ * callback and drm managed release actions to finish, not for whoever calls
+ * drm_dev_put() to finish calling it.
+ *
+ * A common case is dropping the last reference from a deferred context, such
+ * as a workqueue item. In that case it's the responsibility of whoever
+ * queued that work item to guarantee it has run to completion before the
+ * module can unload, for example by draining a module-lifetime workqueue at
+ * module exit time. Holding a module reference only until the work item
+ * starts running is insufficient: that reference would already be dropped
+ * before this call runs, even though this call is what may still need the
+ * module's code to remain resident.
  */
 void drm_dev_put(struct drm_device *dev)
 {
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index b23830494ed4..169b404df54a 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -485,6 +485,7 @@ void drm_dev_exit(int idx);
 void drm_dev_unplug(struct drm_device *dev);
 int drm_dev_wedged_event(struct drm_device *dev, unsigned long method,
 			 struct drm_wedge_task_info *info);
+void drm_dev_release_barrier(void);
 
 /**
  * drm_dev_is_unplugged - is a DRM device unplugged
-- 
2.55.0

[PATCH v3 2/3] drm/xe: Don't unload the driver until all drm devices are freed

[Thomas Hellström]

xe, and shared helpers it uses such as drm_gpuvm, already take bare
drm_device references (drm_dev_get()) from several contexts, for
example GuC submission fence workers, EU stall, OA and PMU sampling
code, and drm_gpuvm's own vm object lifetime, without pairing them
with a module reference. Since xe_exit() is only invoked after the
module's own refcount has dropped to zero, none of these references
currently prevent `rmmod xe` from proceeding while they, or the
underlying xe_device release path they can trigger, are still
outstanding, i.e. driver code belonging to a module whose text is
being freed could still end up executing.

Close this gap by keeping a device-count and waiting for it to reach
zero at module unload, then waiting for any release callback that has
started executing to finish, using the drm_dev_release_barrier()
infrastructure introduced in the previous commit.

The wait for the device-count to reach zero at module unload is
unbounded and non-interruptible. Rather than blocking silently forever
if a reference is ever leaked, use wait_var_event_timeout() with a 20s
timeout, well above the typical maximum dma_fence signalling time, and
warn once if devices still remain by then, before falling back to an
unbounded wait_var_event() so a stuck rmmod is at least observable
instead of an indefinite, silent hang.

Note that if a reference genuinely leaks, this still ends up as an
indefinite uninterruptible sleep, which may eventually trip the
kernel's hung-task watchdog. The alternative would be for these bare
drm_device references to also take a module reference, which would
instead make the module unable to be unloaded unless all its devices are
manually unbound first. The wait-based approach is chosen here since it
keeps rmmod usable in the common case.

drm_dev_release_barrier() uses a single, global SRCU domain shared by
all drivers, so xe's wait for its own release callbacks may
occasionally end up blocking on an unrelated driver's release path
too, a trade-off accepted in favor of not requiring each driver to set
up and tear down its own SRCU domain.

xe_device_exit() is added as a new module exit hook. Its entry in the
init_funcs[] table is placed between xe_destroy_wq_module_init and
xe_register_pci_driver, so that (exit functions run in reverse array
order) it executes after xe_unregister_pci_driver() has forced all
devices to unbind, but before xe_destroy_wq_module_exit() tears down
the module-lifetime xe_destroy_wq. This preserves xe_destroy_wq's
existing teardown ordering relative to xe_sched_job_module_exit() and
xe_hw_fence_module_exit(), which destroy kmem_caches that work drained
from xe_destroy_wq relies on, while ensuring xe_destroy_wq itself is
only torn down once xe_device_exit() has confirmed no more work can be
queued onto it.

v2:
- Updated commit message to describe the actual implementation (a
  single 20s wait_var_event_timeout() followed by one pr_warn() and an
  unbounded wait_var_event(), rather than a loop retrying with a
  diagnostic every 10s) and its uninterruptible-sleep tradeoff
  (sashiko)

v3:
- drm_dev_release_barrier() no longer takes a &drm_driver argument
  since it now uses a single, global SRCU domain; drop the
  &drm_driver.release_srcu registration on both xe drm_driver
  instances accordingly (Christian König)

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Assisted-by: LLM
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_device.c | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_device.h |  2 ++
 drivers/gpu/drm/xe/xe_module.c | 18 +++++++++++++++++-
 3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 205cb4e7f9e8..c54d734bd890 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -372,6 +372,9 @@ bool xe_device_is_admin_only(const struct xe_device *xe)
 }
 #endif
 
+/* Number of allocated struct xe_device */
+static atomic_t xe_device_count;
+
 static void xe_device_destroy(struct drm_device *dev, void *dummy)
 {
 	struct xe_device *xe = to_xe_device(dev);
@@ -391,6 +394,9 @@ static void xe_device_destroy(struct drm_device *dev, void *dummy)
 		destroy_workqueue(xe->destroy_wq);
 
 	ttm_device_fini(&xe->ttm);
+
+	if (atomic_dec_and_test(&xe_device_count))
+		wake_up_var(&xe_device_count);
 }
 
 /**
@@ -461,6 +467,7 @@ int xe_device_init_early(struct xe_device *xe)
 		return err;
 
 	xe_bo_dev_init(&xe->bo_device);
+	atomic_inc(&xe_device_count);
 	err = drmm_add_action_or_reset(&xe->drm, xe_device_destroy, NULL);
 	if (err)
 		return err;
@@ -1501,3 +1508,28 @@ struct xe_vm *xe_device_asid_to_vm(struct xe_device *xe, u32 asid)
 
 	return vm;
 }
+
+/**
+ * xe_device_exit() - Device subsystem exit function.
+ *
+ * Exit function to be called at module unload time.
+ */
+void xe_device_exit(void)
+{
+	/*
+	 * Wait for all devices to be freed. 20s is well above the typical
+	 * maximum dma_fence signalling time, so warn and keep waiting if
+	 * we're still not done by then, since it may indicate a leaked
+	 * xe_device reference is stalling module unload.
+	 */
+	if (!wait_var_event_timeout(&xe_device_count,
+				    !atomic_read(&xe_device_count),
+				    HZ * 20)) {
+		pr_warn("%s: Waiting for %d xe device(s) to be freed before unloading.\n",
+			DRIVER_NAME, atomic_read(&xe_device_count));
+		wait_var_event(&xe_device_count, !atomic_read(&xe_device_count));
+	}
+
+	/* Wait for any driver release callbacks to complete */
+	drm_dev_release_barrier();
+}
diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
index 6d3d6d5eba29..83d6dafab53c 100644
--- a/drivers/gpu/drm/xe/xe_device.h
+++ b/drivers/gpu/drm/xe/xe_device.h
@@ -283,6 +283,8 @@ static inline bool xe_device_is_admin_only(const struct xe_device *xe)
 }
 #endif
 
+void xe_device_exit(void);
+
 /*
  * Occasionally it is seen that the G2H worker starts running after a delay of more than
  * a second even after being queued and activated by the Linux workqueue subsystem. This
diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
index 4bc28dfc1992..c61bd33546f2 100644
--- a/drivers/gpu/drm/xe/xe_module.c
+++ b/drivers/gpu/drm/xe/xe_module.c
@@ -12,7 +12,7 @@
 #include <drm/drm_module.h>
 
 #include "xe_defaults.h"
-#include "xe_device_types.h"
+#include "xe_device.h"
 #include "xe_drv.h"
 #include "xe_configfs.h"
 #include "xe_hw_fence.h"
@@ -162,6 +162,22 @@ static const struct init_funcs init_funcs[] = {
 		.init = xe_destroy_wq_module_init,
 		.exit = xe_destroy_wq_module_exit,
 	},
+	/*
+	 * xe_destroy_wq_module_exit() must run after xe_device_exit()
+	 * (below), since freeing a device can still queue work on
+	 * xe_destroy_wq that must be drained before the module can safely
+	 * unload. At the same time, xe_device_exit() must run after
+	 * xe_unregister_pci_driver() (below), and xe_destroy_wq_module_exit()
+	 * must run before xe_sched_job_module_exit() and
+	 * xe_hw_fence_module_exit() (above), whose kmem_caches are still used
+	 * by work drained from xe_destroy_wq. Exit functions run in reverse
+	 * array order, so this entry must sit between the
+	 * xe_destroy_wq_module_init entry above and the xe_register_pci_driver
+	 * entry below.
+	 */
+	{
+		.exit = xe_device_exit,
+	},
 	{
 		.init = xe_register_pci_driver,
 		.exit = xe_unregister_pci_driver,
-- 
2.55.0

[PATCH v3 3/3] drm/xe: Route deferred xe_vma/xe_vm teardown off system_dfl_wq

[Thomas Hellström]

xe_vma_destroy() can defer the final teardown of a struct xe_vma to a
dma_fence completion callback (vma_destroy_cb()), and xe_vm_free() (the
drm_gpuvm_ops.vm_free callback) always defers struct xe_vm teardown to
a work item, since destroying a VM needs to sleep. Both used to queue
their work on system_dfl_wq, a global, kernel-wide workqueue that xe
has no control over and never waits on during module unload.

drm_gpuvm_free() drops its drm_device reference immediately after
calling xe_vm_free(), without waiting for the deferred work to run.
The same applies one level down: whichever xe_vma or xe_vm reference
happens to be the last one can trigger this chain from a dma_fence
callback that may fire at an arbitrary time, including after the
owning file has already been closed and its own module reference
dropped. Since nothing tracks or waits for work queued on
system_dfl_wq, `rmmod xe` could succeed and free the module's text
while vma_destroy_work_func() or vm_destroy_work_func() is still
queued or running on it, jumping into freed code.

Fix this by queueing this work on xe_destroy_wq instead, the existing
module-lifetime workqueue already used for GuC exec queue teardown.
Unlike a per-device workqueue, this requires no dereference of a
struct xe_device that may already be gone by the time a deferred
callback fires, and unlike system_dfl_wq it is guaranteed to be
drained by xe_destroy_wq_module_exit() before the module is unloaded,
following the drm_pagemap_dev_hold()/unhold_work precedent of using a
workqueue that is waited on at module unload rather than a bare module
reference. The previous commit's reordering of xe_destroy_wq_exit()
to run after xe_device_exit() guarantees that xe_destroy_wq is only
torn down once the device-count has reached zero, i.e. after any
xe_vma or xe_vm whose teardown queues work here has already dropped
its drm_device reference and thus already queued that work.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Assisted-by: LLM
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_module.c | 6 ++++--
 drivers/gpu/drm/xe/xe_vm.c     | 5 +++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
index c61bd33546f2..897724cb5cfb 100644
--- a/drivers/gpu/drm/xe/xe_module.c
+++ b/drivers/gpu/drm/xe/xe_module.c
@@ -114,8 +114,10 @@ static void xe_destroy_wq_module_exit(void)
  * xe_destroy_wq_queue() - Queue work on the destroy workqueue
  * @work: work item to queue
  *
- * The destroy workqueue has module lifetime and is used for GuC exec queue
- * teardown that can outlive a single xe_device. SVM pagemap destroy uses the
+ * The destroy workqueue has module lifetime, and is guaranteed to outlive
+ * any xe_device, and to be drained before the module is unloaded. It is used
+ * for GuC exec queue and xe_vm/xe_vma teardown that can be deferred past the
+ * lifetime of the xe_device that triggered it. SVM pagemap destroy uses the
  * per-device xe->destroy_wq instead.
  *
  * Return: %true if @work was queued, %false if it was already pending.
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 390da884c727..ee369e6c3b28 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -29,6 +29,7 @@
 #include "xe_exec_queue.h"
 #include "xe_gt.h"
 #include "xe_migrate.h"
+#include "xe_module.h"
 #include "xe_pagefault.h"
 #include "xe_pat.h"
 #include "xe_pm.h"
@@ -1249,7 +1250,7 @@ static void vma_destroy_cb(struct dma_fence *fence,
 	struct xe_vma *vma = container_of(cb, struct xe_vma, destroy_cb);
 
 	INIT_WORK(&vma->destroy_work, vma_destroy_work_func);
-	queue_work(system_dfl_wq, &vma->destroy_work);
+	xe_destroy_wq_queue(&vma->destroy_work);
 }
 
 static void xe_vm_assert_write_mode_or_garbage_collector(struct xe_vm *vm)
@@ -2059,7 +2060,7 @@ static void xe_vm_free(struct drm_gpuvm *gpuvm)
 	struct xe_vm *vm = container_of(gpuvm, struct xe_vm, gpuvm);
 
 	/* To destroy the VM we need to be able to sleep */
-	queue_work(system_dfl_wq, &vm->destroy_work);
+	xe_destroy_wq_queue(&vm->destroy_work);
 }
 
 struct xe_vm *xe_vm_lookup(struct xe_file *xef, u32 id)
-- 
2.55.0

Re: [PATCH v3 3/3] drm/xe: Route deferred xe_vma/xe_vm teardown off system_dfl_wq

[Matthew Brost]

On Fri, Sep 25, 2026 at 03:33:35PM +0200, Thomas Hellström wrote:
> xe_vma_destroy() can defer the final teardown of a struct xe_vma to a
> dma_fence completion callback (vma_destroy_cb()), and xe_vm_free() (the
> drm_gpuvm_ops.vm_free callback) always defers struct xe_vm teardown to
> a work item, since destroying a VM needs to sleep. Both used to queue
> their work on system_dfl_wq, a global, kernel-wide workqueue that xe
> has no control over and never waits on during module unload.
> 
> drm_gpuvm_free() drops its drm_device reference immediately after
> calling xe_vm_free(), without waiting for the deferred work to run.
> The same applies one level down: whichever xe_vma or xe_vm reference
> happens to be the last one can trigger this chain from a dma_fence
> callback that may fire at an arbitrary time, including after the
> owning file has already been closed and its own module reference
> dropped. Since nothing tracks or waits for work queued on
> system_dfl_wq, `rmmod xe` could succeed and free the module's text
> while vma_destroy_work_func() or vm_destroy_work_func() is still
> queued or running on it, jumping into freed code.
> 
> Fix this by queueing this work on xe_destroy_wq instead, the existing
> module-lifetime workqueue already used for GuC exec queue teardown.
> Unlike a per-device workqueue, this requires no dereference of a
> struct xe_device that may already be gone by the time a deferred
> callback fires, and unlike system_dfl_wq it is guaranteed to be
> drained by xe_destroy_wq_module_exit() before the module is unloaded,
> following the drm_pagemap_dev_hold()/unhold_work precedent of using a
> workqueue that is waited on at module unload rather than a bare module
> reference. The previous commit's reordering of xe_destroy_wq_exit()
> to run after xe_device_exit() guarantees that xe_destroy_wq is only
> torn down once the device-count has reached zero, i.e. after any
> xe_vma or xe_vm whose teardown queues work here has already dropped
> its drm_device reference and thus already queued that work.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Assisted-by: LLM
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_module.c | 6 ++++--
>  drivers/gpu/drm/xe/xe_vm.c     | 5 +++--
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
> index c61bd33546f2..897724cb5cfb 100644
> --- a/drivers/gpu/drm/xe/xe_module.c
> +++ b/drivers/gpu/drm/xe/xe_module.c
> @@ -114,8 +114,10 @@ static void xe_destroy_wq_module_exit(void)
>   * xe_destroy_wq_queue() - Queue work on the destroy workqueue
>   * @work: work item to queue
>   *
> - * The destroy workqueue has module lifetime and is used for GuC exec queue
> - * teardown that can outlive a single xe_device. SVM pagemap destroy uses the
> + * The destroy workqueue has module lifetime, and is guaranteed to outlive
> + * any xe_device, and to be drained before the module is unloaded. It is used
> + * for GuC exec queue and xe_vm/xe_vma teardown that can be deferred past the
> + * lifetime of the xe_device that triggered it. SVM pagemap destroy uses the
>   * per-device xe->destroy_wq instead.
>   *
>   * Return: %true if @work was queued, %false if it was already pending.
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 390da884c727..ee369e6c3b28 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -29,6 +29,7 @@
>  #include "xe_exec_queue.h"
>  #include "xe_gt.h"
>  #include "xe_migrate.h"
> +#include "xe_module.h"
>  #include "xe_pagefault.h"
>  #include "xe_pat.h"
>  #include "xe_pm.h"
> @@ -1249,7 +1250,7 @@ static void vma_destroy_cb(struct dma_fence *fence,
>  	struct xe_vma *vma = container_of(cb, struct xe_vma, destroy_cb);
>  
>  	INIT_WORK(&vma->destroy_work, vma_destroy_work_func);
> -	queue_work(system_dfl_wq, &vma->destroy_work);
> +	xe_destroy_wq_queue(&vma->destroy_work);
>  }
>  
>  static void xe_vm_assert_write_mode_or_garbage_collector(struct xe_vm *vm)
> @@ -2059,7 +2060,7 @@ static void xe_vm_free(struct drm_gpuvm *gpuvm)
>  	struct xe_vm *vm = container_of(gpuvm, struct xe_vm, gpuvm);
>  
>  	/* To destroy the VM we need to be able to sleep */
> -	queue_work(system_dfl_wq, &vm->destroy_work);
> +	xe_destroy_wq_queue(&vm->destroy_work);

Actually this is still unsafe, right?

destroy_work touches vm->xe which could be gone after gpuvm drops
potentially the final drm_dev_put, right?

Matt 

>  }
>  
>  struct xe_vm *xe_vm_lookup(struct xe_file *xef, u32 id)
> -- 
> 2.55.0
> 

Re: [PATCH v3 3/3] drm/xe: Route deferred xe_vma/xe_vm teardown off system_dfl_wq

[Matthew Brost]

On Fri, Sep 25, 2026 at 12:56:43PM -0700, Matthew Brost wrote:
> On Fri, Sep 25, 2026 at 03:33:35PM +0200, Thomas Hellström wrote:
> > xe_vma_destroy() can defer the final teardown of a struct xe_vma to a
> > dma_fence completion callback (vma_destroy_cb()), and xe_vm_free() (the
> > drm_gpuvm_ops.vm_free callback) always defers struct xe_vm teardown to
> > a work item, since destroying a VM needs to sleep. Both used to queue
> > their work on system_dfl_wq, a global, kernel-wide workqueue that xe
> > has no control over and never waits on during module unload.
> > 
> > drm_gpuvm_free() drops its drm_device reference immediately after
> > calling xe_vm_free(), without waiting for the deferred work to run.
> > The same applies one level down: whichever xe_vma or xe_vm reference
> > happens to be the last one can trigger this chain from a dma_fence
> > callback that may fire at an arbitrary time, including after the
> > owning file has already been closed and its own module reference
> > dropped. Since nothing tracks or waits for work queued on
> > system_dfl_wq, `rmmod xe` could succeed and free the module's text
> > while vma_destroy_work_func() or vm_destroy_work_func() is still
> > queued or running on it, jumping into freed code.
> > 
> > Fix this by queueing this work on xe_destroy_wq instead, the existing
> > module-lifetime workqueue already used for GuC exec queue teardown.
> > Unlike a per-device workqueue, this requires no dereference of a
> > struct xe_device that may already be gone by the time a deferred
> > callback fires, and unlike system_dfl_wq it is guaranteed to be
> > drained by xe_destroy_wq_module_exit() before the module is unloaded,
> > following the drm_pagemap_dev_hold()/unhold_work precedent of using a
> > workqueue that is waited on at module unload rather than a bare module
> > reference. The previous commit's reordering of xe_destroy_wq_exit()
> > to run after xe_device_exit() guarantees that xe_destroy_wq is only
> > torn down once the device-count has reached zero, i.e. after any
> > xe_vma or xe_vm whose teardown queues work here has already dropped
> > its drm_device reference and thus already queued that work.
> > 
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Assisted-by: LLM
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_module.c | 6 ++++--
> >  drivers/gpu/drm/xe/xe_vm.c     | 5 +++--
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
> > index c61bd33546f2..897724cb5cfb 100644
> > --- a/drivers/gpu/drm/xe/xe_module.c
> > +++ b/drivers/gpu/drm/xe/xe_module.c
> > @@ -114,8 +114,10 @@ static void xe_destroy_wq_module_exit(void)
> >   * xe_destroy_wq_queue() - Queue work on the destroy workqueue
> >   * @work: work item to queue
> >   *
> > - * The destroy workqueue has module lifetime and is used for GuC exec queue
> > - * teardown that can outlive a single xe_device. SVM pagemap destroy uses the
> > + * The destroy workqueue has module lifetime, and is guaranteed to outlive
> > + * any xe_device, and to be drained before the module is unloaded. It is used
> > + * for GuC exec queue and xe_vm/xe_vma teardown that can be deferred past the
> > + * lifetime of the xe_device that triggered it. SVM pagemap destroy uses the
> >   * per-device xe->destroy_wq instead.
> >   *
> >   * Return: %true if @work was queued, %false if it was already pending.
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 390da884c727..ee369e6c3b28 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -29,6 +29,7 @@
> >  #include "xe_exec_queue.h"
> >  #include "xe_gt.h"
> >  #include "xe_migrate.h"
> > +#include "xe_module.h"
> >  #include "xe_pagefault.h"
> >  #include "xe_pat.h"
> >  #include "xe_pm.h"
> > @@ -1249,7 +1250,7 @@ static void vma_destroy_cb(struct dma_fence *fence,
> >  	struct xe_vma *vma = container_of(cb, struct xe_vma, destroy_cb);
> >  
> >  	INIT_WORK(&vma->destroy_work, vma_destroy_work_func);
> > -	queue_work(system_dfl_wq, &vma->destroy_work);
> > +	xe_destroy_wq_queue(&vma->destroy_work);
> >  }
> >  
> >  static void xe_vm_assert_write_mode_or_garbage_collector(struct xe_vm *vm)
> > @@ -2059,7 +2060,7 @@ static void xe_vm_free(struct drm_gpuvm *gpuvm)
> >  	struct xe_vm *vm = container_of(gpuvm, struct xe_vm, gpuvm);
> >  
> >  	/* To destroy the VM we need to be able to sleep */
> > -	queue_work(system_dfl_wq, &vm->destroy_work);
> > +	xe_destroy_wq_queue(&vm->destroy_work);
> 
> Actually this is still unsafe, right?
> 
> destroy_work touches vm->xe which could be gone after gpuvm drops
> potentially the final drm_dev_put, right?
> 

Ignore this - we flush this queue before destorying any device.

Matt

> Matt 
> 
> >  }
> >  
> >  struct xe_vm *xe_vm_lookup(struct xe_file *xef, u32 id)
> > -- 
> > 2.55.0
> > 

Re: [PATCH v3 3/3] drm/xe: Route deferred xe_vma/xe_vm teardown off system_dfl_wq

[Thomas Hellström]

On Fri, 2026-09-25 at 13:18 -0700, Matthew Brost wrote:
> On Fri, Sep 25, 2026 at 12:56:43PM -0700, Matthew Brost wrote:
> > On Fri, Sep 25, 2026 at 03:33:35PM +0200, Thomas Hellström wrote:
> > > xe_vma_destroy() can defer the final teardown of a struct xe_vma
> > > to a
> > > dma_fence completion callback (vma_destroy_cb()), and
> > > xe_vm_free() (the
> > > drm_gpuvm_ops.vm_free callback) always defers struct xe_vm
> > > teardown to
> > > a work item, since destroying a VM needs to sleep. Both used to
> > > queue
> > > their work on system_dfl_wq, a global, kernel-wide workqueue that
> > > xe
> > > has no control over and never waits on during module unload.
> > > 
> > > drm_gpuvm_free() drops its drm_device reference immediately after
> > > calling xe_vm_free(), without waiting for the deferred work to
> > > run.
> > > The same applies one level down: whichever xe_vma or xe_vm
> > > reference
> > > happens to be the last one can trigger this chain from a
> > > dma_fence
> > > callback that may fire at an arbitrary time, including after the
> > > owning file has already been closed and its own module reference
> > > dropped. Since nothing tracks or waits for work queued on
> > > system_dfl_wq, `rmmod xe` could succeed and free the module's
> > > text
> > > while vma_destroy_work_func() or vm_destroy_work_func() is still
> > > queued or running on it, jumping into freed code.
> > > 
> > > Fix this by queueing this work on xe_destroy_wq instead, the
> > > existing
> > > module-lifetime workqueue already used for GuC exec queue
> > > teardown.
> > > Unlike a per-device workqueue, this requires no dereference of a
> > > struct xe_device that may already be gone by the time a deferred
> > > callback fires, and unlike system_dfl_wq it is guaranteed to be
> > > drained by xe_destroy_wq_module_exit() before the module is
> > > unloaded,
> > > following the drm_pagemap_dev_hold()/unhold_work precedent of
> > > using a
> > > workqueue that is waited on at module unload rather than a bare
> > > module
> > > reference. The previous commit's reordering of
> > > xe_destroy_wq_exit()
> > > to run after xe_device_exit() guarantees that xe_destroy_wq is
> > > only
> > > torn down once the device-count has reached zero, i.e. after any
> > > xe_vma or xe_vm whose teardown queues work here has already
> > > dropped
> > > its drm_device reference and thus already queued that work.
> > > 
> > > Signed-off-by: Thomas Hellström
> > > <thomas.hellstrom@linux.intel.com>
> > > Assisted-by: LLM
> > > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_module.c | 6 ++++--
> > >  drivers/gpu/drm/xe/xe_vm.c     | 5 +++--
> > >  2 files changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_module.c
> > > b/drivers/gpu/drm/xe/xe_module.c
> > > index c61bd33546f2..897724cb5cfb 100644
> > > --- a/drivers/gpu/drm/xe/xe_module.c
> > > +++ b/drivers/gpu/drm/xe/xe_module.c
> > > @@ -114,8 +114,10 @@ static void xe_destroy_wq_module_exit(void)
> > >   * xe_destroy_wq_queue() - Queue work on the destroy workqueue
> > >   * @work: work item to queue
> > >   *
> > > - * The destroy workqueue has module lifetime and is used for GuC
> > > exec queue
> > > - * teardown that can outlive a single xe_device. SVM pagemap
> > > destroy uses the
> > > + * The destroy workqueue has module lifetime, and is guaranteed
> > > to outlive
> > > + * any xe_device, and to be drained before the module is
> > > unloaded. It is used
> > > + * for GuC exec queue and xe_vm/xe_vma teardown that can be
> > > deferred past the
> > > + * lifetime of the xe_device that triggered it. SVM pagemap
> > > destroy uses the
> > >   * per-device xe->destroy_wq instead.
> > >   *
> > >   * Return: %true if @work was queued, %false if it was already
> > > pending.
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > > b/drivers/gpu/drm/xe/xe_vm.c
> > > index 390da884c727..ee369e6c3b28 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -29,6 +29,7 @@
> > >  #include "xe_exec_queue.h"
> > >  #include "xe_gt.h"
> > >  #include "xe_migrate.h"
> > > +#include "xe_module.h"
> > >  #include "xe_pagefault.h"
> > >  #include "xe_pat.h"
> > >  #include "xe_pm.h"
> > > @@ -1249,7 +1250,7 @@ static void vma_destroy_cb(struct dma_fence
> > > *fence,
> > >  	struct xe_vma *vma = container_of(cb, struct xe_vma,
> > > destroy_cb);
> > >  
> > >  	INIT_WORK(&vma->destroy_work, vma_destroy_work_func);
> > > -	queue_work(system_dfl_wq, &vma->destroy_work);
> > > +	xe_destroy_wq_queue(&vma->destroy_work);
> > >  }
> > >  
> > >  static void xe_vm_assert_write_mode_or_garbage_collector(struct
> > > xe_vm *vm)
> > > @@ -2059,7 +2060,7 @@ static void xe_vm_free(struct drm_gpuvm
> > > *gpuvm)
> > >  	struct xe_vm *vm = container_of(gpuvm, struct xe_vm,
> > > gpuvm);
> > >  
> > >  	/* To destroy the VM we need to be able to sleep */
> > > -	queue_work(system_dfl_wq, &vm->destroy_work);
> > > +	xe_destroy_wq_queue(&vm->destroy_work);
> > 
> > Actually this is still unsafe, right?
> > 
> > destroy_work touches vm->xe which could be gone after gpuvm drops
> > potentially the final drm_dev_put, right?
> > 
> 
> Ignore this - we flush this queue before destorying any device.

Actually I think you have a point. The module-wide queue is destroyed
as the very last thing the module does. Having both a per-device
destroy queue and a module-wide one is confusing. Let me double-check
this. With this series we could just grab a device reference and
release it when we're done.

Thanks,
Thomas




> 
> Matt
> 
> > Matt 
> > 
> > >  }
> > >  
> > >  struct xe_vm *xe_vm_lookup(struct xe_file *xef, u32 id)
> > > -- 
> > > 2.55.0
> > > 

Re: [PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

[Danilo Krummrich]

On Fri Sep 25, 2026 at 3:33 PM CEST, Thomas Hellström wrote:
> Driver and shared DRM helper code is increasingly relying on bare
> drm_device references (drm_dev_get()/drm_dev_put()) to keep a device's
> software state around, without also pairing that with a reference on
> the owning kernel module. Xe itself does this in several places, and
> so does drm_gpuvm for the lifetime of a GPU VM. None of these
> references currently prevent the owning module from being unloaded
> while they, or the teardown work they can still trigger, are
> outstanding, meaning driver code can end up executing after its own
> module's text has already been freed.

Since you mention DRM GPUVM in a couple of places, how can this ever happen? It
wouldn't make sense to keep a VM alive beyond driver unbind. I.e. it can't make
its drm_device reference count reach module unload in the first place.

Besides that, can you please remind me whether there are any other reasons than
the release() callback why a DRM device must not outlive module unload?

I don't think the correct solution is to constrain module unload. The release()
callback shouldn't really do anything other than free the memory of the
drm_device allocation. All other resources a driver may have should be released
on driver unbind.

There may be shared resources, such as e.g. a common workqueue, but those are
module level things that have nothing to do with the DRM device.

I am aware that a few drivers implement release(), but TBH it looks pretty
broken. qxl_drm_release() is a great example, and it already calls it out
itself.

> This series closes that gap for xe:
>
> - Patch 1 adds core DRM infrastructure allowing a driver to wait for
>   its outstanding device-release callbacks to finish before
>   proceeding with module unload.

Please see above; I also wonder why Xe cares in the first place. Xe doesn't
implement release(), no?

> - Patch 2 makes xe use this infrastructure to hold up module unload
>   until every xe_device instance has actually been released, rather
>   than only until the module's own refcount happens to reach zero,
>   with a diagnostic if this ends up taking an unexpectedly long time.
>
> - Patch 3 fixes a related, previously unprotected case where the
>   teardown of a GPU VM or its address space mappings can be deferred
>   to run at an arbitrary later time, including after module unload has
>   already completed.

Huh? GPUVM tracks the GPU's VA space mappings, but after driver unbind there's
no access to the GPU to manage anything anymore. How can this even work?

Thanks,
Danilo

Re: [PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

[Thomas Hellström]

On Fri, 2026-09-25 at 18:18 +0200, Danilo Krummrich wrote:
> On Fri Sep 25, 2026 at 3:33 PM CEST, Thomas Hellström wrote:
> > Driver and shared DRM helper code is increasingly relying on bare
> > drm_device references (drm_dev_get()/drm_dev_put()) to keep a
> > device's
> > software state around, without also pairing that with a reference
> > on
> > the owning kernel module. Xe itself does this in several places,
> > and
> > so does drm_gpuvm for the lifetime of a GPU VM. None of these
> > references currently prevent the owning module from being unloaded
> > while they, or the teardown work they can still trigger, are
> > outstanding, meaning driver code can end up executing after its own
> > module's text has already been freed.
> 
> Since you mention DRM GPUVM in a couple of places, how can this ever
> happen? It
> wouldn't make sense to keep a VM alive beyond driver unbind. I.e. it
> can't make
> its drm_device reference count reach module unload in the first
> place.

There seems to be a bit of misunderstanding here.

Driver unbind removes the struct device from the driver, triggers
device unplug, and eventually the devres release actions. IIRC the last
devres action removes a *single reference* on the struct drm_device. 
Hence if there exists other reference holders on the struct drm_device
(open files, exported dma-bufs, exported drm_pagemaps as an example),
the drm_device will survive the driver unbind. So will open files and
thus drm_gpuvms until user-space decides to remove them.

Files, dma-bufs and drm_pagemaps all hold a driver module reference
until they have successfully released the drm_device. The requirement
is "If a drm_device reference is held, a module reference of the driver
providing the drm_device must also be held, or if it's held by the
driver itself, it must ensure at driver unload time that any drm_device
references it holds are released and drmm release callbacks have
finished executing."

What this series in effect does is to change this to to "The driver
won't unload until all drm_device references are gone, and all drmm
release callbacks have finished executing."

I agree that the use of drm_gpuvm in the documentation is a bit unfair.
Since the code calling drm_dev_get() and drm_dev_put() is intended to
be called from the driver, the reference in effect becomes the driver's
responsibility, but if someone would, in the future change that so that
those references are put from a worker from within the driver or even
within drm_gpuvm itself, things would break. If a future code reviewer,
developer or AI agent knows about the new drm_device reference
guarantee, then that will lessen the review scope and code will become
more rubost.

> 
> Besides that, can you please remind me whether there are any other
> reasons than
> the release() callback why a DRM device must not outlive module
> unload?

The drmm release callbacks.

> 
> I don't think the correct solution is to constrain module unload. The
> release()
> callback shouldn't really do anything other than free the memory of
> the
> drm_device allocation. All other resources a driver may have should
> be released
> on driver unbind.

That is not true. See above. The alternative would be, as mentioned in
the commit message of patch 2, IIRC that each drm_device itself hold a
module reference of the creating module. But then you wouldn't be able
to execute rmmod with devices alive, You would have to manually unbind
all devices first.


> 
> There may be shared resources, such as e.g. a common workqueue, but
> those are
> module level things that have nothing to do with the DRM device.

I don't think that is correct either. Take a look at Matt Brost reply
to patch 3 there where he points out that the drm device (a base class
of the xe device) is acually referenced in a work item after the struct
drm_device reference is put. (There is a workqueue naming confusion in
xe, but I do believe that patch needs a fix). With the poposed series
in place a simple fix would be to hold a drm_device reference across
the workqueue item. The unload process would then block until all
devices are unreferenced, and then again at destroy_worqueue time
waiting for the work item epilogue to finish executing.

> 
> I am aware that a few drivers implement release(), but TBH it looks
> pretty
> broken. qxl_drm_release() is a great example, and it already calls it
> out
> itself.

drm_release is not of major interest.

> 
> > This series closes that gap for xe:
> > 
> > - Patch 1 adds core DRM infrastructure allowing a driver to wait
> > for
> >   its outstanding device-release callbacks to finish before
> >   proceeding with module unload.
> 
> Please see above; I also wonder why Xe cares in the first place. Xe
> doesn't
> implement release(), no?

See above.

> 
> > - Patch 2 makes xe use this infrastructure to hold up module unload
> >   until every xe_device instance has actually been released, rather
> >   than only until the module's own refcount happens to reach zero,
> >   with a diagnostic if this ends up taking an unexpectedly long
> > time.
> > 
> > - Patch 3 fixes a related, previously unprotected case where the
> >   teardown of a GPU VM or its address space mappings can be
> > deferred
> >   to run at an arbitrary later time, including after module unload
> > has
> >   already completed.
> 
> Huh? GPUVM tracks the GPU's VA space mappings, but after driver
> unbind there's
> no access to the GPU to manage anything anymore. How can this even
> work?
> 
> 
> 

As previously mentioned, software device state may well outlive a
driver unbind. This is all about its cleanup.

Thanks,
Thomas


> 
> Thanks,
> Danilo

Re: [PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

[Danilo Krummrich]

On Mon Sep 28, 2026 at 10:46 AM CEST, Thomas Hellström wrote:
> On Fri, 2026-09-25 at 18:18 +0200, Danilo Krummrich wrote:
>> On Fri Sep 25, 2026 at 3:33 PM CEST, Thomas Hellström wrote:
>> > Driver and shared DRM helper code is increasingly relying on bare
>> > drm_device references (drm_dev_get()/drm_dev_put()) to keep a
>> > device's
>> > software state around, without also pairing that with a reference
>> > on
>> > the owning kernel module. Xe itself does this in several places,
>> > and
>> > so does drm_gpuvm for the lifetime of a GPU VM. None of these
>> > references currently prevent the owning module from being unloaded
>> > while they, or the teardown work they can still trigger, are
>> > outstanding, meaning driver code can end up executing after its own
>> > module's text has already been freed.
>> 
>> Since you mention DRM GPUVM in a couple of places, how can this ever
>> happen? It
>> wouldn't make sense to keep a VM alive beyond driver unbind. I.e. it
>> can't make
>> its drm_device reference count reach module unload in the first
>> place.
>
> There seems to be a bit of misunderstanding here.
>
> Driver unbind removes the struct device from the driver, triggers
> device unplug, and eventually the devres release actions. IIRC the last
> devres action removes a *single reference* on the struct drm_device.

Correct.

> Hence if there exists other reference holders on the struct drm_device
> (open files, exported dma-bufs, exported drm_pagemaps as an example),
> the drm_device will survive the driver unbind. So will open files and
> thus drm_gpuvms until user-space decides to remove them.

There's two lifetimes we have to deal with in drivers: the lifetime of (bus /
physical) device resources, which are managed by the driver and the software
state that is represented through the class device to userspace (e.g. file
handles).

The former is bounded to the scope where the driver is bound to the device and
the latter is unbounded and indeed depends on userspace.

Either the subsystem or the driver has to decouple those lifetimes. I.e. if the
driver is unbound it should clean up all GPUVMs as they represent the GPU's
virtual address space and hence are associated with the hardware. However, the
driver should not operated the hardware anymore after driver unbind.

So, in your case it seems that file lifetime and VM lifetime are conflated
although they should be separate.

We can't have userspace to decide when we drop device resources, such as DMA
mappings, I/O memory mappings, etc.

> Files, dma-bufs and drm_pagemaps all hold a driver module reference
> until they have successfully released the drm_device. The requirement
> is "If a drm_device reference is held, a module reference of the driver
> providing the drm_device must also be held, or if it's held by the
> driver itself, it must ensure at driver unload time that any drm_device
> references it holds are released and drmm release callbacks have
> finished executing."
>
> What this series in effect does is to change this to to "The driver
> won't unload until all drm_device references are gone, and all drmm
> release callbacks have finished executing."
>
> I agree that the use of drm_gpuvm in the documentation is a bit unfair.
> Since the code calling drm_dev_get() and drm_dev_put() is intended to
> be called from the driver, the reference in effect becomes the driver's
> responsibility, but if someone would, in the future change that so that
> those references are put from a worker from within the driver or even
> within drm_gpuvm itself, things would break. If a future code reviewer,
> developer or AI agent knows about the new drm_device reference
> guarantee, then that will lessen the review scope and code will become
> more rubost.
>
>> 
>> Besides that, can you please remind me whether there are any other
>> reasons than
>> the release() callback why a DRM device must not outlive module
>> unload?
>
> The drmm release callbacks.

Right, I forgot about them for a second. However, they are similar to the
release() callbacks as in they are the wrong cleanup model for driver private
structures.

drmm is a great tool for common subsystem structures that lifetime wise tie to
the drm_device. But it is the wrong lifetime model for stuff that is used to
operate the device, as this should be torn down on device unbind.

I had a quick look at Xe and found this for instance:

	 drmm_add_action_or_reset(&xe->drm, control_fini_action, gt)

control_fini_action() stops a worker that writes device registeres, which must
not be done after driver unbind anymore.

Now, there's two options, either after driver unbind this work is never running
(which would be correct), but then this could have been
devm_add_action_or_reset(), or it does actually run after driver unbind, but
this would violate the driver model.

>> I don't think the correct solution is to constrain module unload. The
>> release()
>> callback shouldn't really do anything other than free the memory of
>> the
>> drm_device allocation. All other resources a driver may have should
>> be released
>> on driver unbind.
>
> That is not true. See above.

I know it is not true in practice, but we are doing the wrong thing. We are
conflating the unbounded userspace lifetime with the bounded lifetime from the
driver model.

I also know that we can't fix this easily, but I want to create some awareness,
especially when we introduce more band aid for the status quo, such that we can
subsequently address the fundamental lifetime problems.

>> There may be shared resources, such as e.g. a common workqueue, but
>> those are
>> module level things that have nothing to do with the DRM device.
>
> I don't think that is correct either. Take a look at Matt Brost reply
> to patch 3 there where he points out that the drm device (a base class
> of the xe device) is acually referenced in a work item after the struct
> drm_device reference is put. (There is a workqueue naming confusion in
> xe, but I do believe that patch needs a fix). With the poposed series
> in place a simple fix would be to hold a drm_device reference across
> the workqueue item. The unload process would then block until all
> devices are unreferenced, and then again at destroy_worqueue time
> waiting for the work item epilogue to finish executing.

Well, but the workqueue itself has a bounded lifetime, which should either be
driver unbind or module unload (when shared between driver instances). The work
items themselves should ideally not extend beyond driver unbind, because there
shouldn't be anything to do for the driver after unbind, because all the
hardware should be torn down and not touched at this point anymore.

I had another brief look at Xe and found this:

	drmm_add_action_or_reset(&xe->drm, ggtt_fini_early, ggtt)

where ggtt_fini_early() destroys a workqueue. It also calls

	drm_mm_takedown(&ggtt->mm);

which IIUC is the range allocator for the global GTT. (The hardware is gone on
driver unbind (i.e. no more GGTT is available for the driver), so there's
shouldn't be a need for this drm_mm to live longer than driver unbind).

This model ties the lifetime of all shorter lived resources that are bounded to
the driver unload scope to the unbounded lifetime of the drm_device that is
controlled by userspace.

I.e. the model is backwards and hence also extends your driver structures and
callback entry points not only beyond driver unbind, but also potentially beyond
module unload.

If it would be done the other way around, tear down everything on driver unload,
and then use default trampolines for userspace still trying to call into the
driver (which is also what DMA fence does and other subsystem do), all those
issues go away.

>> > - Patch 3 fixes a related, previously unprotected case where the
>> >   teardown of a GPU VM or its address space mappings can be
>> > deferred
>> >   to run at an arbitrary later time, including after module unload
>> > has
>> >   already completed.
>> 
>> Huh? GPUVM tracks the GPU's VA space mappings, but after driver
>> unbind there's
>> no access to the GPU to manage anything anymore. How can this even
>> work?
>
> As previously mentioned, software device state may well outlive a
> driver unbind. This is all about its cleanup.

GPUVM shouldn't be lifetime wise tied to a software state, it represents
hardware resoruces.

Re: [PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

[Thomas Hellström]

On Mon, 2026-09-28 at 12:13 +0200, Danilo Krummrich wrote:
> On Mon Sep 28, 2026 at 10:46 AM CEST, Thomas Hellström wrote:
> > On Fri, 2026-09-25 at 18:18 +0200, Danilo Krummrich wrote:
> > > On Fri Sep 25, 2026 at 3:33 PM CEST, Thomas Hellström wrote:
> > > > Driver and shared DRM helper code is increasingly relying on
> > > > bare
> > > > drm_device references (drm_dev_get()/drm_dev_put()) to keep a
> > > > device's
> > > > software state around, without also pairing that with a
> > > > reference
> > > > on
> > > > the owning kernel module. Xe itself does this in several
> > > > places,
> > > > and
> > > > so does drm_gpuvm for the lifetime of a GPU VM. None of these
> > > > references currently prevent the owning module from being
> > > > unloaded
> > > > while they, or the teardown work they can still trigger, are
> > > > outstanding, meaning driver code can end up executing after its
> > > > own
> > > > module's text has already been freed.
> > > 
> > > Since you mention DRM GPUVM in a couple of places, how can this
> > > ever
> > > happen? It
> > > wouldn't make sense to keep a VM alive beyond driver unbind. I.e.
> > > it
> > > can't make
> > > its drm_device reference count reach module unload in the first
> > > place.
> > 
> > There seems to be a bit of misunderstanding here.
> > 
> > Driver unbind removes the struct device from the driver, triggers
> > device unplug, and eventually the devres release actions. IIRC the
> > last
> > devres action removes a *single reference* on the struct
> > drm_device.
> 
> Correct.
> 
> > Hence if there exists other reference holders on the struct
> > drm_device
> > (open files, exported dma-bufs, exported drm_pagemaps as an
> > example),
> > the drm_device will survive the driver unbind. So will open files
> > and
> > thus drm_gpuvms until user-space decides to remove them.
> 
> There's two lifetimes we have to deal with in drivers: the lifetime
> of (bus /
> physical) device resources, which are managed by the driver and the
> software
> state that is represented through the class device to userspace (e.g.
> file
> handles).
> 
> The former is bounded to the scope where the driver is bound to the
> device and
> the latter is unbounded and indeed depends on userspace.
> 
> Either the subsystem or the driver has to decouple those lifetimes.
> I.e. if the
> driver is unbound it should clean up all GPUVMs as they represent the
> GPU's
> virtual address space and hence are associated with the hardware.
> However, the
> driver should not operated the hardware anymore after driver unbind.

I disagree here. At unbind time we decouple the HW and SW state, The
device no longer uses it's pointers to the page-table so, for example
VRAM page-tables can be torn down, system page-tables lose their dma-
mappings, but in xe we don't tear down the page-table structure itself.

If HW accesses are properly protected by drm_dev_enter() /
drm_dev_exit(), Hw won't be accessed after unbind.


> 
> So, in your case it seems that file lifetime and VM lifetime are
> conflated
> although they should be separate.

I view the VM as software state, page-table pointers, dma-mappings and
VRAM storage as HW state.

It seems like what we're not agreeing on is where to separate those. I
see no reason as to why we would complicate the driver to remove more
than necessary at unbind time?

> 
> We can't have userspace to decide when we drop device resources, such
> as DMA
> mappings, I/O memory mappings, etc.

We don't (Unless we have bugs, and you may have stumbled on those
below?) Those should be removed at unbind time. We should also revoke
dma-buf mappings and SVM migrates all dma-buf mappings to system.

> 
> > Files, dma-bufs and drm_pagemaps all hold a driver module reference
> > until they have successfully released the drm_device. The
> > requirement
> > is "If a drm_device reference is held, a module reference of the
> > driver
> > providing the drm_device must also be held, or if it's held by the
> > driver itself, it must ensure at driver unload time that any
> > drm_device
> > references it holds are released and drmm release callbacks have
> > finished executing."
> > 
> > What this series in effect does is to change this to to "The driver
> > won't unload until all drm_device references are gone, and all drmm
> > release callbacks have finished executing."
> > 
> > I agree that the use of drm_gpuvm in the documentation is a bit
> > unfair.
> > Since the code calling drm_dev_get() and drm_dev_put() is intended
> > to
> > be called from the driver, the reference in effect becomes the
> > driver's
> > responsibility, but if someone would, in the future change that so
> > that
> > those references are put from a worker from within the driver or
> > even
> > within drm_gpuvm itself, things would break. If a future code
> > reviewer,
> > developer or AI agent knows about the new drm_device reference
> > guarantee, then that will lessen the review scope and code will
> > become
> > more rubost.
> > 
> > > 
> > > Besides that, can you please remind me whether there are any
> > > other
> > > reasons than
> > > the release() callback why a DRM device must not outlive module
> > > unload?
> > 
> > The drmm release callbacks.
> 
> Right, I forgot about them for a second. However, they are similar to
> the
> release() callbacks as in they are the wrong cleanup model for driver
> private
> structures.
> 
> drmm is a great tool for common subsystem structures that lifetime
> wise tie to
> the drm_device. But it is the wrong lifetime model for stuff that is
> used to
> operate the device, as this should be torn down on device unbind.

The current model used by xe (and amdgpu AFACT, that also ties vm
lifetime to file lifetime) is to block all hardware access and dma at
unbind time. The rest is state that doesn't necessarily need to be torn
down at unbind time. I believe the current separation is mostly done
with drm_dev_enter() / drm_dev_exit() and why should we enforce a
change of that? I'd say the drivers should be free to release what's
convenient.

Also if drm_gpuvms are designed to not outlive the struct device, why
do they need to take a struct drm_device reference in the first place,
I mean I brought this problem up then and IIRC I think you argued the
reference was needed and punted any problems it caused to the drivers?

> 
> I had a quick look at Xe and found this for instance:
> 
> 	 drmm_add_action_or_reset(&xe->drm, control_fini_action, gt)
> 
> control_fini_action() stops a worker that writes device registeres,
> which must
> not be done after driver unbind anymore.
> 
> Now, there's two options, either after driver unbind this work is
> never running
> (which would be correct), but then this could have been
> devm_add_action_or_reset(), or it does actually run after driver
> unbind, but
> this would violate the driver model.

Agreed, Unless there is something protecting the hardware access after
unplug in that control subsystem, that's a genuine bug, but that's
separate from this discussion


> 
> > > I don't think the correct solution is to constrain module unload.
> > > The
> > > release()
> > > callback shouldn't really do anything other than free the memory
> > > of
> > > the
> > > drm_device allocation. All other resources a driver may have
> > > should
> > > be released
> > > on driver unbind.
> > 
> > That is not true. See above.
> 
> I know it is not true in practice, but we are doing the wrong thing.
> We are
> conflating the unbounded userspace lifetime with the bounded lifetime
> from the
> driver model.

I don't think we are. As long as all HW access is given up or blocked,
we're fine.

> 
> I also know that we can't fix this easily, but I want to create some
> awareness,
> especially when we introduce more band aid for the status quo, such
> that we can
> subsequently address the fundamental lifetime problems.
> 
> > > There may be shared resources, such as e.g. a common workqueue,
> > > but
> > > those are
> > > module level things that have nothing to do with the DRM device.
> > 
> > I don't think that is correct either. Take a look at Matt Brost
> > reply
> > to patch 3 there where he points out that the drm device (a base
> > class
> > of the xe device) is acually referenced in a work item after the
> > struct
> > drm_device reference is put. (There is a workqueue naming confusion
> > in
> > xe, but I do believe that patch needs a fix). With the poposed
> > series
> > in place a simple fix would be to hold a drm_device reference
> > across
> > the workqueue item. The unload process would then block until all
> > devices are unreferenced, and then again at destroy_worqueue time
> > waiting for the work item epilogue to finish executing.
> 
> Well, but the workqueue itself has a bounded lifetime, which should
> either be
> driver unbind or module unload (when shared between driver
> instances). The work
> items themselves should ideally not extend beyond driver unbind,
> because there
> shouldn't be anything to do for the driver after unbind, because all
> the
> hardware should be torn down and not touched at this point anymore.

Hardware isn't touched but I don't think there is any reason for
cleanup tasks to stop executing at unbind time?

> 
> I had another brief look at Xe and found this:
> 
> 	drmm_add_action_or_reset(&xe->drm, ggtt_fini_early, ggtt)
> 
> where ggtt_fini_early() destroys a workqueue. It also calls
> 
> 	drm_mm_takedown(&ggtt->mm);
> 
> which IIUC is the range allocator for the global GTT. (The hardware
> is gone on
> driver unbind (i.e. no more GGTT is available for the driver), so
> there's
> shouldn't be a need for this drm_mm to live longer than driver
> unbind).

Yes, this *can* probably be taken down at unbind time at the expense of
subsystem and structure validity checking, but it doesn't have to.

> 
> This model ties the lifetime of all shorter lived resources that are
> bounded to
> the driver unload scope to the unbounded lifetime of the drm_device
> that is
> controlled by userspace.
> 
> I.e. the model is backwards and hence also extends your driver
> structures and
> callback entry points not only beyond driver unbind, but also
> potentially beyond
> module unload.
> 
> If it would be done the other way around, tear down everything on
> driver unload,
> and then use default trampolines for userspace still trying to call
> into the
> driver (which is also what DMA fence does and other subsystem do),
> all those
> issues go away.

But that is arguably something that's never going to happen, and even
if it is, I think that's mostly orthogonal to code keeping references
to drm_device.


> 
> > > > - Patch 3 fixes a related, previously unprotected case where
> > > > the
> > > >   teardown of a GPU VM or its address space mappings can be
> > > > deferred
> > > >   to run at an arbitrary later time, including after module
> > > > unload
> > > > has
> > > >   already completed.
> > > 
> > > Huh? GPUVM tracks the GPU's VA space mappings, but after driver
> > > unbind there's
> > > no access to the GPU to manage anything anymore. How can this
> > > even
> > > work?
> > 
> > As previously mentioned, software device state may well outlive a
> > driver unbind. This is all about its cleanup.
> 
> GPUVM shouldn't be lifetime wise tied to a software state, it
> represents
> hardware resoruces.

So then we can remove it's drm device reference? Why would it need to
keep a reference to the software state guaranteed to outlive it?

Regardless, I can remove all mentions of GPUVM in the documentation,
but that doesn't remove the fact that keeping a reference to a struct
drm_device without a module reference or a module presence guarantee is
extremely fragile, and will be also in a model that removes a larger
part of its software state at unbind time. IMO removing that fragility
is a good thing.

Thanks,
Thomas

Re: [PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

[Danilo Krummrich]

On Mon Sep 28, 2026 at 2:13 PM CEST, Thomas Hellström wrote:
> On Mon, 2026-09-28 at 12:13 +0200, Danilo Krummrich wrote:
>> On Mon Sep 28, 2026 at 10:46 AM CEST, Thomas Hellström wrote:
>> > On Fri, 2026-09-25 at 18:18 +0200, Danilo Krummrich wrote:
>> > > On Fri Sep 25, 2026 at 3:33 PM CEST, Thomas Hellström wrote:
>> > > > Driver and shared DRM helper code is increasingly relying on
>> > > > bare
>> > > > drm_device references (drm_dev_get()/drm_dev_put()) to keep a
>> > > > device's
>> > > > software state around, without also pairing that with a
>> > > > reference
>> > > > on
>> > > > the owning kernel module. Xe itself does this in several
>> > > > places,
>> > > > and
>> > > > so does drm_gpuvm for the lifetime of a GPU VM. None of these
>> > > > references currently prevent the owning module from being
>> > > > unloaded
>> > > > while they, or the teardown work they can still trigger, are
>> > > > outstanding, meaning driver code can end up executing after its
>> > > > own
>> > > > module's text has already been freed.
>> > > 
>> > > Since you mention DRM GPUVM in a couple of places, how can this
>> > > ever
>> > > happen? It
>> > > wouldn't make sense to keep a VM alive beyond driver unbind. I.e.
>> > > it
>> > > can't make
>> > > its drm_device reference count reach module unload in the first
>> > > place.
>> > 
>> > There seems to be a bit of misunderstanding here.
>> > 
>> > Driver unbind removes the struct device from the driver, triggers
>> > device unplug, and eventually the devres release actions. IIRC the
>> > last
>> > devres action removes a *single reference* on the struct
>> > drm_device.
>> 
>> Correct.
>> 
>> > Hence if there exists other reference holders on the struct
>> > drm_device
>> > (open files, exported dma-bufs, exported drm_pagemaps as an
>> > example),
>> > the drm_device will survive the driver unbind. So will open files
>> > and
>> > thus drm_gpuvms until user-space decides to remove them.
>> 
>> There's two lifetimes we have to deal with in drivers: the lifetime
>> of (bus /
>> physical) device resources, which are managed by the driver and the
>> software
>> state that is represented through the class device to userspace (e.g.
>> file
>> handles).
>> 
>> The former is bounded to the scope where the driver is bound to the
>> device and
>> the latter is unbounded and indeed depends on userspace.
>> 
>> Either the subsystem or the driver has to decouple those lifetimes.
>> I.e. if the
>> driver is unbound it should clean up all GPUVMs as they represent the
>> GPU's
>> virtual address space and hence are associated with the hardware.
>> However, the
>> driver should not operated the hardware anymore after driver unbind.
>
> I disagree here. At unbind time we decouple the HW and SW state, The
> device no longer uses it's pointers to the page-table so, for example
> VRAM page-tables can be torn down, system page-tables lose their dma-
> mappings, but in xe we don't tear down the page-table structure itself.
>
> If HW accesses are properly protected by drm_dev_enter() /
> drm_dev_exit(), Hw won't be accessed after unbind.
>
>
>> 
>> So, in your case it seems that file lifetime and VM lifetime are
>> conflated
>> although they should be separate.
>
> I view the VM as software state, page-table pointers, dma-mappings and
> VRAM storage as HW state.
>
> It seems like what we're not agreeing on is where to separate those. I
> see no reason as to why we would complicate the driver to remove more
> than necessary at unbind time?

This can certainly be done, correct. But, the VM itself represents a GPU's
virtual address space and takes ownership of the corresponding hardware
resources.

We can indeed revoke the hardware resources from the VM implementation and
leave it in place. But this messes with the ownership model within the VM
implementation:

Because now, and you say this a couple of times below, we need to guard all
relevant entry points into the VM code with guards, such as
drm_dev_{enter/exit}().

IOW, it creates partially uninitialized structures with stale pointers that we
now have to guard against.

It makes much more sense to tear down everything that owns device resources on
driver unbind. I.e. why keep structures with stale pointers around that we have
to guard against in the first place?

It also gets us rid of the module unload issue as it allows us to prevent
callbacks into the driver code after driver unbind on the subsystem level.

>> We can't have userspace to decide when we drop device resources, such
>> as DMA
>> mappings, I/O memory mappings, etc.
>
> We don't (Unless we have bugs, and you may have stumbled on those
> below?) Those should be removed at unbind time. We should also revoke
> dma-buf mappings and SVM migrates all dma-buf mappings to system.
>
>> 
>> > Files, dma-bufs and drm_pagemaps all hold a driver module reference
>> > until they have successfully released the drm_device. The
>> > requirement
>> > is "If a drm_device reference is held, a module reference of the
>> > driver
>> > providing the drm_device must also be held, or if it's held by the
>> > driver itself, it must ensure at driver unload time that any
>> > drm_device
>> > references it holds are released and drmm release callbacks have
>> > finished executing."
>> > 
>> > What this series in effect does is to change this to to "The driver
>> > won't unload until all drm_device references are gone, and all drmm
>> > release callbacks have finished executing."
>> > 
>> > I agree that the use of drm_gpuvm in the documentation is a bit
>> > unfair.
>> > Since the code calling drm_dev_get() and drm_dev_put() is intended
>> > to
>> > be called from the driver, the reference in effect becomes the
>> > driver's
>> > responsibility, but if someone would, in the future change that so
>> > that
>> > those references are put from a worker from within the driver or
>> > even
>> > within drm_gpuvm itself, things would break. If a future code
>> > reviewer,
>> > developer or AI agent knows about the new drm_device reference
>> > guarantee, then that will lessen the review scope and code will
>> > become
>> > more rubost.
>> > 
>> > > 
>> > > Besides that, can you please remind me whether there are any
>> > > other
>> > > reasons than
>> > > the release() callback why a DRM device must not outlive module
>> > > unload?
>> > 
>> > The drmm release callbacks.
>> 
>> Right, I forgot about them for a second. However, they are similar to
>> the
>> release() callbacks as in they are the wrong cleanup model for driver
>> private
>> structures.
>> 
>> drmm is a great tool for common subsystem structures that lifetime
>> wise tie to
>> the drm_device. But it is the wrong lifetime model for stuff that is
>> used to
>> operate the device, as this should be torn down on device unbind.
>
> The current model used by xe (and amdgpu AFACT, that also ties vm
> lifetime to file lifetime) is to block all hardware access and dma at
> unbind time. The rest is state that doesn't necessarily need to be torn
> down at unbind time. I believe the current separation is mostly done
> with drm_dev_enter() / drm_dev_exit() and why should we enforce a
> change of that? I'd say the drivers should be free to release what's
> convenient.

See the reasons above, it is not a good layer for the lifetime decoupling.

> Also if drm_gpuvms are designed to not outlive the struct device, why
> do they need to take a struct drm_device reference in the first place,
> I mean I brought this problem up then and IIRC I think you argued the
> reference was needed and punted any problems it caused to the drivers?

The lifetime of the hardware resources that are owned by a GPUVM implementation
is restricted by the underlying bus device (e.g. PCI) being bound to the driver.

The lifetime of the DRM device as a class device is technically independent, it
can live longer (which is likely), but it could technically also be shorter
lived (which drivers don't do in practice). But even though drivers don't do
this in practice, the dependency should be expressed: if GPUVM stores a pointer
to a DRM device, it has to take a reference count.

>> I had a quick look at Xe and found this for instance:
>> 
>> 	 drmm_add_action_or_reset(&xe->drm, control_fini_action, gt)
>> 
>> control_fini_action() stops a worker that writes device registeres,
>> which must
>> not be done after driver unbind anymore.
>> 
>> Now, there's two options, either after driver unbind this work is
>> never running
>> (which would be correct), but then this could have been
>> devm_add_action_or_reset(), or it does actually run after driver
>> unbind, but
>> this would violate the driver model.
>
> Agreed, Unless there is something protecting the hardware access after
> unplug in that control subsystem, that's a genuine bug, but that's
> separate from this discussion

I argue that it is related; surely, we can keep everything around until the DRM
device is destroyed and just guard every single (callback) entry point.

But, that's far more complicated and error prone than just shutting down the hardware
on driver unbind and tear down all entry points on the subsystem level; it
messes with the ownership model of structures leaving stale pointers behind.

As mentioned, this is what subsystems commonly do in the kernel, and I don't see
why DRM would be special in this regard.

>> > > I don't think the correct solution is to constrain module unload.
>> > > The
>> > > release()
>> > > callback shouldn't really do anything other than free the memory
>> > > of
>> > > the
>> > > drm_device allocation. All other resources a driver may have
>> > > should
>> > > be released
>> > > on driver unbind.
>> > 
>> > That is not true. See above.
>> 
>> I know it is not true in practice, but we are doing the wrong thing.
>> We are
>> conflating the unbounded userspace lifetime with the bounded lifetime
>> from the
>> driver model.
>
> I don't think we are. As long as all HW access is given up or blocked,
> we're fine.

Right, what you describe above works, but it leaves stale pointers and invalid
structures behind that we then need to guard against.

>> I also know that we can't fix this easily, but I want to create some
>> awareness,
>> especially when we introduce more band aid for the status quo, such
>> that we can
>> subsequently address the fundamental lifetime problems.
>> 
>> > > There may be shared resources, such as e.g. a common workqueue,
>> > > but
>> > > those are
>> > > module level things that have nothing to do with the DRM device.
>> > 
>> > I don't think that is correct either. Take a look at Matt Brost
>> > reply
>> > to patch 3 there where he points out that the drm device (a base
>> > class
>> > of the xe device) is acually referenced in a work item after the
>> > struct
>> > drm_device reference is put. (There is a workqueue naming confusion
>> > in
>> > xe, but I do believe that patch needs a fix). With the poposed
>> > series
>> > in place a simple fix would be to hold a drm_device reference
>> > across
>> > the workqueue item. The unload process would then block until all
>> > devices are unreferenced, and then again at destroy_worqueue time
>> > waiting for the work item epilogue to finish executing.
>> 
>> Well, but the workqueue itself has a bounded lifetime, which should
>> either be
>> driver unbind or module unload (when shared between driver
>> instances). The work
>> items themselves should ideally not extend beyond driver unbind,
>> because there
>> shouldn't be anything to do for the driver after unbind, because all
>> the
>> hardware should be torn down and not touched at this point anymore.
>
> Hardware isn't touched but I don't think there is any reason for
> cleanup tasks to stop executing at unbind time?

Well, it has the implications as stated above. And it means that instead of
having a global SRCU read side critical section for a global entry point, we are
forced to leave invalid pointers behind and have drivers protect them each
individually with SRCU (i.e. drm_dev_enter() / drm_dev_exit()).

>> I had another brief look at Xe and found this:
>> 
>> 	drmm_add_action_or_reset(&xe->drm, ggtt_fini_early, ggtt)
>> 
>> where ggtt_fini_early() destroys a workqueue. It also calls
>> 
>> 	drm_mm_takedown(&ggtt->mm);
>> 
>> which IIUC is the range allocator for the global GTT. (The hardware
>> is gone on
>> driver unbind (i.e. no more GGTT is available for the driver), so
>> there's
>> shouldn't be a need for this drm_mm to live longer than driver
>> unbind).
>
> Yes, this *can* probably be taken down at unbind time at the expense of
> subsystem and structure validity checking, but it doesn't have to.

Same reasoning as above.

>> This model ties the lifetime of all shorter lived resources that are
>> bounded to
>> the driver unload scope to the unbounded lifetime of the drm_device
>> that is
>> controlled by userspace.
>> 
>> I.e. the model is backwards and hence also extends your driver
>> structures and
>> callback entry points not only beyond driver unbind, but also
>> potentially beyond
>> module unload.
>> 
>> If it would be done the other way around, tear down everything on
>> driver unload,
>> and then use default trampolines for userspace still trying to call
>> into the
>> driver (which is also what DMA fence does and other subsystem do),
>> all those
>> issues go away.
>
> But that is arguably something that's never going to happen, and even
> if it is, I think that's mostly orthogonal to code keeping references
> to drm_device.

Well, I hope we can move to this model eventually.

I don't think it is orthogonal, if we don't have driver entry pointer after
driver unbind anymore, there is no module unload problem anymore. And structs
that keep a reference to a struct drm_device shouldn't be a problem in this
regard either.

>> > > > - Patch 3 fixes a related, previously unprotected case where
>> > > > the
>> > > >   teardown of a GPU VM or its address space mappings can be
>> > > > deferred
>> > > >   to run at an arbitrary later time, including after module
>> > > > unload
>> > > > has
>> > > >   already completed.
>> > > 
>> > > Huh? GPUVM tracks the GPU's VA space mappings, but after driver
>> > > unbind there's
>> > > no access to the GPU to manage anything anymore. How can this
>> > > even
>> > > work?
>> > 
>> > As previously mentioned, software device state may well outlive a
>> > driver unbind. This is all about its cleanup.
>> 
>> GPUVM shouldn't be lifetime wise tied to a software state, it
>> represents
>> hardware resoruces.
>
> So then we can remove it's drm device reference? Why would it need to
> keep a reference to the software state guaranteed to outlive it?

There is no structural guarantee that it can't happen that the DRM device is
destroyed first, then GPUVM and then the driver is unbound.

> Regardless, I can remove all mentions of GPUVM in the documentation,

That's not my main concern, but I found it to be a good example for what
actually is my concern:

The lifetime model in DRM often conflates software and hardware state
structurally; whether the consequence is either that we just keep hardware
resources alive for longer than they should be alive, or whether we tear them
down and leave invalid pointers behind that we then protect with lots of manual
guard sections does not matter too much for me. Both is a consequence of the
structurally conflated lifetime and ownership.

> but that doesn't remove the fact that keeping a reference to a struct
> drm_device without a module reference or a module presence guarantee is
> extremely fragile, and will be also in a model that removes a larger
> part of its software state at unbind time. IMO removing that fragility
> is a good thing.

Don't get me wrong, I don't object to this patch series. Well, at least not too
much, requiring drivers to call drm_dev_release_barrier() in module unload is
pretty ugly, and I don't know any other subsystem that has such a guard, because
they do structurally prevent callbacks into drivers after driver unbind and
drivers hence tie the lifetime of structures to driver unbind.

But then there's reality and the things are as they are right now, so it might
just be necessary. However, I hope that in the future we can improve this and
have the subsystem provide the required guards on the subsystem callback level.

Re: [PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

[Thomas Hellström]

On Mon, 2026-09-28 at 15:03 +0200, Danilo Krummrich wrote:
> On Mon Sep 28, 2026 at 2:13 PM CEST, Thomas Hellström wrote:
> > On Mon, 2026-09-28 at 12:13 +0200, Danilo Krummrich wrote:
> > > On Mon Sep 28, 2026 at 10:46 AM CEST, Thomas Hellström wrote:
> > > > On Fri, 2026-09-25 at 18:18 +0200, Danilo Krummrich wrote:
> > > > > On Fri Sep 25, 2026 at 3:33 PM CEST, Thomas Hellström wrote:
> > > > > > Driver and shared DRM helper code is increasingly relying
> > > > > > on
> > > > > > bare
> > > > > > drm_device references (drm_dev_get()/drm_dev_put()) to keep
> > > > > > a
> > > > > > device's
> > > > > > software state around, without also pairing that with a
> > > > > > reference
> > > > > > on
> > > > > > the owning kernel module. Xe itself does this in several
> > > > > > places,
> > > > > > and
> > > > > > so does drm_gpuvm for the lifetime of a GPU VM. None of
> > > > > > these
> > > > > > references currently prevent the owning module from being
> > > > > > unloaded
> > > > > > while they, or the teardown work they can still trigger,
> > > > > > are
> > > > > > outstanding, meaning driver code can end up executing after
> > > > > > its
> > > > > > own
> > > > > > module's text has already been freed.
> > > > > 
> > > > > Since you mention DRM GPUVM in a couple of places, how can
> > > > > this
> > > > > ever
> > > > > happen? It
> > > > > wouldn't make sense to keep a VM alive beyond driver unbind.
> > > > > I.e.
> > > > > it
> > > > > can't make
> > > > > its drm_device reference count reach module unload in the
> > > > > first
> > > > > place.
> > > > 
> > > > There seems to be a bit of misunderstanding here.
> > > > 
> > > > Driver unbind removes the struct device from the driver,
> > > > triggers
> > > > device unplug, and eventually the devres release actions. IIRC
> > > > the
> > > > last
> > > > devres action removes a *single reference* on the struct
> > > > drm_device.
> > > 
> > > Correct.
> > > 
> > > > Hence if there exists other reference holders on the struct
> > > > drm_device
> > > > (open files, exported dma-bufs, exported drm_pagemaps as an
> > > > example),
> > > > the drm_device will survive the driver unbind. So will open
> > > > files
> > > > and
> > > > thus drm_gpuvms until user-space decides to remove them.
> > > 
> > > There's two lifetimes we have to deal with in drivers: the
> > > lifetime
> > > of (bus /
> > > physical) device resources, which are managed by the driver and
> > > the
> > > software
> > > state that is represented through the class device to userspace
> > > (e.g.
> > > file
> > > handles).
> > > 
> > > The former is bounded to the scope where the driver is bound to
> > > the
> > > device and
> > > the latter is unbounded and indeed depends on userspace.
> > > 
> > > Either the subsystem or the driver has to decouple those
> > > lifetimes.
> > > I.e. if the
> > > driver is unbound it should clean up all GPUVMs as they represent
> > > the
> > > GPU's
> > > virtual address space and hence are associated with the hardware.
> > > However, the
> > > driver should not operated the hardware anymore after driver
> > > unbind.
> > 
> > I disagree here. At unbind time we decouple the HW and SW state,
> > The
> > device no longer uses it's pointers to the page-table so, for
> > example
> > VRAM page-tables can be torn down, system page-tables lose their
> > dma-
> > mappings, but in xe we don't tear down the page-table structure
> > itself.
> > 
> > If HW accesses are properly protected by drm_dev_enter() /
> > drm_dev_exit(), Hw won't be accessed after unbind.
> > 
> > 
> > > 
> > > So, in your case it seems that file lifetime and VM lifetime are
> > > conflated
> > > although they should be separate.
> > 
> > I view the VM as software state, page-table pointers, dma-mappings
> > and
> > VRAM storage as HW state.
> > 
> > It seems like what we're not agreeing on is where to separate
> > those. I
> > see no reason as to why we would complicate the driver to remove
> > more
> > than necessary at unbind time?
> 
> This can certainly be done, correct. But, the VM itself represents a
> GPU's
> virtual address space and takes ownership of the corresponding
> hardware
> resources.
> 
> We can indeed revoke the hardware resources from the VM
> implementation and
> leave it in place. But this messes with the ownership model within
> the VM
> implementation:
> 
> Because now, and you say this a couple of times below, we need to
> guard all
> relevant entry points into the VM code with guards, such as
> drm_dev_{enter/exit}().
> 
> IOW, it creates partially uninitialized structures with stale
> pointers that we
> now have to guard against.
> 
> It makes much more sense to tear down everything that owns device
> resources on
> driver unbind. I.e. why keep structures with stale pointers around
> that we have
> to guard against in the first place?
> 
> It also gets us rid of the module unload issue as it allows us to
> prevent
> callbacks into the driver code after driver unbind on the subsystem
> level.
> 
> > > We can't have userspace to decide when we drop device resources,
> > > such
> > > as DMA
> > > mappings, I/O memory mappings, etc.
> > 
> > We don't (Unless we have bugs, and you may have stumbled on those
> > below?) Those should be removed at unbind time. We should also
> > revoke
> > dma-buf mappings and SVM migrates all dma-buf mappings to system.
> > 
> > > 
> > > > Files, dma-bufs and drm_pagemaps all hold a driver module
> > > > reference
> > > > until they have successfully released the drm_device. The
> > > > requirement
> > > > is "If a drm_device reference is held, a module reference of
> > > > the
> > > > driver
> > > > providing the drm_device must also be held, or if it's held by
> > > > the
> > > > driver itself, it must ensure at driver unload time that any
> > > > drm_device
> > > > references it holds are released and drmm release callbacks
> > > > have
> > > > finished executing."
> > > > 
> > > > What this series in effect does is to change this to to "The
> > > > driver
> > > > won't unload until all drm_device references are gone, and all
> > > > drmm
> > > > release callbacks have finished executing."
> > > > 
> > > > I agree that the use of drm_gpuvm in the documentation is a bit
> > > > unfair.
> > > > Since the code calling drm_dev_get() and drm_dev_put() is
> > > > intended
> > > > to
> > > > be called from the driver, the reference in effect becomes the
> > > > driver's
> > > > responsibility, but if someone would, in the future change that
> > > > so
> > > > that
> > > > those references are put from a worker from within the driver
> > > > or
> > > > even
> > > > within drm_gpuvm itself, things would break. If a future code
> > > > reviewer,
> > > > developer or AI agent knows about the new drm_device reference
> > > > guarantee, then that will lessen the review scope and code will
> > > > become
> > > > more rubost.
> > > > 
> > > > > 
> > > > > Besides that, can you please remind me whether there are any
> > > > > other
> > > > > reasons than
> > > > > the release() callback why a DRM device must not outlive
> > > > > module
> > > > > unload?
> > > > 
> > > > The drmm release callbacks.
> > > 
> > > Right, I forgot about them for a second. However, they are
> > > similar to
> > > the
> > > release() callbacks as in they are the wrong cleanup model for
> > > driver
> > > private
> > > structures.
> > > 
> > > drmm is a great tool for common subsystem structures that
> > > lifetime
> > > wise tie to
> > > the drm_device. But it is the wrong lifetime model for stuff that
> > > is
> > > used to
> > > operate the device, as this should be torn down on device unbind.
> > 
> > The current model used by xe (and amdgpu AFACT, that also ties vm
> > lifetime to file lifetime) is to block all hardware access and dma
> > at
> > unbind time. The rest is state that doesn't necessarily need to be
> > torn
> > down at unbind time. I believe the current separation is mostly
> > done
> > with drm_dev_enter() / drm_dev_exit() and why should we enforce a
> > change of that? I'd say the drivers should be free to release
> > what's
> > convenient.
> 
> See the reasons above, it is not a good layer for the lifetime
> decoupling.
> 
> > Also if drm_gpuvms are designed to not outlive the struct device,
> > why
> > do they need to take a struct drm_device reference in the first
> > place,
> > I mean I brought this problem up then and IIRC I think you argued
> > the
> > reference was needed and punted any problems it caused to the
> > drivers?
> 
> The lifetime of the hardware resources that are owned by a GPUVM
> implementation
> is restricted by the underlying bus device (e.g. PCI) being bound to
> the driver.
> 
> The lifetime of the DRM device as a class device is technically
> independent, it
> can live longer (which is likely), but it could technically also be
> shorter
> lived (which drivers don't do in practice). But even though drivers
> don't do
> this in practice, the dependency should be expressed: if GPUVM stores
> a pointer
> to a DRM device, it has to take a reference count.

It doesn't have to in a model where all users are removed before the
drm device is freed. Following your argument, shouldn't that drm_device
reference should also be accompanied by a module reference? Except that
will block rmmod?


> 
> > > I had a quick look at Xe and found this for instance:
> > > 
> > > 	 drmm_add_action_or_reset(&xe->drm, control_fini_action,
> > > gt)
> > > 
> > > control_fini_action() stops a worker that writes device
> > > registeres,
> > > which must
> > > not be done after driver unbind anymore.
> > > 
> > > Now, there's two options, either after driver unbind this work is
> > > never running
> > > (which would be correct), but then this could have been
> > > devm_add_action_or_reset(), or it does actually run after driver
> > > unbind, but
> > > this would violate the driver model.
> > 
> > Agreed, Unless there is something protecting the hardware access
> > after
> > unplug in that control subsystem, that's a genuine bug, but that's
> > separate from this discussion
> 
> I argue that it is related; surely, we can keep everything around
> until the DRM
> device is destroyed and just guard every single (callback) entry
> point.
> 
> But, that's far more complicated and error prone than just shutting
> down the hardware
> on driver unbind and tear down all entry points on the subsystem
> level; it
> messes with the ownership model of structures leaving stale pointers
> behind.
> 
> As mentioned, this is what subsystems commonly do in the kernel, and
> I don't see
> why DRM would be special in this regard.

Without knowing for sure, I think this was the route taken with
hotplugging.

> 
> > > > > I don't think the correct solution is to constrain module
> > > > > unload.
> > > > > The
> > > > > release()
> > > > > callback shouldn't really do anything other than free the
> > > > > memory
> > > > > of
> > > > > the
> > > > > drm_device allocation. All other resources a driver may have
> > > > > should
> > > > > be released
> > > > > on driver unbind.
> > > > 
> > > > That is not true. See above.
> > > 
> > > I know it is not true in practice, but we are doing the wrong
> > > thing.
> > > We are
> > > conflating the unbounded userspace lifetime with the bounded
> > > lifetime
> > > from the
> > > driver model.
> > 
> > I don't think we are. As long as all HW access is given up or
> > blocked,
> > we're fine.
> 
> Right, what you describe above works, but it leaves stale pointers
> and invalid
> structures behind that we then need to guard against.

But isn't essentially what you describe a design where we release all
dma_buf, file- and drm_pagemap references of struct drm_device at
module_unload time. That would replace their references with SRCU
protecting the drm_device pointer, falling back to a stub behaviour
when unbind has been called. So the "Can I access hardware?" would be
replaced by a "Can I access the DRM device?". 

That's an interesting idea, but would probably need careful work so
fence waits etc. doesn't block the SRCU read sections. And ofc to avoid
user-space regressing.

> 
> Don't get me wrong, I don't object to this patch series. Well, at
> least not too
> much, requiring drivers to call drm_dev_release_barrier() in module
> unload is
> pretty ugly, and I don't know any other subsystem that has such a
> guard, because
> they do structurally prevent callbacks into drivers after driver
> unbind and
> drivers hence tie the lifetime of structures to driver unbind.

FWIW, IIRC that SRCU is only strictly needed when non-driver code
(pagemap, files and dma-buf) drops the last drm_device reference
without having a module reference. And there is no such code (yet)
AFAIK, so I can drop that patch to when we think it's necessary and we
have other driver buy-ins.


Thanks,
Thomas

Re: [PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

[Danilo Krummrich]

On Mon Sep 28, 2026 at 3:37 PM CEST, Thomas Hellström wrote:
> It doesn't have to in a model where all users are removed before the
> drm device is freed. Following your argument, shouldn't that drm_device
> reference should also be accompanied by a module reference? Except that
> will block rmmod?

Yes, if we establish that a DRM device must outlive a GPUVM (which by convention
we implicitly do by storing a pointer) then you don't need a reference count.

However, there's nothing that ensures the caller sticks to the convention. And
given that the DRM device is reference counted already, taking the reference
count is the correct thing to do.

(As a side note, in Rust you can actually model and enforce this lifetime
relationship at compile time without a reference count. You can even establish
more granular lifetime relationships. For instance, you could have:

	struct GpuVm<'a> {
		drm: &'a drm::Device<Registered>,
	}

and establish that a GPUVM can only ever live as long as the DRM device is
registered and therefore implicitly also establish that the GPUVM can't outlive
driver unbind, as a DRM device can't be registered beyond driver unbind and
hence the lifetime 'a is guaranteed to end before driver unbind.

In C you can only establish this by convention, and at least take the reference
count.)

The module reference seems orthogonal though, GPUVM is not actively emitting
calls into anything (unlike a workqueue for instance), it's a passive data
structure. So, there's no need for any defensive measure AFAICS.

> Without knowing for sure, I think this was the route taken with
> hotplugging.

Sure, both approaches are there to cover hotplugging. Almost all class device
implementations have to consider hotplugging as it is depends on the bus the
physical device sits on whether hot(un)plug can happen.

> But isn't essentially what you describe a design where we release all
> dma_buf, file- and drm_pagemap references of struct drm_device at
> module_unload time. That would replace their references with SRCU
> protecting the drm_device pointer, falling back to a stub behaviour
> when unbind has been called. So the "Can I access hardware?" would be
> replaced by a "Can I access the DRM device?".

I think the question is not "Can I access the DRM device?", the question is "Is
the DRM device still registered?", or IOW, "Is the DRM device still backed by a
driver?".

There's a bounded lifetime when a driver is allowed to operated a device, which
is between probe and remove. This is (typically) the same scope as the class
device (e.g. DRM) is registered.

After the driver is unbound from it's (physical) bus device, there's no value
anymore in letting the driver operate the class device (which is exactly what
register() / unregister() describes) in the first place.

There's nothing hardware specific left at this point, so it is not a driver job
anymore; the lifetime decoupling can be at subsystem / component level (e.g. DMA
fence).

> That's an interesting idea, but would probably need careful work so
> fence waits etc. doesn't block the SRCU read sections. And ofc to avoid
> user-space regressing.

For the fence waits specifically, this can (or should) never happen. Driver
fences must be signaled on driver unbind. The hardware is gone at this point,
there's nothing left that could signal them otherwise.

> FWIW, IIRC that SRCU is only strictly needed when non-driver code
> (pagemap, files and dma-buf) drops the last drm_device reference
> without having a module reference. And there is no such code (yet)
> AFAIK, so I can drop that patch to when we think it's necessary and we
> have other driver buy-ins.

I think those components should do the lifetime decoupling work instead. Once
the driver is unbound, none of the driver callbacks are "special" anymore as
there's nothing hardware specific left at this point.