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