Message-Id: <20241030-borrow-mut-v1-2-8f0ceaf78eaf@gmail.com>
Date: Wed, 30 Oct 2024 16:46:39 -0400
From: Tamir Duberstein <tamird@...il.com>
To: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
 Benno Lossin <benno.lossin@...ton.me>, 
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, 
 Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
 Tamir Duberstein <tamird@...il.com>
Subject: [PATCH 2/5] rust: types: avoid `as` casts, narrow unsafe scope

Replace `as` casts with `cast{,_const,_mut}` which are a bit safer.

Reduce the scope of unsafe blocks and add missing safety comments where
an unsafe block has been split into several unsafe blocks.

Signed-off-by: Tamir Duberstein <tamird@...il.com>
---
 rust/kernel/alloc/kbox.rs | 32 +++++++++++++++----------
 rust/kernel/sync/arc.rs   | 59 +++++++++++++++++++++++++++++------------------
 rust/kernel/types.rs      |  5 ++--
 3 files changed, 59 insertions(+), 37 deletions(-)

diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index d69c32496b86a2315f81cafc8e6771ebb0cf10d1..7a5fdf7b660fb91ca2a8e5023d69d629b0d66062 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -182,12 +182,12 @@ impl<T, A> Box<MaybeUninit<T>, A>
     ///
     /// Callers must ensure that the value inside of `b` is in an initialized state.
     pub unsafe fn assume_init(self) -> Box<T, A> {
-        let raw = Self::into_raw(self);
+        let raw = Self::into_raw(self).cast();
 
         // SAFETY: `raw` comes from a previous call to `Box::into_raw`. By the safety requirements
         // of this function, the value inside the `Box` is in an initialized state. Hence, it is
         // safe to reconstruct the `Box` as `Box<T, A>`.
-        unsafe { Box::from_raw(raw.cast()) }
+        unsafe { Box::from_raw(raw) }
     }
 
     /// Writes the value and converts to `Box<T, A>`.
@@ -247,10 +247,10 @@ pub fn pin(x: T, flags: Flags) -> Result<Pin<Box<T, A>>, AllocError>
 
     /// Forgets the contents (does not run the destructor), but keeps the allocation.
     fn forget_contents(this: Self) -> Box<MaybeUninit<T>, A> {
-        let ptr = Self::into_raw(this);
+        let ptr = Self::into_raw(this).cast();
 
         // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
-        unsafe { Box::from_raw(ptr.cast()) }
+        unsafe { Box::from_raw(ptr) }
     }
 
     /// Drops the contents, but keeps the allocation.
@@ -356,19 +356,21 @@ impl<T: 'static, A> ForeignOwnable for Box<T, A>
     type Borrowed<'a> = &'a T;
 
     fn into_foreign(self) -> *const core::ffi::c_void {
-        Box::into_raw(self) as _
+        Box::into_raw(self).cast_const().cast()
     }
 
     unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
+        let ptr = ptr.cast_mut().cast();
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        unsafe { Box::from_raw(ptr as _) }
+        unsafe { Box::from_raw(ptr) }
     }
 
     unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T {
+        let ptr = ptr.cast();
         // SAFETY: The safety requirements of this method ensure that the object remains alive and
         // immutable for the duration of 'a.
-        unsafe { &*ptr.cast() }
+        unsafe { &*ptr }
     }
 }
 
@@ -380,21 +382,25 @@ impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
 
     fn into_foreign(self) -> *const core::ffi::c_void {
         // SAFETY: We are still treating the box as pinned.
-        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }) as _
+        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) })
+            .cast_const()
+            .cast()
     }
 
     unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
+        let ptr = ptr.cast_mut().cast();
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        unsafe { Pin::new_unchecked(Box::from_raw(ptr as _)) }
+        unsafe { Pin::new_unchecked(Box::from_raw(ptr)) }
     }
 
     unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Pin<&'a T> {
+        let ptr = ptr.cast();
         // SAFETY: The safety requirements for this function ensure that the object is still alive,
         // so it is safe to dereference the raw pointer.
         // The safety requirements of `from_foreign` also ensure that the object remains alive for
         // the lifetime of the returned value.
-        let r = unsafe { &*ptr.cast() };
+        let r = unsafe { &*ptr };
 
         // SAFETY: This pointer originates from a `Pin<Box<T>>`.
         unsafe { Pin::new_unchecked(r) }
