[PATCH 0/5] rust: add improved version of `ForeignOwnable::borrow_mut`

[PATCH 0/5] rust: add improved version of `ForeignOwnable::borrow_mut`

[Tamir Duberstein]

This is a re-submission of Alice's patch[0]. The leading commits are
intended to improve the consistency and ergonomics of `ForeignOwnable`,
and to split out the code movement originally included in the patch.

`ForeignOwnable::borrow_mut` is needed for `rnull`.

Link: https://lore.kernel.org/all/20230710074642.683831-1-aliceryhl@google.com/T/#u [0]

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Alice Ryhl (1):
      rust: add improved version of `ForeignOwnable::borrow_mut`

Tamir Duberstein (4):
      rust: arc: use `NonNull::new_unchecked`
      rust: types: avoid `as` casts, narrow unsafe scope
      rust: change `ForeignOwnable` pointer to mut
      rust: reorder `ForeignOwnable` items

 rust/kernel/alloc/kbox.rs | 63 ++++++++++++++++++++++---------
 rust/kernel/sync/arc.rs   | 86 +++++++++++++++++++++++++++---------------
 rust/kernel/types.rs      | 96 +++++++++++++++++++++++++++++++++++------------
 3 files changed, 171 insertions(+), 74 deletions(-)
---
base-commit: 718c4069896cabba5c39b637cbb7205927f16ae0
change-id: 20241030-borrow-mut-75f181feef4c

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

[PATCH 1/5] rust: arc: use `NonNull::new_unchecked`

[Tamir Duberstein]

There is no need to check (and panic on violations of) the safety
requirements on `ForeignOwnable` functions. Avoiding the check is
consistent with the implementation of `ForeignOwnable` for `Box`.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/sync/arc.rs | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index db9da352d588f65348aa7a5204abbb165b70197f..4857230bd8d410bcca97b2081c3ce2f617ee7921 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -337,9 +337,9 @@ fn into_foreign(self) -> *const core::ffi::c_void {
     }
 
     unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
-        // By the safety requirement of this function, we know that `ptr` came from
-        // a previous call to `Arc::into_foreign`.
-        let inner = NonNull::new(ptr as *mut ArcInner<T>).unwrap();
+        // 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 _) };
 
         // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
         // for the lifetime of the returned value.
@@ -347,10 +347,14 @@ unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
     }
 
     unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
+        // 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 _) };
+
         // 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
         // holds a reference count increment that is transferrable to us.
-        unsafe { Self::from_inner(NonNull::new(ptr as _).unwrap()) }
+        unsafe { Self::from_inner(inner) }
     }
 }
 

-- 
2.47.0

Re: [PATCH 1/5] rust: arc: use `NonNull::new_unchecked`

[Andreas Hindborg]

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

> There is no need to check (and panic on violations of) the safety
> requirements on `ForeignOwnable` functions. Avoiding the check is
> consistent with the implementation of `ForeignOwnable` for `Box`.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/sync/arc.rs | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index db9da352d588f65348aa7a5204abbb165b70197f..4857230bd8d410bcca97b2081c3ce2f617ee7921 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -337,9 +337,9 @@ fn into_foreign(self) -> *const core::ffi::c_void {
>      }
>
>      unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
> -        // By the safety requirement of this function, we know that `ptr` came from
> -        // a previous call to `Arc::into_foreign`.
> -        let inner = NonNull::new(ptr as *mut ArcInner<T>).unwrap();
> +        // 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 _) };

Please use an explicit cast.

>
>          // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
>          // for the lifetime of the returned value.
> @@ -347,10 +347,14 @@ unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
>      }
>
>      unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
> +        // 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 _) };

Please use an explicit cast.

> +
>          // 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
>          // holds a reference count increment that is transferrable to us.
> -        unsafe { Self::from_inner(NonNull::new(ptr as _).unwrap()) }
> +        unsafe { Self::from_inner(inner) }
>      }
>  }

Otherwise lgtm.

Best regards,
Andreas

Re: [PATCH 1/5] rust: arc: use `NonNull::new_unchecked`

[Tamir Duberstein]

On Thu, Oct 31, 2024 at 4:50 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > There is no need to check (and panic on violations of) the safety
> > requirements on `ForeignOwnable` functions. Avoiding the check is
> > consistent with the implementation of `ForeignOwnable` for `Box`.
> >
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/sync/arc.rs | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index db9da352d588f65348aa7a5204abbb165b70197f..4857230bd8d410bcca97b2081c3ce2f617ee7921 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -337,9 +337,9 @@ fn into_foreign(self) -> *const core::ffi::c_void {
> >      }
> >
> >      unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
> > -        // By the safety requirement of this function, we know that `ptr` came from
> > -        // a previous call to `Arc::into_foreign`.
> > -        let inner = NonNull::new(ptr as *mut ArcInner<T>).unwrap();
> > +        // 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 _) };
>
> Please use an explicit cast.

This changes to .cast() in a subsequent patch.

>
> >
> >          // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
> >          // for the lifetime of the returned value.
> > @@ -347,10 +347,14 @@ unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
> >      }
> >
> >      unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
> > +        // 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 _) };
>
> Please use an explicit cast.

This changes in a subsequent patch, here it is matching the prevailing
convention. I will restore the type ascription in v2.


>
> > +
> >          // 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
> >          // holds a reference count increment that is transferrable to us.
> > -        unsafe { Self::from_inner(NonNull::new(ptr as _).unwrap()) }
> > +        unsafe { Self::from_inner(inner) }
> >      }
> >  }
>
> Otherwise lgtm.
>
> Best regards,
> Andreas
>

