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 <[email protected]> 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/[email protected]?part=10
