Message-ID: <CAJ-ks9n9g+QgH=YMkeenm5219sCAK5v8-gaOh46r=E9snbR2iQ@mail.gmail.com>
Date: Mon, 4 Nov 2024 17:19:42 -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 Fri, Nov 1, 2024 at 9:21 AM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
> "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>`.

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

Powered by blists - more mailing lists