Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Lock inversion in Registration::drop() when freeing file data under filelist_mutex - [Low] Missing #[inline] annotations on small abstraction functions --
--- Patch [6]: [PATCH 6/7] rust: drm: make Driver::File lifetime-parameterized --- commit f0644f05187ec5973de59ac9760eaeeee5140b6a Author: Danilo Krummrich <[email protected]> rust: drm: make Driver::File lifetime-parameterized Make the DRM file private data lifetime-parameterized, allowing drivers to borrow from RegistrationData in their per-file data. > diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs > --- a/rust/kernel/drm/driver.rs > +++ b/rust/kernel/drm/driver.rs [ ... ] > @@ -221,6 +237,51 @@ fn drop(&mut self) { > unsafe { bindings::drm_dev_unplug(self.drm.as_raw()) }; > // After drm_dev_unplug(), the SRCU barrier guarantees that all > RegistrationGuard critical > // sections have completed, so no one holds a reference to reg_data > anymore. > - // reg_data is dropped here automatically. > + > + // Drop all remaining file private data before dropping registration > data. This guarantees > + // that file data (which may borrow from RegistrationData) is always > dropped first. > + let raw = self.drm.as_raw(); > + > + // SAFETY: `filelist_mutex` is initialized by `drm_dev_init()` and > remains valid for > + // the lifetime of the `struct drm_device`. > + let filelist_mutex = unsafe { Mutex::from_raw(&raw mut > (*raw).filelist_mutex) }; > + { > + let _guard = filelist_mutex.lock(); > + > + // SAFETY: `filelist` is a valid, initialized sentinel > `list_head`; the mutex > + // guard prevents concurrent modification. > + let filelist = unsafe { > + clist_create!( > + &raw mut (*raw).filelist, > + drm::File<T>, > + bindings::drm_file, > + lhead > + ) > + }; > + > + for file in filelist.iter() { > + // SAFETY: `file` is a valid `drm_file` on this device's > filelist. > + let priv_ptr = unsafe { (*file.as_raw()).driver_priv }; > + > + // SAFETY: Setting `driver_priv` to NULL is visible to > `postclose_callback()` > + // through the `filelist_mutex` acquire/release chain in > `drm_close_helper()`. > + unsafe { (*file.as_raw()).driver_priv = > core::ptr::null_mut() }; > + > + // SAFETY: `driver_priv` was created by `open_callback()` > via `KBox::into_raw` and > + // has not yet been freed (the file is still in the list, so > `postclose_callback()` > + // has not run). > + drop(unsafe { KBox::from_raw(priv_ptr.cast::<<T::File as > ForLt>::Of<'static>>()) }); [Severity: High] Does dropping the file data while holding `filelist_mutex` introduce a lock inversion risk? If the driver's File Drop implementation acquires a driver-specific lock, this creates a `filelist_mutex` -> driver lock dependency. Since VFS operations routinely take driver locks before the DRM core takes `filelist_mutex` (e.g., in `drm_file_free`), this could violate core kernel locking rules and cause an AB-BA deadlock on device unplug. > + > + self.drm.open_count.fetch_sub(1, Release); > + } > + } > + > + // Wait for in-flight `postclose_callback()` calls to complete. > After `drm_dev_unplug()`, no > + // new opens can succeed, so `open_count` is monotonically > decreasing. > + self.drm > + .open_count_wq > + .wait_event(|| self.drm.open_count.load(Acquire) == 0); > + > + // `_reg_data` is dropped here automatically, after all file data > has been dropped. > } > } > diff --git a/rust/kernel/drm/file.rs b/rust/kernel/drm/file.rs > --- a/rust/kernel/drm/file.rs > +++ b/rust/kernel/drm/file.rs [ ... ] > @@ -45,59 +62,94 @@ pub(super) fn as_raw(&self) -> *mut bindings::drm_file { > self.0.get() > } > > - fn driver_priv(&self) -> *mut T { > - // SAFETY: By the type invariants of `Self`, `self.as_raw()` is > always valid. > - unsafe { (*self.as_raw()).driver_priv }.cast() > + /// Return a pinned reference to the driver file data. > + /// > + /// Only available when `D::File` implements [`trait@CovariantForLt`]. > For invariant types, use > + /// [`inner_with()`](Self::inner_with). > + pub fn inner(&self) -> Pin<&<D::File as ForLt>::Of<'_>> > + where > + D::File: CovariantForLt, [Severity: Low] This isn't a bug, but shouldn't this small accessor abstraction function be annotated with #[inline]? > + { > + // SAFETY: `driver_priv` was initialized by `open_callback()`. > `CovariantForLt` guarantees > + // the lifetime shortening from `'static` to `'_` is sound. > + unsafe { > Pin::new_unchecked(&*(*self.as_raw()).driver_priv.cast_const().cast()) } > } > > - /// Return a pinned reference to the driver file structure. > - pub fn inner(&self) -> Pin<&T> { > - // SAFETY: By the type invariant the pointer `self.as_raw()` points > to a valid and opened > - // `struct drm_file`, hence `driver_priv` has been properly > initialized by `open_callback`. > - unsafe { Pin::new_unchecked(&*(self.driver_priv())) } > + /// Access the driver file data through a closure. > + /// > + /// This works for all file data types, including invariant ones. For > covariant types, > + /// [`inner()`](Self::inner) provides direct access without a closure. > + pub fn inner_with<R, F>(&self, f: F) -> R > + where > + F: for<'a> FnOnce(Pin<&'a <D::File as ForLt>::Of<'a>>) -> R, [Severity: Low] This isn't a bug, but shouldn't this abstraction function also be annotated with #[inline]? > + { > + // SAFETY: `driver_priv` was initialized by `open_callback()`. The > HRTB `for<'a>` prevents > + // the caller from choosing a concrete lifetime, making the lifetime > shortening sound > + // regardless of variance. > + f(unsafe { > Pin::new_unchecked(&*(*self.as_raw()).driver_priv.cast_const().cast()) }) > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
