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
