Message-ID: <CAJ-ks9mrHuBEGvCi8pTNE=P1YHU81+cWF_emAvOKnsW0wQSWMQ@mail.gmail.com>
Date: Thu, 31 Oct 2024 07:50:26 -0400
From: Tamir Duberstein <tamird@...il.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: 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>, 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

On Thu, Oct 31, 2024 at 4:51 AM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
> 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.

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]

Powered by blists - more mailing lists