It was pointed out during patch review that if we fail to initialize the driver's private data in drm::device::Device::new(), we end up calling drm_dev_put(). This would call down to release(), which calls core::ptr::drop_in_place() on the device, which would result in releasing currently uninitialized private driver data.
So, fix this by just keeping track of when the private driver data is initialized or not and sticking it in a MaybeUninit. Signed-off-by: Lyude Paul <[email protected]> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction") Cc: <[email protected]> # v6.16+ --- rust/kernel/drm/device.rs | 53 +++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs index 629ef0bd1188e..38ae8de0af5d6 100644 --- a/rust/kernel/drm/device.rs +++ b/rust/kernel/drm/device.rs @@ -22,12 +22,14 @@ }; use core::{ alloc::Layout, - mem, - ops::Deref, - ptr::{ + cell::UnsafeCell, + mem::{ self, - NonNull, // + MaybeUninit, // }, + ops::Deref, + ptr::NonNull, + sync::atomic::*, }; #[cfg(CONFIG_DRM_LEGACY)] @@ -71,7 +73,14 @@ macro_rules! drm_legacy_fields { #[repr(C)] pub struct Device<T: drm::Driver> { dev: Opaque<bindings::drm_device>, - data: T::Data, + + /// Keeps track of whether we've initialized the device data yet. + pub(super) data_is_init: AtomicBool, + + /// The Driver's private data. + /// + /// This must only be written to from [`Device::new`]. + pub(super) data: UnsafeCell<MaybeUninit<T::Data>>, } impl<T: drm::Driver> Device<T> { @@ -128,8 +137,13 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A .cast(); let raw_drm = NonNull::new(from_err_ptr(raw_drm)?).ok_or(ENOMEM)?; + // Extract *mut MaybeUninit<T::Data> from UnsafeCell<MaybeUninit<T::Data>> // SAFETY: `raw_drm` is a valid pointer to `Self`. - let raw_data = unsafe { ptr::addr_of_mut!((*raw_drm.as_ptr()).data) }; + let raw_data = unsafe { (*(raw_drm.as_ptr())).data.get() }; + + // Extract *mut T::Data from *mut MaybeUninit<T::Data> + // SAFETY: `raw_data` is derived from `raw_drm` which is a valid pointer to `Self`. + let raw_data = unsafe { (*raw_data).as_mut_ptr() }; // SAFETY: // - `raw_data` is a valid pointer to uninitialized memory. @@ -144,6 +158,14 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A unsafe { bindings::drm_dev_put(drm_dev) }; })?; + // SAFETY: We just initialized raw_drm above using __drm_dev_alloc(), ensuring it is safe to + // dereference + unsafe { + (*raw_drm.as_ptr()) + .data_is_init + .store(true, Ordering::Relaxed) + }; + // SAFETY: The reference count is one, and now we take ownership of that reference as a // `drm::Device`. Ok(unsafe { ARef::from_raw(raw_drm) }) @@ -195,6 +217,22 @@ extern "C" fn release(ptr: *mut bindings::drm_device) { // SAFETY: `ptr` is a valid pointer to a `struct drm_device` and embedded in `Self`. let this = unsafe { Self::from_drm_device(ptr) }; + // SAFETY: + // - Since we are in release(), we are guaranteed that no one else has access to `this`. + // - We confirmed above that `this` is a valid pointer to an initialized `Self`. + let is_init = unsafe { &*this }.data_is_init.load(Ordering::Relaxed); + if is_init { + // SAFETY: + // - We confirmed we have unique access to this above. + // - We confirmed that `data` is initialized above. + let data_ptr = unsafe { &mut (*this).data }; + + // SAFETY: + // - We checked that the data is initialized above. + // - We do not use `data` any point after calling this function. + unsafe { data_ptr.get_mut().assume_init_drop() }; + } + // SAFETY: // - When `release` runs it is guaranteed that there is no further access to `this`. // - `this` is valid for dropping. @@ -206,7 +244,8 @@ impl<T: drm::Driver> Deref for Device<T> { type Target = T::Data; fn deref(&self) -> &Self::Target { - &self.data + // SAFETY: `data` is only written to once in `Device::new()`, so this read will never race. + unsafe { (&*self.data.get()).assume_init_ref() } } } -- 2.53.0
