> On 14 Aug 2026, at 20:21, [email protected] wrote:
>
> 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().
I wonder if this is a problem in practice? And if so, would passing a e.g.:
Option<&File> be a solution?
From a purely logical perspective, I do agree that a balanced open()/close()
makes more sense than silently skipping close() though.
>
>> + };
>> +
>> // 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
