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@gmail.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

Hi Tamir,

"Tamir Duberstein" <tamird@gmail.com> writes:

> 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@gmail.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) }

I don't think this change makes sense, and it also does not do what the
commit message says. The patch has quite a few changes of this pattern,
and I think you should drop those changes from the patch.

I _do_ think changing `as _` to `ptr::cast` makes sense.

>      }
>
>      /// 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()

But since we are at it, why not be more explicit and do `cast::<core::ffi:c_void>`?

<cut>

> @@ -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.

I think it could be "By the type invariant and the existence of `&self`,
it is safe to create a shared reference to the object pointed to by
`self.ptr`."

<cut>

> 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) }

I think this change makes the code _less_ readable.


Best regards,
Andreas

On Thu, Oct 31, 2024 at 4:51 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Hi Tamir,
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > 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@gmail.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) }
>
> I don't think this change makes sense, and it also does not do what the
> commit message says. The patch has quite a few changes of this pattern,
> and I think you should drop those changes from the patch.

It's reducing the scope of the unsafe block, as mentioned in the commit message.

> I _do_ think changing `as _` to `ptr::cast` makes sense.
>
> >      }
> >
> >      /// 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()
>
> But since we are at it, why not be more explicit and do `cast::<core::ffi:c_void>`?

These functions (`cast`, `cast_const`, and `cast_mut`) exist as a
compromise between fully inferred and fully explicit type coercion.
The doc comment on `cast_mut` explains:

/// This is a bit safer than as because it wouldn’t silently change
the type if the code is refactored. [0]

The inverse is true of `cast` - it won't silently change the constness
if the code is refactored.

The purpose of this patch is to demonstrate the number of places where
both type and constness casting is taking place, and to set up the
removal of costness casts in a subsequent patch.

>
> <cut>
>
> > @@ -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.
>
> I think it could be "By the type invariant and the existence of `&self`,
> it is safe to create a shared reference to the object pointed to by
> `self.ptr`."

This comment was taken from the `Deref` impl just above. Should it be
updated there as well? The comment is contained in the `Drop` impl as
well.

>
> <cut>
>
> > 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) }
>
> I think this change makes the code _less_ readable.
>
>
> Best regards,
> Andreas
>
>

Link: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.cast_mut
[0]

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Thu, Oct 31, 2024 at 4:51 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Hi Tamir,
>>
>> "Tamir Duberstein" <tamird@gmail.com> writes:
>>
>> > 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@gmail.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) }
>>
>> I don't think this change makes sense, and it also does not do what the
>> commit message says. The patch has quite a few changes of this pattern,
>> and I think you should drop those changes from the patch.
>
> It's reducing the scope of the unsafe block, as mentioned in the commit message.

I guess you are right. I read the commit message semantically as "split
unsafe blocks where there are multiple unsafe operations". I still do
not think it makes sense to do these code movements.

>
>> I _do_ think changing `as _` to `ptr::cast` makes sense.
>>
>> >      }
>> >
>> >      /// 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()
>>
>> But since we are at it, why not be more explicit and do `cast::<core::ffi:c_void>`?
>
> These functions (`cast`, `cast_const`, and `cast_mut`) exist as a
> compromise between fully inferred and fully explicit type coercion.
> The doc comment on `cast_mut` explains:
>
> /// This is a bit safer than as because it wouldn’t silently change
> the type if the code is refactored. [0]
>
> The inverse is true of `cast` - it won't silently change the constness
> if the code is refactored.
>
> The purpose of this patch is to demonstrate the number of places where
> both type and constness casting is taking place, and to set up the
> removal of costness casts in a subsequent patch.

I agree that `cast_const`, `cast_mut` and `cast` is an improvement to `as
_`. But I think we would get even more robustness if we went with fully
qualifying the generic parameter as in `.cast::<core::ffi::c_void>`.

>
>>
>> <cut>
>>
>> > @@ -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.
>>
>> I think it could be "By the type invariant and the existence of `&self`,
>> it is safe to create a shared reference to the object pointed to by
>> `self.ptr`."
>
> This comment was taken from the `Deref` impl just above. Should it be
> updated there as well? The comment is contained in the `Drop` impl as
> well.

I did not realize it was a copy.

I think the type invariant is actually not well formed:

/// # Invariants
///
/// The reference count on an instance of [`Arc`] is always non-zero.

