Re: [PATCH 6/7] rust: drm: make Driver::File lifetime-parameterized

[sashiko-bot@kernel.org]

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 <dakr@kernel.org>

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/20260814230923.1292966-1-dakr@kernel.org?part=6