[v1] rust: add improved version of `ForeignOwnable::borrow_mut`

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

Posted by Tamir Duberstein 3 weeks, 4 days ago

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`

Posted by Andreas Hindborg 3 weeks, 3 days ago

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

Posted by Tamir Duberstein 3 weeks, 3 days ago

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`

Posted by Alice Ryhl 3 weeks, 3 days ago

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>