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