Re: [PATCH v3 10/13] rust: drm: Add RegistrationData to drm::Driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `Sync` and `Send` bounds on `RegistrationData` allows safe code to introduce data races when concurrently accessed via `Device` or `Registration`.
- [High] `registration_guard()` can be called on an `UnregisteredDevice`, allowing access to an uninitialized (dangling) `registration_data` pointer.
--

commit 44df82d554a5a6b1d866d87bea0b20d5dd954c66
Author: Danilo Krummrich <dakr@kernel.org>

rust: drm: Add RegistrationData to drm::Driver

This commit adds a RegistrationData associated type to drm::Driver. It is
tied to the parent bus device binding scope and stored in drm::Device before
registration. Access is provided via Device::registration_data_with().

> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index bc8cfd753596e..0a2ee308b1b2a 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs

[ ... ]

> @@ -230,6 +232,9 @@ pub fn new(
>          // SAFETY: `drm_dev` is still private to this function.
>          unsafe { (*drm_dev).driver = const { &Self::VTABLE } };
>  
> +        // SAFETY: `raw_drm` is valid; no concurrent access before registration.
> +        unsafe { (*raw_drm.as_ptr()).registration_data = UnsafeCell::new(NonNull::dangling()) };

[Severity: High]
Could a caller safely dereference this dangling pointer by calling
registration_guard() before the device is actually registered?

Since UnregisteredDevice dereferences to Device, a caller can invoke
registration_guard() before registration. The C function drm_dev_enter()
only checks if the device is unplugged, not if it has been registered, so it
will return true and grant a RegistrationGuard prematurely.

If safe code then calls registration_data_with() on that guard, it seems it
would result in a dangling pointer dereference here:

    unsafe { (*self.registration_data.get()).cast::<_>().as_ref() }

> +
>          // SAFETY: The reference count is one, and now we take ownership of that reference as a
>          // `drm::Device`.
>          // INVARIANT: We just created the device above, but have yet to call `drm_dev_register`.

[ ... ]

> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index ceb2829985c78..987d116e3501e 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs

[ ... ]

> @@ -110,6 +110,16 @@ pub trait Driver {
>      /// Context data associated with the DRM driver
>      type Data: Sync + Send;
>  
> +    /// Data owned by the [`Registration`] and accessible within a
> +    /// [`RegistrationGuard`](drm::RegistrationGuard) critical section via
> +    /// [`Device::registration_data_with()`](drm::Device::registration_data_with).
> +    ///
> +    /// This is a [`ForLt`](trait@ForLt) type whose lifetime is tied to the parent bus
> +    /// device binding scope. References handed out by
> +    /// [`Device::registration_data_with()`](drm::Device::registration_data_with) are tied to
> +    /// the closure scope.
> +    type RegistrationData: ForLt;

[Severity: High]
Does this type definition need Sync and Send bounds to prevent data races?

Since drm::Device unconditionally implements Sync, references to the
device can be shared across multiple threads. The new registration_data_with()
method allows these threads to concurrently obtain shared references to
RegistrationData.

If a driver author provides a type that is not thread-safe (such as Cell)
for RegistrationData in safe Rust, multiple threads could concurrently access
and mutate the inner data, potentially leading to data races.

> +
>      /// The type used to manage memory for this driver.
>      type Object: AllocImpl;
>

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