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
