Re: [PATCH 2/7] rust: drm: gem: gate open/close callbacks with RegistrationGuard

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bypassing `T::close` when the DRM device is unregistered leaks driver-specific resources and software state allocated in `T::open`.
- [High] Wrapping the entire `open` and `close` callbacks in `RegistrationGuard` introduces a severe AB-BA deadlock hazard with the device unplug path.
--

--- Patch [2]: [PATCH 2/7] rust: drm: gem: gate open/close callbacks with RegistrationGuard ---
commit 56b4431da0d3b7c0798814690d882352e01b7d71
Author: Danilo Krummrich <[email protected]>

rust: drm: gem: gate open/close callbacks with RegistrationGuard
    
Wrap the GEM object open and close callbacks with a RegistrationGuard
(drm_dev_enter / drm_dev_exit) to ensure the driver callbacks only run
while the parent bus device is bound.

If the device has been unbound, open returns -ENODEV and close silently
returns. This prevents driver code from accessing device resources after
unbind and is a prerequisite for making drm::Driver::File
lifetime-parameterized.

> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index 80d8f524f9d57..560403ca8e38f 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -128,6 +128,14 @@ extern "C" fn open_callback<T: DriverObject>(
>      raw_obj: *mut bindings::drm_gem_object,
>      raw_file: *mut bindings::drm_file,
>  ) -> core::ffi::c_int {
> +    // SAFETY: `raw_obj` is a valid pointer to a `struct drm_gem_object` with a valid `dev`.
> +    let dev: &drm::Device<T::Driver, drm::Userspace> =
> +        unsafe { drm::Device::from_raw((*raw_obj).dev) };
> +
> +    let Some(_guard) = dev.registration_guard() else {

[Severity: High]
Could this cause an AB-BA deadlock with the device unplug path?

If an unbind thread acquires a driver-specific lock and then calls
drm_dev_unplug(), it will block in synchronize_srcu(&drm_unplug_srcu)
waiting for SRCU read sections to finish.

Concurrently, if a userspace thread opens a GEM handle, this
registration_guard() call takes the drm_unplug_srcu read lock.

When the callback proceeds to call T::open(), the driver might try to
acquire the same driver-specific lock held by the unbind thread, deadlocking
the system.

> +        return ENODEV.to_errno();
> +    };
> +
>      // SAFETY: `open_callback` is only ever called with a valid pointer to a `struct drm_file`.
>      let file = unsafe { DriverFile::<T>::from_raw(raw_file) };

[ ... ]

> @@ -148,6 +156,14 @@ extern "C" fn close_callback<T: DriverObject>(
>      raw_obj: *mut bindings::drm_gem_object,
>      raw_file: *mut bindings::drm_file,
>  ) {
> +    // SAFETY: `raw_obj` is a valid pointer to a `struct drm_gem_object` with a valid `dev`.
> +    let dev: &drm::Device<T::Driver, drm::Userspace> =
> +        unsafe { drm::Device::from_raw((*raw_obj).dev) };
> +
> +    let Some(_guard) = dev.registration_guard() else {
> +        return;

[Severity: High]
Is it safe to return early here without calling T::close()?

If a GEM object is created while the device is bound, and the device is then
hot-unplugged, the guard check will fail.

Returning early skips the software cleanup in T::close(), which could
permanently leak driver-specific memory and state (like tracking lists or IDR
entries) allocated during T::open().

> +    };
> +
>      // SAFETY: `open_callback` is only ever called with a valid pointer to a `struct drm_file`.
>      let file = unsafe { DriverFile::<T>::from_raw(raw_file) };
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2