Re: [PATCH 1/5] rust: arc: use `NonNull::new_unchecked`

[Alice Ryhl]

On Wed, Oct 30, 2024 at 9:46 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> There is no need to check (and panic on violations of) the safety
> requirements on `ForeignOwnable` functions. Avoiding the check is
> consistent with the implementation of `ForeignOwnable` for `Box`.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

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

[Tamir Duberstein]

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@gmail.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) }
     }
 
     /// 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()
     }
 
     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`.
-        unsafe { Box::from_raw(ptr as _) }
+        unsafe { Box::from_raw(ptr) }
     }
 
     unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T {
+        let ptr = ptr.cast();
         // SAFETY: The safety requirements of this method ensure that the object remains alive and
         // immutable for the duration of 'a.
-        unsafe { &*ptr.cast() }
+        unsafe { &*ptr }
     }
 }
 
@@ -380,21 +382,25 @@ impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
 
     fn into_foreign(self) -> *const core::ffi::c_void {
         // SAFETY: We are still treating the box as pinned.
-        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }) as _
+        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) })
+            .cast_const()
+            .cast()
     }
 
     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`.
-        unsafe { Pin::new_unchecked(Box::from_raw(ptr as _)) }
+        unsafe { Pin::new_unchecked(Box::from_raw(ptr)) }
     }
 
     unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Pin<&'a T> {
+        let ptr = ptr.cast();
         // SAFETY: The safety requirements for this function ensure that the object is still alive,
         // so it is safe to dereference the raw pointer.
         // The safety requirements of `from_foreign` also ensure that the object remains alive for
         // the lifetime of the returned value.
-        let r = unsafe { &*ptr.cast() };
+        let r = unsafe { &*ptr };
 
         // SAFETY: This pointer originates from a `Pin<Box<T>>`.
         unsafe { Pin::new_unchecked(r) }
@@ -445,12 +451,14 @@ impl<T, A> Drop for Box<T, A>
     fn drop(&mut self) {
         let layout = Layout::for_value::<T>(self);
 
+        let ptr = self.0.as_ptr();
         // SAFETY: The pointer in `self.0` is guaranteed to be valid by the type invariant.
-        unsafe { core::ptr::drop_in_place::<T>(self.deref_mut()) };
+        unsafe { core::ptr::drop_in_place(ptr) };
 
+        let addr = self.0.cast();
         // SAFETY:
         // - `self.0` was previously allocated with `A`.
         // - `layout` is equal to the `Layout´ `self.0` was allocated with.
-        unsafe { A::free(self.0.cast(), layout) };
+        unsafe { A::free(addr, layout) };
     }
 }
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 4857230bd8d410bcca97b2081c3ce2f617ee7921..88e5208369e40bdeebc6f758e89f836a97790d89 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -148,9 +148,10 @@ unsafe fn container_of(ptr: *const T) -> NonNull<ArcInner<T>> {
         let refcount_layout = Layout::new::<bindings::refcount_t>();
         // SAFETY: The caller guarantees that the pointer is valid.
         let val_layout = Layout::for_value(unsafe { &*ptr });
+        let val_offset = refcount_layout.extend(val_layout);
         // SAFETY: We're computing the layout of a real struct that existed when compiling this
         // binary, so its layout is not so large that it can trigger arithmetic overflow.
-        let val_offset = unsafe { refcount_layout.extend(val_layout).unwrap_unchecked().1 };
+        let (_, val_offset) = unsafe { val_offset.unwrap_unchecked() };
 
         // Pointer casts leave the metadata unchanged. This is okay because the metadata of `T` and
         // `ArcInner<T>` is the same since `ArcInner` is a struct with `T` as its last field.
@@ -164,9 +165,10 @@ unsafe fn container_of(ptr: *const T) -> NonNull<ArcInner<T>> {
         // still valid.
         let ptr = unsafe { ptr.byte_sub(val_offset) };
 
+        let ptr = ptr.cast_mut();
         // SAFETY: The pointer can't be null since you can't have an `ArcInner<T>` value at the null
         // address.
-        unsafe { NonNull::new_unchecked(ptr.cast_mut()) }
+        unsafe { NonNull::new_unchecked(ptr) }
     }
 }
 
@@ -201,10 +203,11 @@ pub fn new(contents: T, flags: Flags) -> Result<Self, AllocError> {
         };
 
         let inner = KBox::new(value, flags)?;
+        let inner = KBox::leak(inner).into();
 
         // SAFETY: We just created `inner` with a reference count of 1, which is owned by the new
         // `Arc` object.
-        Ok(unsafe { Self::from_inner(KBox::leak(inner).into()) })
+        Ok(unsafe { Self::from_inner(inner) })
     }
 }
 
@@ -333,13 +336,15 @@ impl<T: 'static> ForeignOwnable for Arc<T> {
     type Borrowed<'a> = ArcBorrow<'a, T>;
 
     fn into_foreign(self) -> *const core::ffi::c_void {
-        ManuallyDrop::new(self).ptr.as_ptr() as _
+        ManuallyDrop::new(self).ptr.as_ptr().cast_const().cast()
     }
 
     unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
+        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: The safety requirements of `from_foreign` ensure that the object remains alive
         // for the lifetime of the returned value.
@@ -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.
+        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
+
         // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot overflow to zero.
         // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
         // safe to increment the refcount.
-        unsafe { bindings::refcount_inc(self.ptr.as_ref().refcount.get()) };
+        unsafe { bindings::refcount_inc(refcount) };
 
         // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
         unsafe { Self::from_inner(self.ptr) }
