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
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.