There is not a reference count on an instance of `Arc`, there is a count
on `ArcInner`, of which `Arc` owns one.

What do you think?

I am OK with you copying the comment from `Deref`, but if you want to,
you could fix the wording of the invariant and update the comments.

In that case the safety comment could be something like my suggestion.
The fact that a reference to the `Arc` is live combined with the
invariant is what guarantees the pointee of `self.ptr` is live as well.

Best regards,
Andreas Hindborg

On Fri, Nov 1, 2024 at 9:21 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > On Thu, Oct 31, 2024 at 4:51 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> Hi Tamir,
> >>
> >> "Tamir Duberstein" <tamird@gmail.com> writes:
> >>
> >> > 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@gmail.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) }
> >>
> >> I don't think this change makes sense, and it also does not do what the
> >> commit message says. The patch has quite a few changes of this pattern,
> >> and I think you should drop those changes from the patch.
> >
> > It's reducing the scope of the unsafe block, as mentioned in the commit message.
>
> I guess you are right. I read the commit message semantically as "split
> unsafe blocks where there are multiple unsafe operations". I still do
> not think it makes sense to do these code movements.
>
> >
> >> I _do_ think changing `as _` to `ptr::cast` makes sense.
> >>
> >> >      }
> >> >
> >> >      /// 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()
> >>
> >> But since we are at it, why not be more explicit and do `cast::<core::ffi:c_void>`?
> >
> > These functions (`cast`, `cast_const`, and `cast_mut`) exist as a
> > compromise between fully inferred and fully explicit type coercion.
> > The doc comment on `cast_mut` explains:
> >
> > /// This is a bit safer than as because it wouldn’t silently change
> > the type if the code is refactored. [0]
> >
> > The inverse is true of `cast` - it won't silently change the constness
> > if the code is refactored.
> >
> > The purpose of this patch is to demonstrate the number of places where
> > both type and constness casting is taking place, and to set up the
> > removal of costness casts in a subsequent patch.
>
> I agree that `cast_const`, `cast_mut` and `cast` is an improvement to `as
> _`. But I think we would get even more robustness if we went with fully
> qualifying the generic parameter as in `.cast::<core::ffi::c_void>`.

I'd prefer not to make this series more controversial with that
change. Since we all agree that `as` casts are better avoided, I'll do
only that here.

> >
> >>
> >> <cut>
> >>
> >> > @@ -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.
> >>
> >> I think it could be "By the type invariant and the existence of `&self`,
> >> it is safe to create a shared reference to the object pointed to by
> >> `self.ptr`."
> >
> > This comment was taken from the `Deref` impl just above. Should it be
> > updated there as well? The comment is contained in the `Drop` impl as
> > well.
>
> I did not realize it was a copy.
>
> I think the type invariant is actually not well formed:
>
> /// # Invariants
> ///
> /// The reference count on an instance of [`Arc`] is always non-zero.
>
> There is not a reference count on an instance of `Arc`, there is a count
> on `ArcInner`, of which `Arc` owns one.
>
> What do you think?

I think this is picking nits. The next line is similarly "malformed":

/// The object pointed to by [`Arc`] is always pinned.

There's no object pointed to by Arc, it is pointed to by ArcInner.

> I am OK with you copying the comment from `Deref`, but if you want to,
> you could fix the wording of the invariant and update the comments.

It's not crystal clear to me what the difference between these
wordings is, so I'll just leave it as-is. Perhaps you could update all
the phrasing in a separate patch?

> In that case the safety comment could be something like my suggestion.
> The fact that a reference to the `Arc` is live combined with the
> invariant is what guarantees the pointee of `self.ptr` is live as well.
>
> Best regards,
> Andreas Hindborg

Cheers.
Tamir

On Wed, Oct 30, 2024 at 9:46 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> 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.

Reducing the scope of unsafe is good, but moving calls to "cast"
outside of the scope is excessive IMO.

Alice

On Thu, Oct 31, 2024 at 4:47 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Wed, Oct 30, 2024 at 9:46 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > 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.
>
> Reducing the scope of unsafe is good, but moving calls to "cast"
> outside of the scope is excessive IMO.

My intention here was to avoid applying subjective judgement by moving
out of unsafe everything that could be. I'll omit this from v2 since
it isn't clear where to draw the line.