Re: [PATCH v4 10/16] rust: drm: Add RegistrationGuard for drm_dev_enter/exit critical sections

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RegistrationGuard fails to protect driver unbind/teardown from concurrent IOCTLs because drm_dev_unplug() runs too late.
--

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

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

This commit introduces RegistrationGuard, which uses drm_dev_enter() and
drm_dev_exit() to represent an SRCU critical section ensuring the parent bus
device remains bound. It updates Registration::drop() to use
drm_dev_unplug() instead of drm_dev_unregister() to provide the necessary
SRCU barrier.

> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index d429b4655449f..c32cc0f0eba03 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs

[ ... ]

> +/// 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> {

[Severity: High]
Does this invariant hold true during the entire driver unbind lifecycle?

Since drm_dev_unplug() is what prevents new drm_dev_enter() critical
sections, the safety of this guard relies on exactly when drm_dev_unplug()
is called.

> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index 5152a18a8312b..3cda8dceb498d 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs

[ ... ]

>  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: High]
Does placing drm_dev_unplug() in the Drop implementation of Registration
leave a window where concurrent IOCTLs can access hardware during driver
unbind?

In the driver model, Registration is stored in the driver's private data,
which is dropped during devres teardown or via post_unbind_callback. This
happens strictly after the driver core invokes the driver's unbind() or
remove() callback.

Since the unbind() callback is the documented place for drivers to perform
graceful teardown and hardware I/O, does this mean drm_dev_enter() will
still return true during and immediately after unbind()?

If so, concurrent IOCTLs could successfully acquire RegistrationGuard and
attempt to access the hardware while it is actively being torn down, which
seems to violate the safety invariant and risk a hardware hang or
use-after-free.

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