@@ -399,10 +410,11 @@ fn drop(&mut self) {
         // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
         let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
         if is_zero {
+            let ptr = self.ptr.as_ptr();
             // The count reached zero, we must free the memory.
             //
             // SAFETY: The pointer was initialised from the result of `KBox::leak`.
-            unsafe { drop(KBox::from_raw(self.ptr.as_ptr())) };
+            unsafe { drop(KBox::from_raw(ptr)) };
         }
     }
 }
@@ -550,7 +562,7 @@ impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
     fn deref(&self) -> &Self::Target {
         // SAFETY: By the type invariant, the underlying object is still alive with no mutable
         // references to it, so it is safe to create a shared reference.
-        unsafe { &self.inner.as_ref().data }
+        &unsafe { self.inner.as_ref() }.data
     }
 }
 
@@ -652,11 +664,11 @@ pub fn new_uninit(flags: Flags) -> Result<UniqueArc<MaybeUninit<T>>, AllocError>
             }? AllocError),
             flags,
         )?;
-        Ok(UniqueArc {
-            // INVARIANT: The newly-created object has a refcount of 1.
-            // SAFETY: The pointer from the `KBox` is valid.
-            inner: unsafe { Arc::from_inner(KBox::leak(inner).into()) },
-        })
+        let inner = KBox::leak(inner).into();
+        // INVARIANT: The newly-created object has a refcount of 1.
+        // SAFETY: The pointer from the `KBox` is valid.
+        let inner = unsafe { Arc::from_inner(inner) };
+        Ok(UniqueArc { inner })
     }
 }
 
@@ -675,18 +687,18 @@ pub fn write(mut self, value: T) -> UniqueArc<T> {
     /// The caller guarantees that the value behind this pointer has been initialized. It is
     /// *immediate* UB to call this when the value is not initialized.
     pub unsafe fn assume_init(self) -> UniqueArc<T> {
-        let inner = ManuallyDrop::new(self).inner.ptr;
-        UniqueArc {
-            // SAFETY: The new `Arc` is taking over `ptr` from `self.inner` (which won't be
-            // dropped). The types are compatible because `MaybeUninit<T>` is compatible with `T`.
-            inner: unsafe { Arc::from_inner(inner.cast()) },
-        }
+        let inner = ManuallyDrop::new(self).inner.ptr.cast();
+        // SAFETY: The new `Arc` is taking over `ptr` from `self.inner` (which won't be
+        // dropped). The types are compatible because `MaybeUninit<T>` is compatible with `T`.
+        let inner = unsafe { Arc::from_inner(inner) };
+        UniqueArc { inner }
     }
 
     /// Initialize `self` using the given initializer.
     pub fn init_with<E>(mut self, init: impl Init<T, E>) -> core::result::Result<UniqueArc<T>, E> {
+        let ptr = self.as_mut_ptr();
         // SAFETY: The supplied pointer is valid for initialization.
-        match unsafe { init.__init(self.as_mut_ptr()) } {
+        match unsafe { init.__init(ptr) } {
             // SAFETY: Initialization completed successfully.
             Ok(()) => Ok(unsafe { self.assume_init() }),
             Err(err) => Err(err),
@@ -698,9 +710,10 @@ pub fn pin_init_with<E>(
         mut self,
         init: impl PinInit<T, E>,
     ) -> core::result::Result<Pin<UniqueArc<T>>, E> {
+        let ptr = self.as_mut_ptr();
         // SAFETY: The supplied pointer is valid for initialization and we will later pin the value
         // to ensure it does not move.
-        match unsafe { init.__pinned_init(self.as_mut_ptr()) } {
+        match unsafe { init.__pinned_init(ptr) } {
             // SAFETY: Initialization completed successfully.
             Ok(()) => Ok(unsafe { self.assume_init() }.into()),
             Err(err) => Err(err),
@@ -729,7 +742,7 @@ fn deref_mut(&mut self) -> &mut Self::Target {
         // SAFETY: By the `Arc` type invariant, there is necessarily a reference to the object, so
         // it is safe to dereference it. Additionally, we know there is only one reference when
         // it's inside a `UniqueArc`, so it is safe to get a mutable reference.
-        unsafe { &mut self.inner.ptr.as_mut().data }
+        &mut unsafe { self.inner.ptr.as_mut() }.data
     }
 }
 
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) }
     }
 }
 

-- 
2.47.0

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

[Andreas Hindborg]

Hi Tamir,

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

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

[Tamir Duberstein]

On Thu, Oct 31, 2024 at 4:51 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Hi Tamir,
>
> "Tamir Duberstein" <tamird@gmail.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@gmail.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]

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

[Andreas Hindborg]

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

> On Thu, Oct 31, 2024 at 4:51 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Hi Tamir,
>>
>> "Tamir Duberstein" <tamird@gmail.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@gmail.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

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

[Tamir Duberstein]

On Fri, Nov 1, 2024 at 9:21 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > On Thu, Oct 31, 2024 at 4:51 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> Hi Tamir,
> >>
> >> "Tamir Duberstein" <tamird@gmail.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@gmail.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

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

[Alice Ryhl]

On Wed, Oct 30, 2024 at 9:46 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> 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.

Reducing the scope of unsafe is good, but moving calls to "cast"
outside of the scope is excessive IMO.

Alice

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

[Tamir Duberstein]

On Thu, Oct 31, 2024 at 4:47 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Wed, Oct 30, 2024 at 9:46 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > 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.
>
> Reducing the scope of unsafe is good, but moving calls to "cast"
> outside of the scope is excessive IMO.

My intention here was to avoid applying subjective judgement by moving
out of unsafe everything that could be. I'll omit this from v2 since
it isn't clear where to draw the line.

[PATCH 3/5] rust: change `ForeignOwnable` pointer to mut

[Tamir Duberstein]

It is slightly more convenient to operate on mut pointers, and this also
properly conveys the desired ownership semantics of the trait.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/alloc/kbox.rs | 22 ++++++++++------------
 rust/kernel/sync/arc.rs   | 12 ++++++------
 rust/kernel/types.rs      | 14 +++++++-------
 3 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index 7a5fdf7b660fb91ca2a8e5023d69d629b0d66062..de7fadeb7fdf5cf6742c2e9749e959ac5f82359e 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -355,18 +355,18 @@ 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).cast_const().cast()
