Re: [PATCH v3 09/13] rust: drm: Add RegistrationGuard for drm_dev_enter/exit critical sections

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Device::registration_guard() grants a `Registered` context to unregistered devices, allowing access to unbound hardware.
- [High] RegistrationGuard is implicitly `Send`, causing lockdep splats if dropped on a different thread.
- [Medium] Safe Rust can cause an SRCU deadlock by dropping `Registration` while holding `RegistrationGuard`.
--

commit 3b6c73976ebe0a7066664f9ddc910e85fe1582da
Author: Danilo Krummrich <dakr@kernel.org>

rust: drm: Add RegistrationGuard for drm_dev_enter/exit critical sections

This patch adds RegistrationGuard, a guard object representing a 
drm_dev_enter/exit SRCU critical section. It also changes 
Registration::drop to use drm_dev_unplug() instead of 
drm_dev_unregister() to provide the SRCU barrier required for safety.

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

[ ... ]

> @@ -323,6 +321,62 @@ pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC
>      }
>  }
>  
> +impl<T: drm::Driver> Device<T> {
> +    /// Guard against the parent bus device being unbound.
> +    ///
> +    /// Returns a [`RegistrationGuard`] if the device has not been unplugged, [`None`] otherwise.
> +    ///
> +    /// While [`RegistrationGuard`] is held the parent device is guaranteed to be bound.
> +    #[must_use]
> +    pub fn registration_guard(&self) -> Option<RegistrationGuard<'_, T>> {
> +        let mut idx: i32 = 0;
> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct drm_device`.
> +        if unsafe { bindings::drm_dev_enter(self.as_raw(), &mut idx) } {

[Severity: High]
Does this logic correctly prove the device is registered?

Because dev->unplugged defaults to false upon allocation, drm_dev_enter()
will return true for an UnregisteredDevice that has not yet called
drm_dev_register(). 

If device probe fails, the kernel unwinds devres-managed resources but never
creates a Registration or calls drm_dev_unplug(). If an ARef<Device> is
kept alive by a background thread, registration_guard() will still succeed
and return a RegistrationGuard.

This guard dereferences to Device<T, Registered>, which falsely exposes the
parent bus device as bound. Could this lead to a Use-After-Free or invalid
memory access if the parent device's devres-managed hardware resources are
accessed after probe failure? 

Should the guard explicitly verify the registration state (e.g.,
dev->registered) after acquiring the SRCU lock?

> +            Some(RegistrationGuard { dev: self, idx })
> +        } else {
> +            None
> +        }
> +    }
> +}
> +
> +/// A guard proving the DRM device is registered and the parent bus device is bound.
> +///
> +/// The guard dereferences to [`Device<T, Registered>`], providing access to the DRM device with
> +/// the guarantee that the parent bus device is bound for the entire duration of the critical
> +/// section.
> +///
> +/// Internally this is backed by a `drm_dev_enter()` / `drm_dev_exit()` SRCU critical section.
> +///
> +/// # Invariants
> +///
> +/// - `idx` is the SRCU read lock index returned by a successful `drm_dev_enter()` call.
> +/// - The parent bus device of `dev` is bound for the lifetime of this guard.
> +#[must_use]
> +pub struct RegistrationGuard<'a, T: drm::Driver> {
> +    dev: &'a Device<T>,
> +    idx: i32,
> +}

[Severity: High]
Is it intentional that RegistrationGuard is implicitly Send?

Because its inner fields are Send, this allows safe Rust to acquire the
guard on one thread and drop it on another.

While SRCU technically allows cross-thread unlocks, the kernel's lockdep
implementation tracks SRCU read locks per-task. If a guard is dropped on a
different thread, will lockdep emit a bad unlock balance detected splat?

Should RegistrationGuard be explicitly marked !Send (e.g., by adding a
PhantomData<*mut ()> field)?

> +
> +impl<T: drm::Driver> Deref for RegistrationGuard<'_, T> {
> +    type Target = Device<T, Registered>;
> +
> +    #[inline]
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: A successful `drm_dev_enter()` guarantees that the device is registered and
> +        // the parent bus device is bound.
> +        unsafe { self.dev.assume_ctx() }

[Severity: High]
Does a successful drm_dev_enter() alone guarantee the Registered state?

As mentioned above, if drm_dev_enter() succeeds on an UnregisteredDevice
after probe failure, this unsafe block will grant the Registered state
incorrectly.

> +    }
> +}

[ ... ]

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

[ ... ]

> @@ -199,8 +199,14 @@ unsafe impl<T: Driver> Send for Registration<T> {}
>  
>  impl<T: Driver> Drop for Registration<T> {
>      fn drop(&mut self) {
> +        // Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure that existing
> +        // `drm_dev_enter()` critical sections complete before unregistration proceeds. This
> +        // is required for the safety of `RegistrationGuard`, which relies on the SRCU barrier in
> +        // `drm_dev_unplug()` to guarantee that the parent device is still bound within the
> +        // critical section.
> +        //
>          // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this
> -        // `Registration` also guarantees the this `drm::Device` is actually registered.
> -        unsafe { bindings::drm_dev_unregister(self.0.as_raw()) };
> +        // `Registration` also guarantees that this `drm::Device` is actually registered.
> +        unsafe { bindings::drm_dev_unplug(self.0.as_raw()) };

[Severity: Medium]
Can dropping Registration while holding RegistrationGuard on the same
thread cause an SRCU deadlock?

drm_dev_unplug() synchronously waits for all SRCU readers by calling
synchronize_srcu(&drm_unplug_srcu). Since safe Rust can acquire an
independent reference to the device, obtain a RegistrationGuard (which
holds the SRCU read lock), and then drop Registration on the same thread,
synchronize_srcu() could be called from within its own read-side critical
section.

Would this deadlock the thread and trigger an SRCU lockdep warning?

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