Reviewed-by: Lyude Paul <[email protected]>
On Sun, 2026-06-28 at 16:53 +0200, Danilo Krummrich wrote:
> If drm_dev_register() fails after registering a minor (e.g. render
> minor
> registered, primary minor fails), userspace could have opened the
> first
> minor and entered a drm_dev_enter() critical section. Since the
> unplugged flag was never set, the ioctl proceeds while the error path
> tears down device resources.
>
> Fix this by introducing drm_dev_synchronize_unplug(), which sets the
> unplugged flag and waits for the SRCU barrier, ensuring all in-flight
> drm_dev_enter() critical sections complete before cleanup proceeds;
> call
> it on the error path of drm_dev_register().
>
> Fixes: bee330f3d672 ("drm: Use srcu to protect drm_device.unplugged")
> Cc: [email protected]
> Reported-by: [email protected]
> Closes:
> https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Danilo Krummrich <[email protected]>
> ---
> drivers/gpu/drm/drm_drv.c | 34 +++++++++++++++++++++++++---------
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 675675480da4..e890052061f3 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -473,6 +473,22 @@ void drm_dev_exit(int idx)
> }
> EXPORT_SYMBOL(drm_dev_exit);
>
> +/*
> + * Mark the device as unplugged and wait for any in-flight
> drm_dev_enter()
> + * critical sections to complete.
> + */
> +static void drm_dev_synchronize_unplug(struct drm_device *dev)
> +{
> + /*
> + * After synchronizing any critical read section is
> guaranteed to see
> + * the new value of ->unplugged, and any critical section
> which might
> + * still have seen the old value of ->unplugged is
> guaranteed to have
> + * finished.
> + */
> + dev->unplugged = true;
> + synchronize_srcu(&drm_unplug_srcu);
> +}
> +
> /**
> * drm_dev_unplug - unplug a DRM device
> * @dev: DRM device
> @@ -485,15 +501,7 @@ EXPORT_SYMBOL(drm_dev_exit);
> */
> void drm_dev_unplug(struct drm_device *dev)
> {
> - /*
> - * After synchronizing any critical read section is
> guaranteed to see
> - * the new value of ->unplugged, and any critical section
> which might
> - * still have seen the old value of ->unplugged is
> guaranteed to have
> - * finished.
> - */
> - dev->unplugged = true;
> - synchronize_srcu(&drm_unplug_srcu);
> -
> + drm_dev_synchronize_unplug(dev);
> drm_dev_unregister(dev);
>
> /* Clear all CPU mappings pointing to this device */
> @@ -1091,6 +1099,7 @@ int drm_dev_register(struct drm_device *dev,
> unsigned long flags)
> goto err_minors;
>
> dev->registered = true;
> + dev->unplugged = false;
>
> if (driver->load) {
> ret = driver->load(dev, flags);
> @@ -1118,6 +1127,13 @@ int drm_dev_register(struct drm_device *dev,
> unsigned long flags)
> if (dev->driver->unload)
> dev->driver->unload(dev);
> err_minors:
> + /*
> + * If a minor was registered before the failure, userspace
> could have
> + * opened it and entered a drm_dev_enter() critical section.
> Ensure all
> + * such sections complete before we clean up.
> + */
> + drm_dev_synchronize_unplug(dev);
> +
> remove_compat_control_link(dev);
> drm_minor_unregister(dev, DRM_MINOR_ACCEL);
> drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