+    fn into_foreign(self) -> *mut core::ffi::c_void {
+        Box::into_raw(self).cast()
     }
 
-    unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
-        let ptr = ptr.cast_mut().cast();
+    unsafe fn from_foreign(ptr: *mut core::ffi::c_void) -> Self {
+        let ptr = ptr.cast();
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
         unsafe { Box::from_raw(ptr) }
     }
 
-    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T {
+    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> &'a T {
         let ptr = ptr.cast();
         // SAFETY: The safety requirements of this method ensure that the object remains alive and
         // immutable for the duration of 'a.
@@ -380,21 +380,19 @@ impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
 {
     type Borrowed<'a> = Pin<&'a T>;
 
-    fn into_foreign(self) -> *const core::ffi::c_void {
+    fn into_foreign(self) -> *mut core::ffi::c_void {
         // SAFETY: We are still treating the box as pinned.
-        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) })
-            .cast_const()
-            .cast()
+        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }).cast()
     }
 
-    unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
-        let ptr = ptr.cast_mut().cast();
+    unsafe fn from_foreign(ptr: *mut core::ffi::c_void) -> Self {
+        let ptr = ptr.cast();
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
         unsafe { Pin::new_unchecked(Box::from_raw(ptr)) }
     }
 
-    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Pin<&'a T> {
+    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> Pin<&'a T> {
         let ptr = ptr.cast();
         // SAFETY: The safety requirements for this function ensure that the object is still alive,
         // so it is safe to dereference the raw pointer.
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 88e5208369e40bdeebc6f758e89f836a97790d89..26b7272424aafdd4847d9642456cab837797ac33 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -335,12 +335,12 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
 impl<T: 'static> ForeignOwnable for Arc<T> {
     type Borrowed<'a> = ArcBorrow<'a, T>;
 
-    fn into_foreign(self) -> *const core::ffi::c_void {
-        ManuallyDrop::new(self).ptr.as_ptr().cast_const().cast()
+    fn into_foreign(self) -> *mut core::ffi::c_void {
+        ManuallyDrop::new(self).ptr.as_ptr().cast()
     }
 
-    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
-        let ptr = ptr.cast_mut().cast();
+    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> ArcBorrow<'a, T> {
+        let ptr = ptr.cast();
 
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
@@ -351,8 +351,8 @@ unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
         unsafe { ArcBorrow::new(inner) }
     }
 
