Message-ID: <87cyjfnl8d.fsf@kernel.org>
Date: Fri, 01 Nov 2024 14:21:06 +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>,  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

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

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

Powered by blists - more mailing lists