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

[Alice Ryhl <aliceryhl@google.com>]

On Sat, Aug 15, 2026 at 01:09:04AM +0200, Danilo Krummrich wrote:
> +    /// The type used to represent a DRM File (client).
> +    ///
> +    /// File data may borrow from [`RegistrationData`](Driver::RegistrationData). File data is
> +    /// guaranteed to be dropped before registration data, either when the file is closed or
> +    /// when the device is unregistered, whichever comes first.
> +    ///
> +    /// Drivers set this to `CovariantForLt!(MyFileData)` (or `ForLt!` for invariant types)
> +    /// and implement [`DriverFile`](drm::file::DriverFile) for their file data type.
> +    type File: ForLt + 'static;

This looks like it can be

	type File<'a>;

and if so, then I think it should be.

> @@ -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>>()) });
> +
> +                self.drm.open_count.fetch_sub(1, Release);
> +            }
> +        }

Using `drm::File<T>` in `clist_create!` here creates `&drm::File<T>`
references after `drm_dev_unplug()` has completed and while `driver_priv` is
being set to NULL and freed (so calling `file.inner()` inside the loop body
would dereference a null pointer).

Could we pass `Opaque<bindings::drm_file>` to `clist_create!` instead? That
way we never create a `&drm::File<T>` when its `driver_priv` is being torn
down.

Also, this drops files under the mutex. Could/should we avoid that for deadlock
avoidance?

> @@ -27,17 +44,17 @@ pub trait DriverFile {
>  /// `self.0` is a valid instance of a `struct drm_file`.
>  #[repr(transparent)]
> -pub struct File<T: DriverFile>(Opaque<bindings::drm_file>, PhantomData<T>);
> +pub struct File<D: drm::Driver>(Opaque<bindings::drm_file>, PhantomData<D>);
>  
> -impl<T: DriverFile> File<T> {
> +impl<D: drm::Driver> File<D> {
>      #[doc(hidden)]
>      /// Not intended to be called externally, except via declare_drm_ioctls!()
>      ///
>      /// # Safety
>      ///
> -    /// `raw_file` must be a valid pointer to an open `struct drm_file`, opened through `T::open`.
> -    pub unsafe fn from_raw<'a>(ptr: *mut bindings::drm_file) -> &'a File<T> {
> -        // SAFETY: `raw_file` is valid by the safety requirements of this function.
> +    /// `ptr` must be a valid pointer to an open `struct drm_file`.
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::drm_file) -> &'a File<D> {
> +        // SAFETY: `ptr` is valid by the safety requirements of this function.
>          unsafe { &*ptr.cast() }
>      }
>  
> @@ -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,
> +    {
> +        // 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()) }
>      }

Relatedly, since an open `struct drm_file` has `driver_priv` set to NULL (and
its `RegistrationData` dropped) after `Registration::drop()` runs, the type
invariants of `File<D>` and the safety requirements of `File::from_raw()`
should also state that:

- the `struct drm_file` was opened on a device of driver `D`, and
- a `RegistrationGuard` is held for the duration of any `&File<D>` reference
  (ensuring that `Registration::drop()` has not cleared `driver_priv` or
  dropped `RegistrationData`).

Alice