-    unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
-        let ptr = ptr.cast_mut().cast();
+    unsafe fn from_foreign(ptr: *mut core::ffi::c_void) -> Self {
+        let ptr = ptr.cast();
 
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index e8b7ff1387381e50d7963978e57b1d567113b035..04358375794dc5ba7bfebbe3cfc5885cff531f15 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -29,7 +29,7 @@ pub trait ForeignOwnable: Sized {
     /// For example, it might be invalid, dangling or pointing to uninitialized memory. Using it in
     /// any way except for [`ForeignOwnable::from_foreign`], [`ForeignOwnable::borrow`],
     /// [`ForeignOwnable::try_from_foreign`] can result in undefined behavior.
-    fn into_foreign(self) -> *const core::ffi::c_void;
+    fn into_foreign(self) -> *mut core::ffi::c_void;
 
     /// Borrows a foreign-owned object.
     ///
@@ -37,7 +37,7 @@ pub trait ForeignOwnable: Sized {
     ///
     /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
     /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
-    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Self::Borrowed<'a>;
+    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> Self::Borrowed<'a>;
 
     /// Converts a foreign-owned object back to a Rust-owned one.
     ///
@@ -47,7 +47,7 @@ pub trait ForeignOwnable: Sized {
     /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
     /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for
     /// this object must have been dropped.
-    unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self;
+    unsafe fn from_foreign(ptr: *mut core::ffi::c_void) -> Self;
 
     /// Tries to convert a foreign-owned object back to a Rust-owned one.
     ///
@@ -58,7 +58,7 @@ pub trait ForeignOwnable: Sized {
     ///
     /// `ptr` must either be null or satisfy the safety requirements for
     /// [`ForeignOwnable::from_foreign`].
-    unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self> {
+    unsafe fn try_from_foreign(ptr: *mut core::ffi::c_void) -> Option<Self> {
         if ptr.is_null() {
             None
         } else {
@@ -72,13 +72,13 @@ unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self> {
 impl ForeignOwnable for () {
     type Borrowed<'a> = ();
 
-    fn into_foreign(self) -> *const core::ffi::c_void {
+    fn into_foreign(self) -> *mut core::ffi::c_void {
         core::ptr::NonNull::dangling().as_ptr()
     }
 
-    unsafe fn borrow<'a>(_: *const core::ffi::c_void) -> Self::Borrowed<'a> {}
+    unsafe fn borrow<'a>(_: *mut core::ffi::c_void) -> Self::Borrowed<'a> {}
 
-    unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {}
+    unsafe fn from_foreign(_: *mut core::ffi::c_void) -> Self {}
 }
 
 /// Runs a cleanup function/closure when dropped.

-- 
2.47.0

Re: [PATCH 3/5] rust: change `ForeignOwnable` pointer to mut

[Andreas Hindborg]

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

> It is slightly more convenient to operate on mut pointers, and this also
> properly conveys the desired ownership semantics of the trait.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>

Re: [PATCH 3/5] rust: change `ForeignOwnable` pointer to mut

[Alice Ryhl]

On Wed, Oct 30, 2024 at 9:46 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> It is slightly more convenient to operate on mut pointers, and this also
> properly conveys the desired ownership semantics of the trait.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH 4/5] rust: reorder `ForeignOwnable` items

[Tamir Duberstein]

`{into,from}_foreign` before `borrow` is slightly more logical.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/sync/arc.rs | 18 +++++++++---------
 rust/kernel/types.rs    | 20 ++++++++++----------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 26b7272424aafdd4847d9642456cab837797ac33..4552913c75f747d646bf408c1e8a1a883afb4b6a 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -339,29 +339,29 @@ fn into_foreign(self) -> *mut core::ffi::c_void {
         ManuallyDrop::new(self).ptr.as_ptr().cast()
     }
 
-    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> ArcBorrow<'a, T> {
+    unsafe fn from_foreign(ptr: *mut core::ffi::c_void) -> Self {
         let ptr = ptr.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) };
 
-        // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
-        // for the lifetime of the returned value.
-        unsafe { ArcBorrow::new(inner) }
+        // 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
+        // holds a reference count increment that is transferrable to us.
+        unsafe { Self::from_inner(inner) }
     }
 
-    unsafe fn from_foreign(ptr: *mut core::ffi::c_void) -> Self {
+    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> ArcBorrow<'a, T> {
         let ptr = ptr.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) };
 
-        // 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
-        // holds a reference count increment that is transferrable to us.
-        unsafe { Self::from_inner(inner) }
+        // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
+        // for the lifetime of the returned value.
+        unsafe { ArcBorrow::new(inner) }
     }
 }
 
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 04358375794dc5ba7bfebbe3cfc5885cff531f15..b8a7b2ee96a17081ad31e1bb73cb0513bcd05ef4 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -31,14 +31,6 @@ pub trait ForeignOwnable: Sized {
     /// [`ForeignOwnable::try_from_foreign`] can result in undefined behavior.
     fn into_foreign(self) -> *mut core::ffi::c_void;
 
-    /// Borrows a foreign-owned object.
-    ///
-    /// # Safety
-    ///
-    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
-    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
-    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> Self::Borrowed<'a>;
-
     /// Converts a foreign-owned object back to a Rust-owned one.
     ///
     /// # Safety
@@ -67,6 +59,14 @@ unsafe fn try_from_foreign(ptr: *mut core::ffi::c_void) -> Option<Self> {
             unsafe { Some(Self::from_foreign(ptr)) }
         }
     }
+
+    /// Borrows a foreign-owned object.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
+    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
+    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> Self::Borrowed<'a>;
 }
 
 impl ForeignOwnable for () {
@@ -76,9 +76,9 @@ fn into_foreign(self) -> *mut core::ffi::c_void {
         core::ptr::NonNull::dangling().as_ptr()
     }
 
-    unsafe fn borrow<'a>(_: *mut core::ffi::c_void) -> Self::Borrowed<'a> {}
-
     unsafe fn from_foreign(_: *mut core::ffi::c_void) -> Self {}
+
+    unsafe fn borrow<'a>(_: *mut core::ffi::c_void) -> Self::Borrowed<'a> {}
 }
 
 /// Runs a cleanup function/closure when dropped.

-- 
2.47.0

Re: [PATCH 4/5] rust: reorder `ForeignOwnable` items

[Andreas Hindborg]

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

> `{into,from}_foreign` before `borrow` is slightly more logical.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

I don't think this change is required, I would drop this patch.

Best regards,
Andreas

Re: [PATCH 4/5] rust: reorder `ForeignOwnable` items

[Tamir Duberstein]

On Thu, Oct 31, 2024 at 4:50 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > `{into,from}_foreign` before `borrow` is slightly more logical.
> >
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>
> I don't think this change is required, I would drop this patch.
>
> Best regards,
> Andreas
>

This change was part of the original patch. Do you prefer the code
movement in the same commit?

Re: [PATCH 4/5] rust: reorder `ForeignOwnable` items

[Miguel Ojeda]

On Thu, Oct 31, 2024 at 1:23 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> This change was part of the original patch. Do you prefer the code
> movement in the same commit?

If we do it, please keep it separate, that is a good idea.

However, I think Andreas means to avoid the movement at all,
regardless of which commit is used to do it.

Cheers,
Miguel

Re: [PATCH 4/5] rust: reorder `ForeignOwnable` items

[Tamir Duberstein]

On Thu, Oct 31, 2024 at 8:40 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, Oct 31, 2024 at 1:23 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > This change was part of the original patch. Do you prefer the code
> > movement in the same commit?
>
> If we do it, please keep it separate, that is a good idea.
>
> However, I think Andreas means to avoid the movement at all,
> regardless of which commit is used to do it.

Understood. I'll update the commit message to explain that this also
avoids inconsistency with the implementation in kbox.rs, which is
already in this order.

Andreas, please let me know if this is acceptable.

Thanks.
Tamir

Re: [PATCH 4/5] rust: reorder `ForeignOwnable` items

[Andreas Hindborg]

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

> On Thu, Oct 31, 2024 at 8:40 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> On Thu, Oct 31, 2024 at 1:23 PM Tamir Duberstein <tamird@gmail.com> wrote:
>> >
>> > This change was part of the original patch. Do you prefer the code
>> > movement in the same commit?
>>
>> If we do it, please keep it separate, that is a good idea.
>>
>> However, I think Andreas means to avoid the movement at all,
>> regardless of which commit is used to do it.
>
> Understood. I'll update the commit message to explain that this also
> avoids inconsistency with the implementation in kbox.rs, which is
> already in this order.
>
> Andreas, please let me know if this is acceptable.

I would prefer not to move the code, but I am not going to make more
noise about it that this :) I don't care what order these items are, and
so to me it is just noise in the history.


Best regards,
Andreas Hindborg

Re: [PATCH 4/5] rust: reorder `ForeignOwnable` items

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Thu, Oct 31, 2024 at 1:23 PM Tamir Duberstein <tamird@gmail.com> wrote:
>>
>> This change was part of the original patch. Do you prefer the code
>> movement in the same commit?
>
> If we do it, please keep it separate, that is a good idea.
>
> However, I think Andreas means to avoid the movement at all,
> regardless of which commit is used to do it.

Exactly.


Best regards,
Andreas Hindborg

[PATCH 5/5] rust: add improved version of `ForeignOwnable::borrow_mut`

[Tamir Duberstein]

From: Alice Ryhl <aliceryhl@google.com>

Previously, the `ForeignOwnable` trait had a method called `borrow_mut`
that was intended to provide mutable access to the inner value. However,
the method accidentally made it possible to change the address of the
object being modified, which usually isn't what we want. (And when we
want that, it can be done by calling `from_foreign` and `into_foreign`,
like how the old `borrow_mut` was implemented.)

In this patch, we introduce an alternate definition of `borrow_mut` that
solves the previous problem. Conceptually, given a pointer type `P` that
implements `ForeignOwnable`, the `borrow_mut` method gives you the same
kind of access as an `&mut P` would, except that it does not let you
change the pointer `P` itself.

This is analogous to how the existing `borrow` method provides the same
kind of access to the inner value as an `&P`.

Note that for types like `Arc`, having an `&mut Arc<T>` only gives you
immutable access to the inner `T`. This is because mutable references
assume exclusive access, but there might be other handles to the same
reference counted value, so the access isn't exclusive. The `Arc` type
implements this by making `borrow_mut` return the same type as `borrow`.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

---

Source: https://lore.kernel.org/all/20230710074642.683831-1-aliceryhl@google.com/
---
 rust/kernel/alloc/kbox.rs | 21 ++++++++++++++
 rust/kernel/sync/arc.rs   |  7 +++++
 rust/kernel/types.rs      | 71 ++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 86 insertions(+), 13 deletions(-)

diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index de7fadeb7fdf5cf6742c2e9749e959ac5f82359e..6ae81d4f74e9f7cf228385490f3a065082c1f4f6 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -354,6 +354,7 @@ impl<T: 'static, A> ForeignOwnable for Box<T, A>
     A: Allocator,
 {
     type Borrowed<'a> = &'a T;
+    type BorrowedMut<'a> = &'a mut T;
 
     fn into_foreign(self) -> *mut core::ffi::c_void {
         Box::into_raw(self).cast()
@@ -372,6 +373,13 @@ unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> &'a T {
         // immutable for the duration of 'a.
         unsafe { &*ptr }
     }
+
+    unsafe fn borrow_mut<'a>(ptr: *mut core::ffi::c_void) -> &'a mut T {
+        let ptr = ptr.cast();
+        // SAFETY: The safety requirements of this method ensure that the pointer is valid and that
+        // nothing else will access the value for the duration of 'a.
+        unsafe { &mut *ptr }
+    }
 }
 
 impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
@@ -379,6 +387,7 @@ impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
     A: Allocator,
 {
     type Borrowed<'a> = Pin<&'a T>;
+    type BorrowedMut<'a> = Pin<&'a mut T>;
 
     fn into_foreign(self) -> *mut core::ffi::c_void {
         // SAFETY: We are still treating the box as pinned.
@@ -403,6 +412,18 @@ unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> Pin<&'a T> {
         // SAFETY: This pointer originates from a `Pin<Box<T>>`.
         unsafe { Pin::new_unchecked(r) }
     }
+
+    unsafe fn borrow_mut<'a>(ptr: *mut core::ffi::c_void) -> Pin<&'a mut T> {
+        let ptr = ptr.cast();
+        // SAFETY: The safety requirements for this function ensure that the object is still alive,
+        // so it is safe to dereference the raw pointer.
+        // The safety requirements of `from_foreign` also ensure that the object remains alive for
+        // the lifetime of the returned value.
+        let r = unsafe { &mut *ptr };
+
+        // SAFETY: This pointer originates from a `Pin<Box<T>>`.
+        unsafe { Pin::new_unchecked(r) }
+    }
 }
 
 impl<T, A> Deref for Box<T, A>
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 4552913c75f747d646bf408c1e8a1a883afb4b6a..8b21197ceba3508bcac6f1b54d73244d9bdfd5a7 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -334,6 +334,7 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
 
 impl<T: 'static> ForeignOwnable for Arc<T> {
     type Borrowed<'a> = ArcBorrow<'a, T>;
+    type BorrowedMut<'a> = Self::Borrowed<'a>;
 
     fn into_foreign(self) -> *mut core::ffi::c_void {
         ManuallyDrop::new(self).ptr.as_ptr().cast()
@@ -363,6 +364,12 @@ unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> ArcBorrow<'a, T> {
         // for the lifetime of the returned value.
         unsafe { ArcBorrow::new(inner) }
     }
+
+    unsafe fn borrow_mut<'a>(ptr: *mut core::ffi::c_void) -> ArcBorrow<'a, T> {
+        // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety
+        // requirements for `borrow`.
+        unsafe { Self::borrow(ptr) }
+    }
 }
 
 impl<T: ?Sized> Deref for Arc<T> {
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index b8a7b2ee96a17081ad31e1bb73cb0513bcd05ef4..d742544da793a08eaf342ba88722dfb1319b93d6 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -19,26 +19,33 @@
 /// This trait is meant to be used in cases when Rust objects are stored in C objects and
 /// eventually "freed" back to Rust.
 pub trait ForeignOwnable: Sized {
-    /// Type of values borrowed between calls to [`ForeignOwnable::into_foreign`] and
-    /// [`ForeignOwnable::from_foreign`].
+    /// Type used to immutably borrow a value that is currently foreign-owned.
     type Borrowed<'a>;
 
+    /// Type used to mutably borrow a value that is currently foreign-owned.
+    type BorrowedMut<'a>;
+
     /// Converts a Rust-owned object to a foreign-owned one.
     ///
     /// The foreign representation is a pointer to void. There are no guarantees for this pointer.
     /// For example, it might be invalid, dangling or pointing to uninitialized memory. Using it in
-    /// any way except for [`ForeignOwnable::from_foreign`], [`ForeignOwnable::borrow`],
-    /// [`ForeignOwnable::try_from_foreign`] can result in undefined behavior.
+    /// any way except for [`from_foreign`], [`try_from_foreign`], [`borrow`], or [`borrow_mut`] can
+    /// result in undefined behavior.
+    ///
+    /// [`from_foreign`]: Self::from_foreign
+    /// [`try_from_foreign`]: Self::try_from_foreign
+    /// [`borrow`]: Self::borrow
+    /// [`borrow_mut`]: Self::borrow_mut
     fn into_foreign(self) -> *mut core::ffi::c_void;
 
     /// Converts a foreign-owned object back to a Rust-owned one.
     ///
     /// # Safety
     ///
-    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
-    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
-    /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for
-    /// this object must have been dropped.
+    /// The provided pointer must have been returned by a previous call to [`into_foreign`], and it
+    /// must not be passed to `from_foreign` more than once.
+    ///
+    /// [`into_foreign`]: Self::into_foreign
     unsafe fn from_foreign(ptr: *mut core::ffi::c_void) -> Self;
 
     /// Tries to convert a foreign-owned object back to a Rust-owned one.
@@ -48,8 +55,9 @@ pub trait ForeignOwnable: Sized {
     ///
     /// # Safety
     ///
-    /// `ptr` must either be null or satisfy the safety requirements for
-    /// [`ForeignOwnable::from_foreign`].
+    /// `ptr` must either be null or satisfy the safety requirements for [`from_foreign`].
+    ///
+    /// [`from_foreign`]: Self::from_foreign
     unsafe fn try_from_foreign(ptr: *mut core::ffi::c_void) -> Option<Self> {
         if ptr.is_null() {
             None
@@ -60,17 +68,53 @@ unsafe fn try_from_foreign(ptr: *mut core::ffi::c_void) -> Option<Self> {
         }
     }
 
-    /// Borrows a foreign-owned object.
+    /// Borrows a foreign-owned object immutably.
+    ///
+    /// This method provides a way to access a foreign-owned value from Rust immutably. It provides
+    /// you with exactly the same abilities as an `&Self` when the value is Rust-owned.
     ///
     /// # Safety
     ///
-    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
-    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
+    /// The provided pointer must have been returned by a previous call to [`into_foreign`], and if
+    /// the pointer is ever passed to [`from_foreign`], then that call must happen after the end of
+    /// the lifetime 'a.
+    ///
+    /// [`into_foreign`]: Self::into_foreign
+    /// [`from_foreign`]: Self::from_foreign
     unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> Self::Borrowed<'a>;
+
+    /// Borrows a foreign-owned object mutably.
+    ///
+    /// This method provides a way to access a foreign-owned value from Rust mutably. It provides
+    /// you with exactly the same abilities as an `&mut Self` when the value is Rust-owned, except
+    /// that the address of the object must not be changed.
+    ///
+    /// Note that for types like [`Arc`], an `&mut Arc<T>` only gives you immutable access to the
+    /// inner value, so this method also only provides immutable access in that case.
+    ///
+    /// In the case of `Box<T>`, this method gives you the ability to modify the inner `T`, but it
+    /// does not let you change the box itself. That is, you cannot change which allocation the box
+    /// points at.
+    ///
+    /// # Safety
+    ///
+    /// The provided pointer must have been returned by a previous call to [`into_foreign`], and if
+    /// the pointer is ever passed to [`from_foreign`], then that call must happen after the end of
+    /// the lifetime 'a.
+    ///
+    /// The lifetime 'a must not overlap with the lifetime of any other call to [`borrow`] or
+    /// `borrow_mut` on the same object.
+    ///
+    /// [`into_foreign`]: Self::into_foreign
+    /// [`from_foreign`]: Self::from_foreign
+    /// [`borrow`]: Self::borrow
+    /// [`Arc`]: crate::sync::Arc
+    unsafe fn borrow_mut<'a>(ptr: *mut core::ffi::c_void) -> Self::BorrowedMut<'a>;
 }
 
 impl ForeignOwnable for () {
     type Borrowed<'a> = ();
+    type BorrowedMut<'a> = ();
 
     fn into_foreign(self) -> *mut core::ffi::c_void {
         core::ptr::NonNull::dangling().as_ptr()
@@ -79,6 +123,7 @@ fn into_foreign(self) -> *mut core::ffi::c_void {
     unsafe fn from_foreign(_: *mut core::ffi::c_void) -> Self {}
 
     unsafe fn borrow<'a>(_: *mut core::ffi::c_void) -> Self::Borrowed<'a> {}
+    unsafe fn borrow_mut<'a>(_: *mut core::ffi::c_void) -> Self::BorrowedMut<'a> {}
 }
 
 /// Runs a cleanup function/closure when dropped.

-- 
2.47.0

Re: [PATCH 5/5] rust: add improved version of `ForeignOwnable::borrow_mut`

[Andreas Hindborg]

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

> From: Alice Ryhl <aliceryhl@google.com>
>
> Previously, the `ForeignOwnable` trait had a method called `borrow_mut`
> that was intended to provide mutable access to the inner value. However,
> the method accidentally made it possible to change the address of the
> object being modified, which usually isn't what we want. (And when we
> want that, it can be done by calling `from_foreign` and `into_foreign`,
> like how the old `borrow_mut` was implemented.)
>
> In this patch, we introduce an alternate definition of `borrow_mut` that
> solves the previous problem. Conceptually, given a pointer type `P` that
> implements `ForeignOwnable`, the `borrow_mut` method gives you the same
> kind of access as an `&mut P` would, except that it does not let you
> change the pointer `P` itself.
>
> This is analogous to how the existing `borrow` method provides the same
> kind of access to the inner value as an `&P`.
>
> Note that for types like `Arc`, having an `&mut Arc<T>` only gives you
> immutable access to the inner `T`. This is because mutable references
> assume exclusive access, but there might be other handles to the same
> reference counted value, so the access isn't exclusive. The `Arc` type
> implements this by making `borrow_mut` return the same type as `borrow`.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>

Best regards,
Andreas Hindborg

Re: [PATCH 5/5] rust: add improved version of `ForeignOwnable::borrow_mut`

[Alice Ryhl]

On Wed, Oct 30, 2024 at 9:46 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> From: Alice Ryhl <aliceryhl@google.com>
>
> Previously, the `ForeignOwnable` trait had a method called `borrow_mut`
> that was intended to provide mutable access to the inner value. However,
> the method accidentally made it possible to change the address of the
> object being modified, which usually isn't what we want. (And when we
> want that, it can be done by calling `from_foreign` and `into_foreign`,
> like how the old `borrow_mut` was implemented.)
>
> In this patch, we introduce an alternate definition of `borrow_mut` that
> solves the previous problem. Conceptually, given a pointer type `P` that
> implements `ForeignOwnable`, the `borrow_mut` method gives you the same
> kind of access as an `&mut P` would, except that it does not let you
> change the pointer `P` itself.
>
> This is analogous to how the existing `borrow` method provides the same
> kind of access to the inner value as an `&P`.
>
> Note that for types like `Arc`, having an `&mut Arc<T>` only gives you
> immutable access to the inner `T`. This is because mutable references
> assume exclusive access, but there might be other handles to the same
> reference counted value, so the access isn't exclusive. The `Arc` type
> implements this by making `borrow_mut` return the same type as `borrow`.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

You must add your own SoB at the end when resending other's patches.

Alice

Re: [PATCH 5/5] rust: add improved version of `ForeignOwnable::borrow_mut`

[Tamir Duberstein]

On Thu, Oct 31, 2024 at 6:54 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Wed, Oct 30, 2024 at 9:46 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > From: Alice Ryhl <aliceryhl@google.com>
> >
> > Previously, the `ForeignOwnable` trait had a method called `borrow_mut`
> > that was intended to provide mutable access to the inner value. However,
> > the method accidentally made it possible to change the address of the
> > object being modified, which usually isn't what we want. (And when we
> > want that, it can be done by calling `from_foreign` and `into_foreign`,
> > like how the old `borrow_mut` was implemented.)
> >
> > In this patch, we introduce an alternate definition of `borrow_mut` that
> > solves the previous problem. Conceptually, given a pointer type `P` that
> > implements `ForeignOwnable`, the `borrow_mut` method gives you the same
> > kind of access as an `&mut P` would, except that it does not let you
> > change the pointer `P` itself.
> >
> > This is analogous to how the existing `borrow` method provides the same
> > kind of access to the inner value as an `&P`.
> >
> > Note that for types like `Arc`, having an `&mut Arc<T>` only gives you
> > immutable access to the inner `T`. This is because mutable references
> > assume exclusive access, but there might be other handles to the same
> > reference counted value, so the access isn't exclusive. The `Arc` type
> > implements this by making `borrow_mut` return the same type as `borrow`.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
>
> You must add your own SoB at the end when resending other's patches.
>
> Alice

Thanks, I'll add it in v2. Does the order matter?

Re: [PATCH 5/5] rust: add improved version of `ForeignOwnable::borrow_mut`

[Miguel Ojeda]

On Thu, Oct 31, 2024 at 1:23 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Thanks, I'll add it in v2. Does the order matter?

Yes, please add it after the reviews you picked up, i.e. the latest
thing in your commit message. Please see:

    https://docs.kernel.org/process/submitting-patches.html

Thanks!

Cheers,
Miguel