@@ -445,12 +451,14 @@ impl<T, A> Drop for Box<T, A>
     fn drop(&mut self) {
         let layout = Layout::for_value::<T>(self);
 
+        let ptr = self.0.as_ptr();
         // SAFETY: The pointer in `self.0` is guaranteed to be valid by the type invariant.
-        unsafe { core::ptr::drop_in_place::<T>(self.deref_mut()) };
+        unsafe { core::ptr::drop_in_place(ptr) };
 
+        let addr = self.0.cast();
         // SAFETY:
         // - `self.0` was previously allocated with `A`.
         // - `layout` is equal to the `Layout´ `self.0` was allocated with.
-        unsafe { A::free(self.0.cast(), layout) };
+        unsafe { A::free(addr, layout) };
     }
 }
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 4857230bd8d410bcca97b2081c3ce2f617ee7921..88e5208369e40bdeebc6f758e89f836a97790d89 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -148,9 +148,10 @@ unsafe fn container_of(ptr: *const T) -> NonNull<ArcInner<T>> {
         let refcount_layout = Layout::new::<bindings::refcount_t>();
         // SAFETY: The caller guarantees that the pointer is valid.
         let val_layout = Layout::for_value(unsafe { &*ptr });
+        let val_offset = refcount_layout.extend(val_layout);
         // SAFETY: We're computing the layout of a real struct that existed when compiling this
         // binary, so its layout is not so large that it can trigger arithmetic overflow.
-        let val_offset = unsafe { refcount_layout.extend(val_layout).unwrap_unchecked().1 };
+        let (_, val_offset) = unsafe { val_offset.unwrap_unchecked() };
 
         // Pointer casts leave the metadata unchanged. This is okay because the metadata of `T` and
         // `ArcInner<T>` is the same since `ArcInner` is a struct with `T` as its last field.
@@ -164,9 +165,10 @@ unsafe fn container_of(ptr: *const T) -> NonNull<ArcInner<T>> {
         // still valid.
         let ptr = unsafe { ptr.byte_sub(val_offset) };
 
+        let ptr = ptr.cast_mut();
         // SAFETY: The pointer can't be null since you can't have an `ArcInner<T>` value at the null
         // address.
-        unsafe { NonNull::new_unchecked(ptr.cast_mut()) }
+        unsafe { NonNull::new_unchecked(ptr) }
     }
 }
 
@@ -201,10 +203,11 @@ pub fn new(contents: T, flags: Flags) -> Result<Self, AllocError> {
         };
 
         let inner = KBox::new(value, flags)?;
+        let inner = KBox::leak(inner).into();
 
         // SAFETY: We just created `inner` with a reference count of 1, which is owned by the new
         // `Arc` object.
-        Ok(unsafe { Self::from_inner(KBox::leak(inner).into()) })
+        Ok(unsafe { Self::from_inner(inner) })
     }
 }
 
@@ -333,13 +336,15 @@ impl<T: 'static> ForeignOwnable for Arc<T> {
     type Borrowed<'a> = ArcBorrow<'a, T>;
 
     fn into_foreign(self) -> *const core::ffi::c_void {
-        ManuallyDrop::new(self).ptr.as_ptr() as _
+        ManuallyDrop::new(self).ptr.as_ptr().cast_const().cast()
     }
 
     unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
+        let ptr = ptr.cast_mut().cast();
+
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        let inner = unsafe { NonNull::new_unchecked(ptr as _) };
+        let inner = unsafe { NonNull::new_unchecked(ptr) };
 
         // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
         // for the lifetime of the returned value.
@@ -347,9 +352,11 @@ unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
     }
 
     unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
+        let ptr = ptr.cast_mut().cast();
+
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        let inner = unsafe { NonNull::new_unchecked(ptr as _) };
+        let inner = unsafe { NonNull::new_unchecked(ptr) };
 
         // SAFETY: By the safety requirement of this function, we know that `ptr` came from
         // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and
@@ -376,10 +383,14 @@ fn as_ref(&self) -> &T {
 
 impl<T: ?Sized> Clone for Arc<T> {
     fn clone(&self) -> Self {
+        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
+        // safe to dereference it.
+        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
+
         // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot overflow to zero.
         // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
         // safe to increment the refcount.
-        unsafe { bindings::refcount_inc(self.ptr.as_ref().refcount.get()) };
+        unsafe { bindings::refcount_inc(refcount) };
 
         // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
         unsafe { Self::from_inner(self.ptr) }
@@ -399,10 +410,11 @@ fn drop(&mut self) {
         // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
         let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
         if is_zero {
+            let ptr = self.ptr.as_ptr();
             // The count reached zero, we must free the memory.
             //
             // SAFETY: The pointer was initialised from the result of `KBox::leak`.
-            unsafe { drop(KBox::from_raw(self.ptr.as_ptr())) };
+            unsafe { drop(KBox::from_raw(ptr)) };
         }
     }
 }
@@ -550,7 +562,7 @@ impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
     fn deref(&self) -> &Self::Target {
         // SAFETY: By the type invariant, the underlying object is still alive with no mutable
         // references to it, so it is safe to create a shared reference.
-        unsafe { &self.inner.as_ref().data }
+        &unsafe { self.inner.as_ref() }.data
     }
 }
 
@@ -652,11 +664,11 @@ pub fn new_uninit(flags: Flags) -> Result<UniqueArc<MaybeUninit<T>>, AllocError>
             }? AllocError),
             flags,
         )?;
