Re: [PATCH v3 1/2] drm: Unwind drm device registration upon error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Debugfs pollution due to early destruction of dev->debugfs_root in drm_dev_register() error path.
--

commit d5c910dd66292cf0f42ddf2dd15a2566f8663a8a
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drm: Unwind drm device registration upon error

The drm device registration is done via drm_dev_register().
This function attempts to undo some of the initiatlization steps
under err_unload and err_minors labels, but this process is
incomplete - debugfs entries remain and the dev->registered flag
is still set to true.

> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index cb53baa70995f..2d964c9535f49 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -1144,6 +1144,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
>  	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
>  	drm_minor_unregister(dev, DRM_MINOR_RENDER);
> +	drm_debugfs_dev_fini(dev);

[Severity: Medium]
Will this cause debugfs pollution if a driver retries registration after a
failure?

In drivers like amdgpu, there is a retry mechanism for registration:

drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:amdgpu_pci_probe() {
    ...
retry_init:
	ret = drm_dev_register(ddev, flags);
	if (ret == -EAGAIN && ++retry <= 3) {
		drm_info(adev_to_drm(adev), "retry init %d\n", retry);
		/* Don't request EX mode too frequently which is attacking */
		msleep(5000);
		goto retry_init;
	}
    ...
}

Because dev->debugfs_root is initialized during drm_dev_init(), calling
drm_debugfs_dev_fini() here in the error path of drm_dev_register() destroys
the directory and permanently sets dev->debugfs_root to NULL.

If the driver retries drm_dev_register() on the same device instance,
drm_debugfs_dev_register() will pass the NULL dev->debugfs_root to
debugfs_create_file(). This causes device-specific debugfs files (like
'clients', 'gem_names', 'framebuffer') to be created in the global debugfs
root (/sys/kernel/debug/) instead of the device-specific directory, polluting
the global namespace and causing naming collisions.

Should dev->debugfs_root only be destroyed in drm_dev_release() to maintain
object lifecycle symmetry?

> +	dev->registered = false;
>  out_unlock:
>  	if (drm_dev_needs_global_mutex(dev))
>  		mutex_unlock(&drm_global_mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827143447.99553-1-krzysztof.niemiec@intel.com?part=1