Message-ID: <87frocpsui.fsf@kernel.org>
Date: Thu, 31 Oct 2024 09:41:25 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Tamir Duberstein" <tamird@...il.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Boqun Feng" <boqun.feng@...il.com>,  "Gary Guo"
 <gary@...yguo.net>,  =?utf-8?Q?Bj=C3=B6rn?=
 Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Benno
 Lossin" <benno.lossin@...ton.me>,  "Alice Ryhl" <aliceryhl@...gle.com>,
  "Trevor Gross" <tmgross@...ch.edu>,  "Danilo Krummrich"
 <dakr@...nel.org>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] rust: types: avoid `as` casts, narrow unsafe scope

Hi Tamir,

"Tamir Duberstein" <tamird@...il.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@...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) }

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

Powered by blists - more mailing lists