-        Ok(UniqueArc {
-            // INVARIANT: The newly-created object has a refcount of 1.
-            // SAFETY: The pointer from the `KBox` is valid.
-            inner: unsafe { Arc::from_inner(KBox::leak(inner).into()) },
-        })
+        let inner = KBox::leak(inner).into();
+        // INVARIANT: The newly-created object has a refcount of 1.
+        // SAFETY: The pointer from the `KBox` is valid.
+        let inner = unsafe { Arc::from_inner(inner) };
+        Ok(UniqueArc { inner })
     }
 }
 
@@ -675,18 +687,18 @@ pub fn write(mut self, value: T) -> UniqueArc<T> {
     /// The caller guarantees that the value behind this pointer has been initialized. It is
     /// *immediate* UB to call this when the value is not initialized.
     pub unsafe fn assume_init(self) -> UniqueArc<T> {
-        let inner = ManuallyDrop::new(self).inner.ptr;
-        UniqueArc {
-            // SAFETY: The new `Arc` is taking over `ptr` from `self.inner` (which won't be
-            // dropped). The types are compatible because `MaybeUninit<T>` is compatible with `T`.
-            inner: unsafe { Arc::from_inner(inner.cast()) },
-        }
+        let inner = ManuallyDrop::new(self).inner.ptr.cast();
+        // SAFETY: The new `Arc` is taking over `ptr` from `self.inner` (which won't be
+        // dropped). The types are compatible because `MaybeUninit<T>` is compatible with `T`.
+        let inner = unsafe { Arc::from_inner(inner) };
+        UniqueArc { inner }
     }
 
     /// Initialize `self` using the given initializer.
     pub fn init_with<E>(mut self, init: impl Init<T, E>) -> core::result::Result<UniqueArc<T>, E> {
+        let ptr = self.as_mut_ptr();
         // SAFETY: The supplied pointer is valid for initialization.
-        match unsafe { init.__init(self.as_mut_ptr()) } {
+        match unsafe { init.__init(ptr) } {
             // SAFETY: Initialization completed successfully.
             Ok(()) => Ok(unsafe { self.assume_init() }),
             Err(err) => Err(err),
@@ -698,9 +710,10 @@ pub fn pin_init_with<E>(
         mut self,
         init: impl PinInit<T, E>,
     ) -> core::result::Result<Pin<UniqueArc<T>>, E> {
+        let ptr = self.as_mut_ptr();
         // SAFETY: The supplied pointer is valid for initialization and we will later pin the value
         // to ensure it does not move.
-        match unsafe { init.__pinned_init(self.as_mut_ptr()) } {
+        match unsafe { init.__pinned_init(ptr) } {
             // SAFETY: Initialization completed successfully.
             Ok(()) => Ok(unsafe { self.assume_init() }.into()),
             Err(err) => Err(err),
@@ -729,7 +742,7 @@ fn deref_mut(&mut self) -> &mut Self::Target {
         // SAFETY: By the `Arc` type invariant, there is necessarily a reference to the object, so
         // it is safe to dereference it. Additionally, we know there is only one reference when
         // it's inside a `UniqueArc`, so it is safe to get a mutable reference.
-        unsafe { &mut self.inner.ptr.as_mut().data }
+        &mut unsafe { self.inner.ptr.as_mut() }.data
     }
 }
 
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index fae80814fa1c5e0f11933f2f15e173f0e3a10fe0..e8b7ff1387381e50d7963978e57b1d567113b035 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -418,7 +418,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
     /// }
     ///
     /// let mut data = Empty {};
-    /// let ptr = NonNull::<Empty>::new(&mut data as *mut _).unwrap();
+    /// let ptr = NonNull::new(&mut data).unwrap();
     /// # // SAFETY: TODO.
     /// let data_ref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };
     /// let raw_ptr: NonNull<Empty> = ARef::into_raw(data_ref);
@@ -450,8 +450,9 @@ fn deref(&self) -> &Self::Target {
 impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
     fn from(b: &T) -> Self {
         b.inc_ref();
+        let b = b.into();
         // SAFETY: We just incremented the refcount above.
-        unsafe { Self::from_raw(NonNull::from(b)) }
+        unsafe { Self::from_raw(b) }
     }
 }
 

-- 
2.47.0

Powered by blists - more mailing lists