Re: [PATCH 2/5] rust: types: avoid `as` casts, narrow unsafe scope

From: Andreas Hindborg
Date: Thu Oct 31 2024 - 04:51:25 EST


Hi Tamir,

"Tamir Duberstein" <tamird@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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