From 8dbee0a65e13b3da04d95f176ef73c4a62b8fd21 Mon Sep 17 00:00:00 2001 From: Ruihan Li Date: Sat, 15 Feb 2025 15:45:51 +0800 Subject: [PATCH] Revise the safety conditions for `OwnerPtr` Co-authored-by: Zhang Junyang --- ostd/src/sync/rcu/mod.rs | 32 +++++++++--- ostd/src/sync/rcu/owner_ptr.rs | 95 +++++++++++++++++----------------- 2 files changed, 73 insertions(+), 54 deletions(-) diff --git a/ostd/src/sync/rcu/mod.rs b/ostd/src/sync/rcu/mod.rs index e94f4f29f..ec7394fbf 100644 --- a/ostd/src/sync/rcu/mod.rs +++ b/ostd/src/sync/rcu/mod.rs @@ -124,7 +124,7 @@ unsafe impl Sync for Rcu where impl Rcu { /// Creates a new RCU cell with the given pointer. pub fn new(pointer: P) -> Self { - let ptr =

::into_raw(pointer).cast_mut(); + let ptr =

::into_raw(pointer).as_ptr(); let ptr = AtomicPtr::new(ptr); Self { ptr, @@ -162,7 +162,7 @@ impl Rcu { /// synchronized writes with locks. Otherwise, you can use [`Self::read`] /// and then [`RcuReadGuard::compare_exchange`] to update the pointer. pub fn update(&self, new_ptr: P) { - let new_ptr =

::into_raw(new_ptr).cast_mut(); + let new_ptr =

::into_raw(new_ptr).as_ptr(); let old_raw_ptr = self.ptr.swap(new_ptr, AcqRel); if let Some(p) = NonNull::new(old_raw_ptr) { @@ -245,15 +245,18 @@ impl RcuReadGuard<'_, P, NULLABLE> { /// This API does not help to avoid /// [the ABA problem](https://en.wikipedia.org/wiki/ABA_problem). pub fn compare_exchange(self, new_ptr: P) -> Result<(), P> { - let new_ptr =

::into_raw(new_ptr).cast_mut(); + let new_ptr =

::into_raw(new_ptr); if self .rcu .ptr - .compare_exchange(self.obj_ptr, new_ptr, AcqRel, Acquire) + .compare_exchange(self.obj_ptr, new_ptr.as_ptr(), AcqRel, Acquire) .is_err() { - // SAFETY: It was previously returned by `into_raw`. + // SAFETY: + // 1. It was previously returned by `into_raw`. + // 2. The `compare_exchange` fails so the pointer will not + // be used anymore. return Err(unsafe {

::from_raw(new_ptr) }); } @@ -271,10 +274,25 @@ impl RcuReadGuard<'_, P, NULLABLE> { /// The pointer must be previously returned by `into_raw` and the pointer /// must be only be dropped once. unsafe fn delay_drop(pointer: NonNull<

::Target>) { - // SAFETY: The pointer is not NULL. - let p = unsafe {

::from_raw(pointer.as_ptr().cast_const()) }; + struct ForceSend(NonNull<

::Target>); + // SAFETY: Sending a raw pointer to another task is safe as long as + // the pointer access in another task is safe (guaranteed by the trait + // bound `P: Send`). + unsafe impl Send for ForceSend

{} + + let pointer: ForceSend

= ForceSend(pointer); + let rcu_monitor = RCU_MONITOR.get().unwrap(); rcu_monitor.after_grace_period(move || { + // This is necessary to make the Rust compiler to move the entire + // `ForceSend` structure into the closure. + let pointer = pointer; + + // SAFETY: + // 1. The pointer was previously returned by `into_raw`. + // 2. The pointer won't be used anymore since the grace period has + // finished and this is the only time the pointer gets dropped. + let p = unsafe {

::from_raw(pointer.0) }; drop(p); }); } diff --git a/ostd/src/sync/rcu/owner_ptr.rs b/ostd/src/sync/rcu/owner_ptr.rs index 5439d9e91..7d36c1817 100644 --- a/ostd/src/sync/rcu/owner_ptr.rs +++ b/ostd/src/sync/rcu/owner_ptr.rs @@ -1,14 +1,23 @@ // SPDX-License-Identifier: MPL-2.0 +use core::ptr::NonNull; + use crate::prelude::*; /// A trait that abstracts pointers that have the ownership of the objects they /// refer to. /// -/// The most typical examples smart pointer types like `Box` and `Arc`. -/// +/// The most typical examples smart pointer types like `Box` and `Arc`, /// which can be converted to and from the raw pointer type of `*const T`. -pub trait OwnerPtr: 'static { +/// +/// # Safety +/// +/// This trait must be implemented correctly (according to the doc comments for +/// each method). Types like [`Rcu`] rely on this assumption to safely use the +/// raw pointers. +/// +/// [`Rcu`]: super::Rcu +pub unsafe trait OwnerPtr: 'static { /// The target type that this pointer refers to. // TODO: allow ?Sized type Target; @@ -18,76 +27,68 @@ pub trait OwnerPtr: 'static { /// Converts to a raw pointer. /// - /// If `Self` owns the object that it refers to (e.g., `Box<_>`), then - /// each call to `into_raw` must be paired with a call to `from_raw` + /// Each call to `into_raw` must be paired with a call to `from_raw` /// in order to avoid memory leakage. - fn into_raw(self) -> *const Self::Target; + /// + /// The resulting raw pointer must be valid to be immutably accessed + /// or borrowed until `from_raw` is called. + fn into_raw(self) -> NonNull; /// Converts back from a raw pointer. /// /// # Safety /// - /// The raw pointer must have been previously returned by a call to `into_raw`. - unsafe fn from_raw(ptr: *const Self::Target) -> Self; + /// 1. The raw pointer must have been previously returned by a call to + /// `into_raw`. + /// 2. The raw pointer must not be used after calling `from_raw`. + /// + /// Note that the second point is a hard requirement: Even if the + /// resulting value has not (yet) been dropped, the pointer cannot be + /// used because it may break Rust aliasing rules (e.g., `Box` + /// requires the pointer to be unique and thus _never_ aliased). + unsafe fn from_raw(ptr: NonNull) -> Self; } -impl OwnerPtr for Box { +unsafe impl OwnerPtr for Box { type Target = T; fn new(value: Self::Target) -> Self { Box::new(value) } - fn into_raw(self) -> *const Self::Target { - Box::into_raw(self) as *const _ + fn into_raw(self) -> NonNull { + let ptr = Box::into_raw(self); + + // SAFETY: The pointer representing a `Box` can never be NULL. + unsafe { NonNull::new_unchecked(ptr) } } - unsafe fn from_raw(ptr: *const Self::Target) -> Self { - Box::from_raw(ptr as *mut _) + unsafe fn from_raw(ptr: NonNull) -> Self { + let ptr = ptr.as_ptr(); + + // SAFETY: The safety is upheld by the caller. + unsafe { Box::from_raw(ptr) } } } -impl OwnerPtr for Arc { +unsafe impl OwnerPtr for Arc { type Target = T; fn new(value: Self::Target) -> Self { Arc::new(value) } - fn into_raw(self) -> *const Self::Target { - Arc::into_raw(self) + fn into_raw(self) -> NonNull { + let ptr = Arc::into_raw(self).cast_mut(); + + // SAFETY: The pointer representing an `Arc` can never be NULL. + unsafe { NonNull::new_unchecked(ptr) } } - unsafe fn from_raw(ptr: *const Self::Target) -> Self { - Arc::from_raw(ptr) - } -} - -impl

OwnerPtr for Option

-where - P: OwnerPtr, - // We cannot support fat pointers, e.g., when `Target: dyn Trait`. - // This is because Rust does not allow fat null pointers. Yet, - // we need the null pointer to represent `None`. - // See https://github.com/rust-lang/rust/issues/66316. -

::Target: Sized, -{ - type Target = P::Target; - - fn new(value: Self::Target) -> Self { - Some(P::new(value)) - } - - fn into_raw(self) -> *const Self::Target { - self.map(|p|

::into_raw(p)) - .unwrap_or(core::ptr::null()) - } - - unsafe fn from_raw(ptr: *const Self::Target) -> Self { - if ptr.is_null() { - Some(

::from_raw(ptr)) - } else { - None - } + unsafe fn from_raw(ptr: NonNull) -> Self { + let ptr = ptr.as_ptr().cast_const(); + + // SAFETY: The safety is upheld by the caller. + unsafe { Arc::from_raw(ptr) } } }