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. Rather than a single SRCU domain shared by all drivers, which would mean drm_dev_release_barrier() could unnecessarily block a driver's module unload on unrelated drivers' release callbacks, require each driver that wants to use drm_dev_release_barrier() to supply its own SRCU domain via a new &drm_driver.release_srcu field. Drivers should define a static SRCU domain (DEFINE_STATIC_SRCU()) and set this field to point at it. Leaving the field unset means @release and drm managed release actions for that driver's devices simply aren't synchronized against, and calling drm_dev_release_barrier() for such a driver is a no-op that triggers a warning. Since &drm_driver.release_srcu is a new field, every existing struct drm_driver instance in the tree was scanned to confirm none of them would leave it uninitialized with indeterminate content. All in-tree instances have static storage duration (plain or const static file-scope objects), or are members of a KUnit test fixture zeroed via kunit_kzalloc(); no instance is stack-allocated or heap-allocated without zeroing. Objects with static storage duration are guaranteed by the C standard to have any member without an explicit initializer zero-initialized, so the new field is reliably NULL, and drm_dev_release() simply skips the SRCU critical section, for all drivers that don't set it. 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) Signed-off-by: Thomas Hellström <[email protected]> Assisted-by: LLM --- drivers/gpu/drm/drm_drv.c | 56 +++++++++++++++++++++++++++++++++++++++ include/drm/drm_drv.h | 24 +++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 0cdc606af8d1..32a03170383c 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -921,18 +921,57 @@ EXPORT_SYMBOL(drm_dev_alloc); static void drm_dev_release(struct kref *ref) { struct drm_device *dev = container_of(ref, struct drm_device, ref); + struct srcu_struct *srcu = dev->driver->release_srcu; + int idx = -1; /* Just in case register/unregister was never called */ drm_debugfs_dev_fini(dev); + if (srcu) + idx = srcu_read_lock(srcu); + if (dev->driver->release) dev->driver->release(dev); drm_managed_release(dev); + if (srcu) + srcu_read_unlock(srcu, idx); + kfree(dev->managed.final_kfree); } +/** + * drm_dev_release_barrier() - Ensure drm device release callbacks are finished + * @driver: driver whose release callbacks to wait for + * + * If a device release method or any of the drm managed release callbacks + * have been called for a device created with @driver, 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 the 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. + * + * @driver must have &drm_driver.release_srcu set to a driver-owned + * &struct srcu_struct for this function to have anything to wait for. + * + * 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(const struct drm_driver *driver) +{ + if (WARN_ON_ONCE(!driver || !driver->release_srcu)) + return; + + synchronize_srcu(driver->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 +997,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..30bb8727a896 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -48,6 +48,7 @@ struct drm_display_mode; struct drm_mode_create_dumb; struct drm_printer; struct sg_table; +struct srcu_struct; /** * enum drm_driver_feature - feature flags @@ -255,6 +256,28 @@ struct drm_driver { */ void (*release) (struct drm_device *); + /** + * @release_srcu: + * + * Optional driver-owned SRCU domain used to synchronize completion of + * the @release callback and drm managed release actions with + * drm_dev_release_barrier(). + * + * Left unset, @release and drm managed release actions for this + * driver's devices aren't synchronized with drm_dev_release_barrier() + * at all, and calling drm_dev_release_barrier() for this driver is a + * no-op that triggers a warning. + * + * Drivers that want to use drm_dev_release_barrier(), for example to + * help determine when it's safe to unload the driver module, should + * define their own static SRCU domain (DEFINE_STATIC_SRCU()) and set + * this field to point at it. Each driver should use its own domain, + * so that drm_dev_release_barrier() only waits for that driver's own + * release callbacks, rather than also for unrelated drivers sharing + * the same domain. + */ + struct srcu_struct *release_srcu; + /** * @master_set: * @@ -485,6 +508,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(const struct drm_driver *driver); /** * drm_dev_is_unplugged - is a DRM device unplugged -- 2.55.0
