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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] rust: drm: missing Send + Sync bounds on Driver::File and
  DriverFile
--

commit 229740f5d6f5f9093208d976d2ea98edc170b6f8
Author: Danilo Krummrich <dakr@kernel.org>

rust: drm: make Driver::File lifetime-parameterized

This commit lifetime-parameterizes Driver::File and DriverFile so per-file
driver data can borrow from RegistrationData, and drops open file data
before registration teardown completes.

> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index f6db91e81e794..92055c4f9e00c 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs
[ ... ]
> @@ -120,8 +132,14 @@ pub trait Driver {
>      /// The type used to manage memory for this driver.
>      type Object: AllocImpl;
>  
> -    /// The type used to represent a DRM File (client)
> -    type File: drm::file::DriverFile;
> +    /// 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 implement [`DriverFile`](drm::file::DriverFile) for their file data type.
> +    type File<'a>: 'a;
>  
>      /// The bus device type of the parent device that the DRM device is associated with.
>      type ParentDevice<Ctx: device::DeviceContext>: device::AsBusDevice<Ctx>;
[ ... ]
> diff --git a/rust/kernel/drm/file.rs b/rust/kernel/drm/file.rs
> index bd5e4ca2977fd..2dd0e61718fbd 100644
> --- a/rust/kernel/drm/file.rs
> +++ b/rust/kernel/drm/file.rs
> @@ -8,36 +8,66 @@
[ ... ]
>  /// Trait that must be implemented by DRM drivers to represent a DRM File (a client instance).
> -pub trait DriverFile {
> +///
> +/// The lifetime `'a` allows the file data to borrow from
> +/// [`RegistrationData`](drm::Driver::RegistrationData).
> +pub trait DriverFile<'a>: Sized {
>      /// The parent `Driver` implementation for this `DriverFile`.
>      type Driver: drm::Driver;
[ ... ]
> @@ -45,60 +75,110 @@ pub(super) fn as_raw(&self) -> *mut bindings::drm_file {
>          self.0.get()
>      }
>  
[ ... ]
> +    /// Access the driver file data through a closure.
> +    ///
> +    /// The closure receives a pinned reference to the file data.
> +    ///
> +    /// The outer reference carries the `&self` lifetime while the inner type carries the HRTB
> +    /// lifetime `'a`, implying `'a` outlives `&self`. This allows the closure to coerce covariant
> +    /// sub-fields (e.g. `&'a T` to the caller's lifetime) and return them directly in `R`.
> +    #[inline]
> +    pub fn inner_with<'this, R, F>(&'this self, f: F) -> R
> +    where
> +        F: for<'a> FnOnce(Pin<&'this D::File<'a>>) -> R,
> +    {
> +        // SAFETY: By the type invariants, `self.as_raw()` points to a valid `drm_file`.
> +        let ptr: *const FileData<_> = unsafe { (*self.as_raw()).driver_priv.cast_const().cast() };
> +
> +        // SAFETY:
> +        // - The type invariants keep this file's pinned `FileData` allocation valid for `'this`.
> +        // - The `data` field is structurally pinned.
> +        // - The HRTB `for<'a>` prevents the caller from choosing a concrete lifetime, so shortening
> +        //   is sound regardless of variance.
> +        let data: Pin<&'this D::File<'this>> = unsafe { Pin::new_unchecked(&(*ptr).data) };
> +        f(data)
> +    }

[Severity: Medium]
Should Driver::File<'a> and DriverFile<'a> require Send + Sync, like
Driver::Data and Driver::RegistrationData<'a>?

Without those bounds, a driver can store non-Sync or non-Send types such
as Cell or RefCell in its per-file data in safe Rust.  Multiple threads
sharing a file descriptor can invoke ioctls or GEM callbacks concurrently
on the same struct drm_file and obtain simultaneous shared references to
D::File<'_> through File::inner_with() (requiring Sync), while
File::postclose_callback() or Registration::drop_file_data() can drop the
FileData allocation on a different thread than the one that opened the
file (requiring Send), causing a data race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107021427.2105374-1-dakr@kernel.org?part=7