[PATCH v2 0/2] Remove `ArcBorrow`

[PATCH v2 0/2] Remove `ArcBorrow`

[Wedson Almeida Filho]

From: Wedson Almeida Filho <walmeida@microsoft.com>

In this small series we remove `ArcBorrow<'_, T>` and replace it with
`&WithRef<T>`, which we used to call `ArcInner` --  it is renamed
because it is now public, so we chose a more meaningful name.

This simplifies the code because we remove a type and have simpler
syntax to refer to borrowed arcs: `ArcBorrow<'a, T>` vs 
`&'a WithRef<T>`.

This became possible when we adopted GATs in 2021 but we only realised
it recently (thanks Boqun!), more details on this zulip thread:
https://rust-for-linux.zulipchat.com/#narrow/stream/291566-Library/topic/Isn't.20.60ArcBorrow.60.20just.20.60.26ArcInner.3CT.3E.60.3F

Changes v1 -> v2:
* Moved the definition of `WithRef` to where `ArcBorrow` was.
* Updated documentation of `as_arc_borrow`.

Wedson Almeida Filho (2):
  rust: arc: rename `ArcInner` to `WithRef`
  rust: arc: remove `ArcBorrow` in favour of `WithRef`

 rust/kernel/sync.rs                |   2 +-
 rust/kernel/sync/arc.rs            | 146 +++++++++--------------------
 rust/kernel/sync/arc/std_vendor.rs |   4 +-
 3 files changed, 47 insertions(+), 105 deletions(-)


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.34.1

[PATCH v2 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Wedson Almeida Filho]

From: Wedson Almeida Filho <walmeida@microsoft.com>

This is in preparation for removing `ArcBorrow` and making `WithRef`
public.

This is a pure name change with no functional changes intended.

Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
 rust/kernel/sync/arc.rs            | 20 ++++++++++----------
 rust/kernel/sync/arc/std_vendor.rs |  4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 3d496391a9bd..86bff1e0002c 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -126,13 +126,13 @@
 /// # Ok::<(), Error>(())
 /// ```
 pub struct Arc<T: ?Sized> {
-    ptr: NonNull<ArcInner<T>>,
-    _p: PhantomData<ArcInner<T>>,
+    ptr: NonNull<WithRef<T>>,
+    _p: PhantomData<WithRef<T>>,
 }
 
 #[pin_data]
 #[repr(C)]
-struct ArcInner<T: ?Sized> {
+struct WithRef<T: ?Sized> {
     refcount: Opaque<bindings::refcount_t>,
     data: T,
 }
@@ -164,7 +164,7 @@ impl<T> Arc<T> {
     /// Constructs a new reference counted instance of `T`.
     pub fn try_new(contents: T) -> Result<Self, AllocError> {
         // INVARIANT: The refcount is initialised to a non-zero value.
-        let value = ArcInner {
+        let value = WithRef {
             // SAFETY: There are no safety requirements for this FFI call.
             refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
             data: contents,
@@ -201,13 +201,13 @@ pub fn init<E>(init: impl Init<T, E>) -> error::Result<Self>
 }
 
 impl<T: ?Sized> Arc<T> {
-    /// Constructs a new [`Arc`] from an existing [`ArcInner`].
+    /// Constructs a new [`Arc`] from an existing [`WithRef`].
     ///
     /// # Safety
     ///
     /// The caller must ensure that `inner` points to a valid location and has a non-zero reference
     /// count, one of which will be owned by the new [`Arc`] instance.
-    unsafe fn from_inner(inner: NonNull<ArcInner<T>>) -> Self {
+    unsafe fn from_inner(inner: NonNull<WithRef<T>>) -> Self {
         // INVARIANT: By the safety requirements, the invariants hold.
         Arc {
             ptr: inner,
@@ -243,7 +243,7 @@ fn into_foreign(self) -> *const core::ffi::c_void {
     unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
         // SAFETY: 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();
+        let inner = NonNull::new(ptr as *mut WithRef<T>).unwrap();
 
         // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
         // for the lifetime of the returned value.
@@ -376,7 +376,7 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
 /// # Ok::<(), Error>(())
 /// ```
 pub struct ArcBorrow<'a, T: ?Sized + 'a> {
-    inner: NonNull<ArcInner<T>>,
+    inner: NonNull<WithRef<T>>,
     _p: PhantomData<&'a ()>,
 }
 
@@ -406,7 +406,7 @@ impl<T: ?Sized> ArcBorrow<'_, T> {
     /// Callers must ensure the following for the lifetime of the returned [`ArcBorrow`] instance:
     /// 1. That `inner` remains valid;
     /// 2. That no mutable references to `inner` are created.
-    unsafe fn new(inner: NonNull<ArcInner<T>>) -> Self {
+    unsafe fn new(inner: NonNull<WithRef<T>>) -> Self {
         // INVARIANT: The safety requirements guarantee the invariants.
         Self {
             inner,
@@ -526,7 +526,7 @@ pub fn try_new(value: T) -> Result<Self, AllocError> {
     /// Tries to allocate a new [`UniqueArc`] instance whose contents are not initialised yet.
     pub fn try_new_uninit() -> Result<UniqueArc<MaybeUninit<T>>, AllocError> {
         // INVARIANT: The refcount is initialised to a non-zero value.
-        let inner = Box::try_init::<AllocError>(try_init!(ArcInner {
+        let inner = Box::try_init::<AllocError>(try_init!(WithRef {
             // SAFETY: There are no safety requirements for this FFI call.
             refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
             data <- init::uninit::<T, AllocError>(),
diff --git a/rust/kernel/sync/arc/std_vendor.rs b/rust/kernel/sync/arc/std_vendor.rs
index a66a0c2831b3..4b30e5597ba5 100644
--- a/rust/kernel/sync/arc/std_vendor.rs
+++ b/rust/kernel/sync/arc/std_vendor.rs
@@ -5,7 +5,7 @@
 //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
 //! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>.
 
-use crate::sync::{arc::ArcInner, Arc};
+use crate::sync::{arc::WithRef, Arc};
 use core::any::Any;
 
 impl Arc<dyn Any + Send + Sync> {
@@ -17,7 +17,7 @@ pub fn downcast<T>(self) -> core::result::Result<Arc<T>, Self>
         if (*self).is::<T>() {
             // SAFETY: We have just checked that the type is correct, so we can cast the pointer.
             unsafe {
-                let ptr = self.ptr.cast::<ArcInner<T>>();
+                let ptr = self.ptr.cast::<WithRef<T>>();
                 core::mem::forget(self);
                 Ok(Arc::from_inner(ptr))
             }
-- 
2.34.1

Re: [PATCH v2 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Martin Rodriguez Reboredo]

On 9/23/23 11:49, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This is in preparation for removing `ArcBorrow` and making `WithRef`
> public.
> 
> This is a pure name change with no functional changes intended.
> 
> Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v2 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Benno Lossin]

On 23.09.23 16:49, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This is in preparation for removing `ArcBorrow` and making `WithRef`
> public.
> 
> This is a pure name change with no functional changes intended.
> 
> Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
>   rust/kernel/sync/arc.rs            | 20 ++++++++++----------
>   rust/kernel/sync/arc/std_vendor.rs |  4 ++--
>   2 files changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

Re: [PATCH v2 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Jianguo Bao]

Reviewed-by: Jianguo Bao <roidinev@gmail.com>

Re: [PATCH v2 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Alice Ryhl]

On Sat, Sep 23, 2023 at 4:49 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> This is in preparation for removing `ArcBorrow` and making `WithRef`
> public.
>
> This is a pure name change with no functional changes intended.
>
> Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
>  rust/kernel/sync/arc.rs            | 20 ++++++++++----------
>  rust/kernel/sync/arc/std_vendor.rs |  4 ++--
>  2 files changed, 12 insertions(+), 12 deletions(-)

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

[PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Wedson Almeida Filho]

From: Wedson Almeida Filho <walmeida@microsoft.com>

With GATs, we don't need a separate type to represent a borrowed object
with a refcount, we can just use Rust's regular shared borrowing. In
this case, we use `&WithRef<T>` instead of `ArcBorrow<'_, T>`.

Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
 rust/kernel/sync.rs     |   2 +-
 rust/kernel/sync/arc.rs | 134 ++++++++++++----------------------------
 2 files changed, 39 insertions(+), 97 deletions(-)

diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index d219ee518eff..083494884500 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -12,7 +12,7 @@
 pub mod lock;
 mod locked_by;
 
-pub use arc::{Arc, ArcBorrow, UniqueArc};
+pub use arc::{Arc, UniqueArc, WithRef};
 pub use condvar::CondVar;
 pub use lock::{mutex::Mutex, spinlock::SpinLock};
 pub use locked_by::LockedBy;
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 86bff1e0002c..a1806e38c37f 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -105,14 +105,14 @@
 /// Coercion from `Arc<Example>` to `Arc<dyn MyTrait>`:
 ///
 /// ```
-/// use kernel::sync::{Arc, ArcBorrow};
+/// use kernel::sync::{Arc, WithRef};
 ///
 /// trait MyTrait {
 ///     // Trait has a function whose `self` type is `Arc<Self>`.
 ///     fn example1(self: Arc<Self>) {}
 ///
-///     // Trait has a function whose `self` type is `ArcBorrow<'_, Self>`.
-///     fn example2(self: ArcBorrow<'_, Self>) {}
+///     // Trait has a function whose `self` type is `&WithRef<Self>`.
+///     fn example2(self: &WithRef<Self>) {}
 /// }
 ///
 /// struct Example;
@@ -130,13 +130,6 @@ pub struct Arc<T: ?Sized> {
     _p: PhantomData<WithRef<T>>,
 }
 
-#[pin_data]
-#[repr(C)]
-struct WithRef<T: ?Sized> {
-    refcount: Opaque<bindings::refcount_t>,
-    data: T,
-}
-
 // This is to allow [`Arc`] (and variants) to be used as the type of `self`.
 impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
 
@@ -215,16 +208,16 @@ unsafe fn from_inner(inner: NonNull<WithRef<T>>) -> Self {
         }
     }
 
-    /// Returns an [`ArcBorrow`] from the given [`Arc`].
+    /// Returns a shared reference to a [`WithRef`] the given [`Arc`].
     ///
-    /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
-    /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised.
+    /// This is useful when the argument of a function call is a [`WithRef`] (e.g., in a method
+    /// receiver), but we have an [`Arc`] instead. Getting a [`WithRef`] is free when optimised.
     #[inline]
-    pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> {
+    pub fn as_with_ref(&self) -> &WithRef<T> {
         // SAFETY: The constraint that the lifetime of the shared reference must outlive that of
-        // the returned `ArcBorrow` ensures that the object remains alive and that no mutable
+        // the returned `WithRef` ensures that the object remains alive and that no mutable
         // reference can be created.
-        unsafe { ArcBorrow::new(self.ptr) }
+        unsafe { self.ptr.as_ref() }
     }
 
     /// Compare whether two [`Arc`] pointers reference the same underlying object.
@@ -234,20 +227,17 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
 }
 
 impl<T: 'static> ForeignOwnable for Arc<T> {
-    type Borrowed<'a> = ArcBorrow<'a, T>;
+    type Borrowed<'a> = &'a WithRef<T>;
 
     fn into_foreign(self) -> *const core::ffi::c_void {
         ManuallyDrop::new(self).ptr.as_ptr() as _
     }
 
-    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
+    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a WithRef<T> {
         // SAFETY: 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 WithRef<T>).unwrap();
-
-        // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
-        // for the lifetime of the returned value.
-        unsafe { ArcBorrow::new(inner) }
+        // a previous call to `Arc::into_foreign`. The safety requirements of `from_foreign` ensure
+        // that the object remains alive for the lifetime of the returned value.
+        unsafe { &*(ptr.cast::<WithRef<T>>()) }
     }
 
     unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
@@ -320,119 +310,71 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
     }
 }
 
-/// A borrowed reference to an [`Arc`] instance.
-///
-/// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler
-/// to use just `&T`, which we can trivially get from an `Arc<T>` instance.
+/// An instance of `T` with an attached reference count.
 ///
-/// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
-/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
-/// to a pointer (`Arc<T>`) to the object (`T`). An [`ArcBorrow`] eliminates this double
-/// indirection while still allowing one to increment the refcount and getting an `Arc<T>` when/if
-/// needed.
-///
-/// # Invariants
-///
-/// There are no mutable references to the underlying [`Arc`], and it remains valid for the
-/// lifetime of the [`ArcBorrow`] instance.
-///
-/// # Example
+/// # Examples
 ///
 /// ```
-/// use kernel::sync::{Arc, ArcBorrow};
+/// use kernel::sync::{Arc, WithRef};
 ///
 /// struct Example;
 ///
-/// fn do_something(e: ArcBorrow<'_, Example>) -> Arc<Example> {
+/// fn do_something(e: &WithRef<Example>) -> Arc<Example> {
 ///     e.into()
 /// }
 ///
 /// let obj = Arc::try_new(Example)?;
-/// let cloned = do_something(obj.as_arc_borrow());
+/// let cloned = do_something(obj.as_with_ref());
 ///
 /// // Assert that both `obj` and `cloned` point to the same underlying object.
 /// assert!(core::ptr::eq(&*obj, &*cloned));
-/// # Ok::<(), Error>(())
 /// ```
 ///
-/// Using `ArcBorrow<T>` as the type of `self`:
+/// Using `WithRef<T>` as the type of `self`:
 ///
 /// ```
-/// use kernel::sync::{Arc, ArcBorrow};
+/// use kernel::sync::{Arc, WithRef};
 ///
 /// struct Example {
-///     a: u32,
-///     b: u32,
+///     _a: u32,
+///     _b: u32,
 /// }
 ///
 /// impl Example {
-///     fn use_reference(self: ArcBorrow<'_, Self>) {
+///     fn use_reference(self: &WithRef<Self>) {
 ///         // ...
 ///     }
 /// }
 ///
-/// let obj = Arc::try_new(Example { a: 10, b: 20 })?;
-/// obj.as_arc_borrow().use_reference();
-/// # Ok::<(), Error>(())
+/// let obj = Arc::try_new(Example { _a: 10, _b: 20 })?;
+/// obj.as_with_ref().use_reference();
 /// ```
-pub struct ArcBorrow<'a, T: ?Sized + 'a> {
-    inner: NonNull<WithRef<T>>,
-    _p: PhantomData<&'a ()>,
-}
-
-// This is to allow [`ArcBorrow`] (and variants) to be used as the type of `self`.
-impl<T: ?Sized> core::ops::Receiver for ArcBorrow<'_, T> {}
-
-// This is to allow `ArcBorrow<U>` to be dispatched on when `ArcBorrow<T>` can be coerced into
-// `ArcBorrow<U>`.
-impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<ArcBorrow<'_, U>>
-    for ArcBorrow<'_, T>
-{
-}
-
-impl<T: ?Sized> Clone for ArcBorrow<'_, T> {
-    fn clone(&self) -> Self {
-        *self
-    }
+#[pin_data]
+#[repr(C)]
+pub struct WithRef<T: ?Sized> {
+    refcount: Opaque<bindings::refcount_t>,
+    data: T,
 }
 
-impl<T: ?Sized> Copy for ArcBorrow<'_, T> {}
-
-impl<T: ?Sized> ArcBorrow<'_, T> {
-    /// Creates a new [`ArcBorrow`] instance.
-    ///
-    /// # Safety
-    ///
-    /// Callers must ensure the following for the lifetime of the returned [`ArcBorrow`] instance:
-    /// 1. That `inner` remains valid;
-    /// 2. That no mutable references to `inner` are created.
-    unsafe fn new(inner: NonNull<WithRef<T>>) -> Self {
-        // INVARIANT: The safety requirements guarantee the invariants.
-        Self {
-            inner,
-            _p: PhantomData,
-        }
-    }
-}
+// This is to allow [`WithRef`] (and variants) to be used as the type of `self`.
+impl<T: ?Sized> core::ops::Receiver for WithRef<T> {}
 
-impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
-    fn from(b: ArcBorrow<'_, T>) -> Self {
+impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
+    fn from(b: &WithRef<T>) -> Self {
         // SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
         // guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
         // increment.
-        ManuallyDrop::new(unsafe { Arc::from_inner(b.inner) })
+        ManuallyDrop::new(unsafe { Arc::from_inner(b.into()) })
             .deref()
             .clone()
     }
 }
 
-impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
+impl<T: ?Sized> Deref for WithRef<T> {
     type Target = 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 }
+        &self.data
     }
 }
 
-- 
2.34.1

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Martin Rodriguez Reboredo]

On 9/23/23 11:49, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> With GATs, we don't need a separate type to represent a borrowed object
> with a refcount, we can just use Rust's regular shared borrowing. In
> this case, we use `&WithRef<T>` instead of `ArcBorrow<'_, T>`.
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Benno Lossin]

On 23.09.23 16:49, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> With GATs, we don't need a separate type to represent a borrowed object
> with a refcount, we can just use Rust's regular shared borrowing. In
> this case, we use `&WithRef<T>` instead of `ArcBorrow<'_, T>`.
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
>   rust/kernel/sync.rs     |   2 +-
>   rust/kernel/sync/arc.rs | 134 ++++++++++++----------------------------
>   2 files changed, 39 insertions(+), 97 deletions(-)
Reviewed-by: Benno Lossin <benno.lossin@proton.me>

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Jianguo Bao]

Hi,
This time i just realize that the `Ref` in WithRef is the refcount, not the reference
in rust . So can we just change the name WithRefC or other to express this 
refcount,not reference.
If we have defined the Ref only mean the refcount already in somewhere such 
as rust doc or have some conventions in rust community before, then omit this suggest.

On Sat, Sep 23, 2023 at 11:49:38AM -0300, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> With GATs, we don't need a separate type to represent a borrowed object
> with a refcount, we can just use Rust's regular shared borrowing. In
> this case, we use `&WithRef<T>` instead of `ArcBorrow<'_, T>`.
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
>  rust/kernel/sync.rs     |   2 +-
>  rust/kernel/sync/arc.rs | 134 ++++++++++++----------------------------
>  2 files changed, 39 insertions(+), 97 deletions(-)
> 
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index d219ee518eff..083494884500 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -12,7 +12,7 @@
>  pub mod lock;
>  mod locked_by;
>  
> -pub use arc::{Arc, ArcBorrow, UniqueArc};
> +pub use arc::{Arc, UniqueArc, WithRef};
>  pub use condvar::CondVar;
>  pub use lock::{mutex::Mutex, spinlock::SpinLock};
>  pub use locked_by::LockedBy;
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 86bff1e0002c..a1806e38c37f 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -105,14 +105,14 @@
>  /// Coercion from `Arc<Example>` to `Arc<dyn MyTrait>`:
>  ///
>  /// ```
> -/// use kernel::sync::{Arc, ArcBorrow};
> +/// use kernel::sync::{Arc, WithRef};
>  ///
>  /// trait MyTrait {
>  ///     // Trait has a function whose `self` type is `Arc<Self>`.
>  ///     fn example1(self: Arc<Self>) {}
>  ///
> -///     // Trait has a function whose `self` type is `ArcBorrow<'_, Self>`.
> -///     fn example2(self: ArcBorrow<'_, Self>) {}
> +///     // Trait has a function whose `self` type is `&WithRef<Self>`.
> +///     fn example2(self: &WithRef<Self>) {}
>  /// }
>  ///
>  /// struct Example;
> @@ -130,13 +130,6 @@ pub struct Arc<T: ?Sized> {
>      _p: PhantomData<WithRef<T>>,
>  }
>  
> -#[pin_data]
> -#[repr(C)]
> -struct WithRef<T: ?Sized> {
> -    refcount: Opaque<bindings::refcount_t>,
> -    data: T,
> -}
> -
>  // This is to allow [`Arc`] (and variants) to be used as the type of `self`.
>  impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
>  
> @@ -215,16 +208,16 @@ unsafe fn from_inner(inner: NonNull<WithRef<T>>) -> Self {
>          }
>      }
>  
> -    /// Returns an [`ArcBorrow`] from the given [`Arc`].
> +    /// Returns a shared reference to a [`WithRef`] the given [`Arc`].

       /// Returns a shared reference to a [`WithRef`] from  the given [`Arc`].
    or 
       /// Returns a borrowed arc [`&WithRef`] from  the given [`Arc`].
    ?

>      ///
> -    /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
> -    /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised.
> +    /// This is useful when the argument of a function call is a [`WithRef`] (e.g., in a method
> +    /// receiver), but we have an [`Arc`] instead. Getting a [`WithRef`] is free when optimised.
>      #[inline]
> -    pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> {
> +    pub fn as_with_ref(&self) -> &WithRef<T> {
>          // SAFETY: The constraint that the lifetime of the shared reference must outlive that of
> -        // the returned `ArcBorrow` ensures that the object remains alive and that no mutable
> +        // the returned `WithRef` ensures that the object remains alive and that no mutable
>          // reference can be created.
> -        unsafe { ArcBorrow::new(self.ptr) }
> +        unsafe { self.ptr.as_ref() }
>      }
>  
>      /// Compare whether two [`Arc`] pointers reference the same underlying object.
> @@ -234,20 +227,17 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
>  }
>  
>  impl<T: 'static> ForeignOwnable for Arc<T> {
> -    type Borrowed<'a> = ArcBorrow<'a, T>;
> +    type Borrowed<'a> = &'a WithRef<T>;
>  
>      fn into_foreign(self) -> *const core::ffi::c_void {
>          ManuallyDrop::new(self).ptr.as_ptr() as _
>      }
>  
> -    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
> +    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a WithRef<T> {
>          // SAFETY: 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 WithRef<T>).unwrap();
> -
> -        // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
> -        // for the lifetime of the returned value.
> -        unsafe { ArcBorrow::new(inner) }
> +        // a previous call to `Arc::into_foreign`. The safety requirements of `from_foreign` ensure
> +        // that the object remains alive for the lifetime of the returned value.
> +        unsafe { &*(ptr.cast::<WithRef<T>>()) }
>      }
>  
>      unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
> @@ -320,119 +310,71 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
>      }
>  }
>  
> -/// A borrowed reference to an [`Arc`] instance.
> -///
> -/// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler
> -/// to use just `&T`, which we can trivially get from an `Arc<T>` instance.
> +/// An instance of `T` with an attached reference count.
>  ///
> -/// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
> -/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
> -/// to a pointer (`Arc<T>`) to the object (`T`). An [`ArcBorrow`] eliminates this double
> -/// indirection while still allowing one to increment the refcount and getting an `Arc<T>` when/if
> -/// needed.
> -///
> -/// # Invariants
> -///
> -/// There are no mutable references to the underlying [`Arc`], and it remains valid for the
> -/// lifetime of the [`ArcBorrow`] instance.
> -///
> -/// # Example
> +/// # Examples
>  ///
>  /// ```
> -/// use kernel::sync::{Arc, ArcBorrow};
> +/// use kernel::sync::{Arc, WithRef};
>  ///
>  /// struct Example;
>  ///
> -/// fn do_something(e: ArcBorrow<'_, Example>) -> Arc<Example> {
> +/// fn do_something(e: &WithRef<Example>) -> Arc<Example> {
>  ///     e.into()
>  /// }
>  ///
>  /// let obj = Arc::try_new(Example)?;
> -/// let cloned = do_something(obj.as_arc_borrow());
> +/// let cloned = do_something(obj.as_with_ref());
>  ///
>  /// // Assert that both `obj` and `cloned` point to the same underlying object.
>  /// assert!(core::ptr::eq(&*obj, &*cloned));
> -/// # Ok::<(), Error>(())
>  /// ```
>  ///
> -/// Using `ArcBorrow<T>` as the type of `self`:
> +/// Using `WithRef<T>` as the type of `self`:
>  ///
>  /// ```
> -/// use kernel::sync::{Arc, ArcBorrow};
> +/// use kernel::sync::{Arc, WithRef};
>  ///
>  /// struct Example {
> -///     a: u32,
> -///     b: u32,
> +///     _a: u32,
> +///     _b: u32,
>  /// }
>  ///
>  /// impl Example {
> -///     fn use_reference(self: ArcBorrow<'_, Self>) {
> +///     fn use_reference(self: &WithRef<Self>) {
>  ///         // ...
>  ///     }
>  /// }
>  ///
> -/// let obj = Arc::try_new(Example { a: 10, b: 20 })?;
> -/// obj.as_arc_borrow().use_reference();
> -/// # Ok::<(), Error>(())
> +/// let obj = Arc::try_new(Example { _a: 10, _b: 20 })?;
> +/// obj.as_with_ref().use_reference();
>  /// ```
> -pub struct ArcBorrow<'a, T: ?Sized + 'a> {
> -    inner: NonNull<WithRef<T>>,
> -    _p: PhantomData<&'a ()>,
> -}
> -
> -// This is to allow [`ArcBorrow`] (and variants) to be used as the type of `self`.
> -impl<T: ?Sized> core::ops::Receiver for ArcBorrow<'_, T> {}
> -
> -// This is to allow `ArcBorrow<U>` to be dispatched on when `ArcBorrow<T>` can be coerced into
> -// `ArcBorrow<U>`.
> -impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<ArcBorrow<'_, U>>
> -    for ArcBorrow<'_, T>
> -{
> -}
> -
> -impl<T: ?Sized> Clone for ArcBorrow<'_, T> {
> -    fn clone(&self) -> Self {
> -        *self
> -    }
> +#[pin_data]
> +#[repr(C)]
> +pub struct WithRef<T: ?Sized> {
> +    refcount: Opaque<bindings::refcount_t>,
> +    data: T,
>  }
>  
> -impl<T: ?Sized> Copy for ArcBorrow<'_, T> {}
> -
> -impl<T: ?Sized> ArcBorrow<'_, T> {
> -    /// Creates a new [`ArcBorrow`] instance.
> -    ///
> -    /// # Safety
> -    ///
> -    /// Callers must ensure the following for the lifetime of the returned [`ArcBorrow`] instance:
> -    /// 1. That `inner` remains valid;
> -    /// 2. That no mutable references to `inner` are created.
> -    unsafe fn new(inner: NonNull<WithRef<T>>) -> Self {
> -        // INVARIANT: The safety requirements guarantee the invariants.
> -        Self {
> -            inner,
> -            _p: PhantomData,
> -        }
> -    }
> -}
> +// This is to allow [`WithRef`] (and variants) to be used as the type of `self`.
> +impl<T: ?Sized> core::ops::Receiver for WithRef<T> {}
>  
> -impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
> -    fn from(b: ArcBorrow<'_, T>) -> Self {
> +impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> +    fn from(b: &WithRef<T>) -> Self {
>          // SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
>          // guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
>          // increment.
> -        ManuallyDrop::new(unsafe { Arc::from_inner(b.inner) })
> +        ManuallyDrop::new(unsafe { Arc::from_inner(b.into()) })
>              .deref()
>              .clone()
>      }
>  }
>  
> -impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
> +impl<T: ?Sized> Deref for WithRef<T> {
>      type Target = 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 }
> +        &self.data
>      }
>  }
>  
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Alice Ryhl]

On Sat, Sep 23, 2023 at 4:50 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> With GATs, we don't need a separate type to represent a borrowed object
> with a refcount, we can just use Rust's regular shared borrowing. In
> this case, we use `&WithRef<T>` instead of `ArcBorrow<'_, T>`.
>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
>  rust/kernel/sync.rs     |   2 +-
>  rust/kernel/sync/arc.rs | 134 ++++++++++++----------------------------
>  2 files changed, 39 insertions(+), 97 deletions(-)

I'm concerned about this change, because an `&WithRef<T>` only has
immutable permissions for the allocation. No pointer derived from it
may be used to modify the value in the Arc, however, the drop
implementation of Arc will do exactly that. It also means that we
can't convert an Arc with refcount 1 into a UniqueArc.

Alice

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Benno Lossin]

On 25.09.23 08:29, Alice Ryhl wrote:
> On Sat, Sep 23, 2023 at 4:50 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>>
>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>>
>> With GATs, we don't need a separate type to represent a borrowed object
>> with a refcount, we can just use Rust's regular shared borrowing. In
>> this case, we use `&WithRef<T>` instead of `ArcBorrow<'_, T>`.
>>
>> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>> ---
>>   rust/kernel/sync.rs     |   2 +-
>>   rust/kernel/sync/arc.rs | 134 ++++++++++++----------------------------
>>   2 files changed, 39 insertions(+), 97 deletions(-)
> 
> I'm concerned about this change, because an `&WithRef<T>` only has
> immutable permissions for the allocation. No pointer derived from it
> may be used to modify the value in the Arc, however, the drop
> implementation of Arc will do exactly that. 

That is indeed a problem. We could put the value in an `UnsafeCell`, but
that would lose us niche optimizations and probably also other optimizations.

> It also means that we
> can't convert an Arc with refcount 1 into a UniqueArc.

I think you still can, since to do that you would consume the `Arc<T>` by
value, thus guaranteeing that no references (and thus no `&WithRef<T>`) exist.
So I think this would still be fine.

-- 
Cheers,
Benno

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 09:14:50AM +0000, Benno Lossin wrote:
> On 25.09.23 08:29, Alice Ryhl wrote:
> > On Sat, Sep 23, 2023 at 4:50 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> >>
> >> From: Wedson Almeida Filho <walmeida@microsoft.com>
> >>
> >> With GATs, we don't need a separate type to represent a borrowed object
> >> with a refcount, we can just use Rust's regular shared borrowing. In
> >> this case, we use `&WithRef<T>` instead of `ArcBorrow<'_, T>`.
> >>
> >> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> >> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> >> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> >> ---
> >>   rust/kernel/sync.rs     |   2 +-
> >>   rust/kernel/sync/arc.rs | 134 ++++++++++++----------------------------
> >>   2 files changed, 39 insertions(+), 97 deletions(-)
> > 
> > I'm concerned about this change, because an `&WithRef<T>` only has
> > immutable permissions for the allocation. No pointer derived from it
> > may be used to modify the value in the Arc, however, the drop
> > implementation of Arc will do exactly that. 
> 
> That is indeed a problem. We could put the value in an `UnsafeCell`, but
> that would lose us niche optimizations and probably also other optimizations.
> 

Not sure I understand the problem here, why do we allow modifying the
value in the Arc if you only have a shared ownership? Also I fail to see
why `ArcBorrow` doesn't have the problem. Maybe I'm missing something
subtle here? Could you provide an example?

Regards,
Boqun

> > It also means that we
> > can't convert an Arc with refcount 1 into a UniqueArc.
> 
> I think you still can, since to do that you would consume the `Arc<T>` by
> value, thus guaranteeing that no references (and thus no `&WithRef<T>`) exist.
> So I think this would still be fine.
> 
> -- 
> Cheers,
> Benno
> 
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Alice Ryhl]

>>> I'm concerned about this change, because an `&WithRef<T>` only has
>>> immutable permissions for the allocation. No pointer derived from it
>>> may be used to modify the value in the Arc, however, the drop
>>> implementation of Arc will do exactly that. 
>> 
>> That is indeed a problem. We could put the value in an `UnsafeCell`, but
>> that would lose us niche optimizations and probably also other optimizations.
>> 
> 
> Not sure I understand the problem here, why do we allow modifying the
> value in the Arc if you only have a shared ownership?

Well, usually it's when you have exclusive access even though the value
is in an `Arc`.

The main example of this is the destructor of the `Arc`. When the last
refcount drops to zero, this gives you exclusive access. This lets you
run the destructor. The destructor requires mutable access.

Another example would be converting the `Arc` back into an `UniqueArc`
by checking that the refcount is 1. Once you have a `UniqueArc`, you can
use it to mutate the inner value.

Finally, there are methods like `Arc::get_mut_unchecked`, where you
unsafely assert that nobody else is using the value while you are
modifying it. We don't have that in our version of `Arc` right now, but
we might want to add it later.

> Also I fail to see why `ArcBorrow` doesn't have the problem. Maybe I'm
> missing something subtle here? Could you provide an example?

It's because `ArcBorrow` just has a raw pointer inside it. Immutable
references give up write permissions, but raw pointers don't even if
they are `*const T`.

Alice

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 03:00:47PM +0000, Alice Ryhl wrote:
> >>> I'm concerned about this change, because an `&WithRef<T>` only has
> >>> immutable permissions for the allocation. No pointer derived from it
> >>> may be used to modify the value in the Arc, however, the drop
> >>> implementation of Arc will do exactly that. 
> >> 
> >> That is indeed a problem. We could put the value in an `UnsafeCell`, but
> >> that would lose us niche optimizations and probably also other optimizations.
> >> 
> > 
> > Not sure I understand the problem here, why do we allow modifying the
> > value in the Arc if you only have a shared ownership?
> 
> Well, usually it's when you have exclusive access even though the value
> is in an `Arc`.
> 
> The main example of this is the destructor of the `Arc`. When the last
> refcount drops to zero, this gives you exclusive access. This lets you
> run the destructor. The destructor requires mutable access.
> 
> Another example would be converting the `Arc` back into an `UniqueArc`
> by checking that the refcount is 1. Once you have a `UniqueArc`, you can
> use it to mutate the inner value.
> 
> Finally, there are methods like `Arc::get_mut_unchecked`, where you
> unsafely assert that nobody else is using the value while you are
> modifying it. We don't have that in our version of `Arc` right now, but
> we might want to add it later.
> 

Hmm.. but the only way to get an `Arc` from `&WithRef` is

	impl From<&WithRef<T>> for Arc<T> {
	    ...
	}

, and we clone `Arc` in the that function (i.e. copying the raw
pointer), so we are still good?

Regards,
Boqun

> > Also I fail to see why `ArcBorrow` doesn't have the problem. Maybe I'm
> > missing something subtle here? Could you provide an example?
> 
> It's because `ArcBorrow` just has a raw pointer inside it. Immutable
> references give up write permissions, but raw pointers don't even if
> they are `*const T`.
> 
> Alice

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Alice Ryhl]

On 9/25/23 17:17, Boqun Feng wrote:
> On Mon, Sep 25, 2023 at 03:00:47PM +0000, Alice Ryhl wrote:
>>>>> I'm concerned about this change, because an `&WithRef<T>` only has
>>>>> immutable permissions for the allocation. No pointer derived from it
>>>>> may be used to modify the value in the Arc, however, the drop
>>>>> implementation of Arc will do exactly that.
>>>>
>>>> That is indeed a problem. We could put the value in an `UnsafeCell`, but
>>>> that would lose us niche optimizations and probably also other optimizations.
>>>>
>>>
>>> Not sure I understand the problem here, why do we allow modifying the
>>> value in the Arc if you only have a shared ownership?
>>
>> Well, usually it's when you have exclusive access even though the value
>> is in an `Arc`.
>>
>> The main example of this is the destructor of the `Arc`. When the last
>> refcount drops to zero, this gives you exclusive access. This lets you
>> run the destructor. The destructor requires mutable access.
>>
>> Another example would be converting the `Arc` back into an `UniqueArc`
>> by checking that the refcount is 1. Once you have a `UniqueArc`, you can
>> use it to mutate the inner value.
>>
>> Finally, there are methods like `Arc::get_mut_unchecked`, where you
>> unsafely assert that nobody else is using the value while you are
>> modifying it. We don't have that in our version of `Arc` right now, but
>> we might want to add it later.
>>
> 
> Hmm.. but the only way to get an `Arc` from `&WithRef` is
> 
> 	impl From<&WithRef<T>> for Arc<T> {
> 	    ...
> 	}
> 
> , and we clone `Arc` in the that function (i.e. copying the raw
> pointer), so we are still good?
> 

No, the raw pointer in the Arc was created from the immutable reference, 
so the raw pointer has the same restrictions as the immutable reference did.

Alice

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 05:30:05PM +0200, Alice Ryhl wrote:
> On 9/25/23 17:17, Boqun Feng wrote:
> > On Mon, Sep 25, 2023 at 03:00:47PM +0000, Alice Ryhl wrote:
> > > > > > I'm concerned about this change, because an `&WithRef<T>` only has
> > > > > > immutable permissions for the allocation. No pointer derived from it
> > > > > > may be used to modify the value in the Arc, however, the drop
> > > > > > implementation of Arc will do exactly that.
> > > > > 
> > > > > That is indeed a problem. We could put the value in an `UnsafeCell`, but
> > > > > that would lose us niche optimizations and probably also other optimizations.
> > > > > 
> > > > 
> > > > Not sure I understand the problem here, why do we allow modifying the
> > > > value in the Arc if you only have a shared ownership?
> > > 
> > > Well, usually it's when you have exclusive access even though the value
> > > is in an `Arc`.
> > > 
> > > The main example of this is the destructor of the `Arc`. When the last
> > > refcount drops to zero, this gives you exclusive access. This lets you
> > > run the destructor. The destructor requires mutable access.
> > > 
> > > Another example would be converting the `Arc` back into an `UniqueArc`
> > > by checking that the refcount is 1. Once you have a `UniqueArc`, you can
> > > use it to mutate the inner value.
> > > 
> > > Finally, there are methods like `Arc::get_mut_unchecked`, where you
> > > unsafely assert that nobody else is using the value while you are
> > > modifying it. We don't have that in our version of `Arc` right now, but
> > > we might want to add it later.
> > > 
> > 
> > Hmm.. but the only way to get an `Arc` from `&WithRef` is
> > 
> > 	impl From<&WithRef<T>> for Arc<T> {
> > 	    ...
> > 	}
> > 
> > , and we clone `Arc` in the that function (i.e. copying the raw
> > pointer), so we are still good?
> > 
> 
> No, the raw pointer in the Arc was created from the immutable reference, so
> the raw pointer has the same restrictions as the immutable reference did.
> 

I see, this was the part I was missing. Thanks!

Looks like the only fix is replacing `&WithRef<T>` with
`&UnsafeCell<WithRef<T>>`? But that's a bit wordy and I'm not sure
whether it's better than `ArcBorrow<'_, T>`...

Regards,
Boqun

> Alice

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Benno Lossin]

On 25.09.23 18:02, Boqun Feng wrote:
> On Mon, Sep 25, 2023 at 05:30:05PM +0200, Alice Ryhl wrote:
>> On 9/25/23 17:17, Boqun Feng wrote:
>>> On Mon, Sep 25, 2023 at 03:00:47PM +0000, Alice Ryhl wrote:
>>>>>>> I'm concerned about this change, because an `&WithRef<T>` only has
>>>>>>> immutable permissions for the allocation. No pointer derived from it
>>>>>>> may be used to modify the value in the Arc, however, the drop
>>>>>>> implementation of Arc will do exactly that.
>>>>>>
>>>>>> That is indeed a problem. We could put the value in an `UnsafeCell`, but
>>>>>> that would lose us niche optimizations and probably also other optimizations.
>>>>>>
>>>>>
>>>>> Not sure I understand the problem here, why do we allow modifying the
>>>>> value in the Arc if you only have a shared ownership?
>>>>
>>>> Well, usually it's when you have exclusive access even though the value
>>>> is in an `Arc`.
>>>>
>>>> The main example of this is the destructor of the `Arc`. When the last
>>>> refcount drops to zero, this gives you exclusive access. This lets you
>>>> run the destructor. The destructor requires mutable access.
>>>>
>>>> Another example would be converting the `Arc` back into an `UniqueArc`
>>>> by checking that the refcount is 1. Once you have a `UniqueArc`, you can
>>>> use it to mutate the inner value.
>>>>
>>>> Finally, there are methods like `Arc::get_mut_unchecked`, where you
>>>> unsafely assert that nobody else is using the value while you are
>>>> modifying it. We don't have that in our version of `Arc` right now, but
>>>> we might want to add it later.
>>>>
>>>
>>> Hmm.. but the only way to get an `Arc` from `&WithRef` is
>>>
>>> 	impl From<&WithRef<T>> for Arc<T> {
>>> 	    ...
>>> 	}
>>>
>>> , and we clone `Arc` in the that function (i.e. copying the raw
>>> pointer), so we are still good?
>>>
>>
>> No, the raw pointer in the Arc was created from the immutable reference, so
>> the raw pointer has the same restrictions as the immutable reference did.
>>
> 
> I see, this was the part I was missing. Thanks!
> 
> Looks like the only fix is replacing `&WithRef<T>` with
> `&UnsafeCell<WithRef<T>>`? But that's a bit wordy and I'm not sure
> whether it's better than `ArcBorrow<'_, T>`...

It should be sufficient to change only the type of the field
within `WithRef`, so no visible API change needed.

--
Cheers,
Benno

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Benno Lossin]

On 25.09.23 16:49, Boqun Feng wrote:
> On Mon, Sep 25, 2023 at 09:14:50AM +0000, Benno Lossin wrote:
>> On 25.09.23 08:29, Alice Ryhl wrote:
>>> On Sat, Sep 23, 2023 at 4:50 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>>>>
>>>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>>>>
>>>> With GATs, we don't need a separate type to represent a borrowed object
>>>> with a refcount, we can just use Rust's regular shared borrowing. In
>>>> this case, we use `&WithRef<T>` instead of `ArcBorrow<'_, T>`.
>>>>
>>>> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
>>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>>>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>>>> ---
>>>>    rust/kernel/sync.rs     |   2 +-
>>>>    rust/kernel/sync/arc.rs | 134 ++++++++++++----------------------------
>>>>    2 files changed, 39 insertions(+), 97 deletions(-)
>>>
>>> I'm concerned about this change, because an `&WithRef<T>` only has
>>> immutable permissions for the allocation. No pointer derived from it
>>> may be used to modify the value in the Arc, however, the drop
>>> implementation of Arc will do exactly that.
>>
>> That is indeed a problem. We could put the value in an `UnsafeCell`, but
>> that would lose us niche optimizations and probably also other optimizations.
>>
> 
> Not sure I understand the problem here, why do we allow modifying the
> value in the Arc if you only have a shared ownership? Also I fail to see
> why `ArcBorrow` doesn't have the problem. Maybe I'm missing something
> subtle here? Could you provide an example?

Sure, here is the problem:

```rust
struct MutatingDrop {
     value: i32,
}

impl Drop for MutatingDrop {
     fn drop(&mut self) {
         self.value = 0;
     }
}

let arc = Arc::new(MutatingDrop { value: 42 });
let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
drop(arc); // this decrements the reference count to 1
drop(arc2); // this decrements the reference count to 0, so it will drop it
```
When dropping `arc2` it will run the destructor for `MutatingDrop`,
which mutates `value`. This is a problem, because the mutable reference
supplied was derived from a `&`, that is not allowed in Rust.

-- 
Cheers,
Benno

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
> On 25.09.23 16:49, Boqun Feng wrote:
> > On Mon, Sep 25, 2023 at 09:14:50AM +0000, Benno Lossin wrote:
> >> On 25.09.23 08:29, Alice Ryhl wrote:
> >>> On Sat, Sep 23, 2023 at 4:50 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> >>>>
> >>>> From: Wedson Almeida Filho <walmeida@microsoft.com>
> >>>>
> >>>> With GATs, we don't need a separate type to represent a borrowed object
> >>>> with a refcount, we can just use Rust's regular shared borrowing. In
> >>>> this case, we use `&WithRef<T>` instead of `ArcBorrow<'_, T>`.
> >>>>
> >>>> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> >>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> >>>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> >>>> ---
> >>>>    rust/kernel/sync.rs     |   2 +-
> >>>>    rust/kernel/sync/arc.rs | 134 ++++++++++++----------------------------
> >>>>    2 files changed, 39 insertions(+), 97 deletions(-)
> >>>
> >>> I'm concerned about this change, because an `&WithRef<T>` only has
> >>> immutable permissions for the allocation. No pointer derived from it
> >>> may be used to modify the value in the Arc, however, the drop
> >>> implementation of Arc will do exactly that.
> >>
> >> That is indeed a problem. We could put the value in an `UnsafeCell`, but
> >> that would lose us niche optimizations and probably also other optimizations.
> >>
> > 
> > Not sure I understand the problem here, why do we allow modifying the
> > value in the Arc if you only have a shared ownership? Also I fail to see
> > why `ArcBorrow` doesn't have the problem. Maybe I'm missing something
> > subtle here? Could you provide an example?
> 
> Sure, here is the problem:
> 

Thanks, Benno.

> ```rust
> struct MutatingDrop {
>      value: i32,
> }
> 
> impl Drop for MutatingDrop {
>      fn drop(&mut self) {
>          self.value = 0;
>      }
> }
> 
> let arc = Arc::new(MutatingDrop { value: 42 });
> let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
> let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2

More precisely, here we did a

	&WithRef<_> -> NonNull<WithRef<_>>

conversion, and later on, we may use the `NonNull<WithRef<_>>` in
`drop` to get a `Box<WithRef<_>>`.

> drop(arc); // this decrements the reference count to 1
> drop(arc2); // this decrements the reference count to 0, so it will drop it
> ```
> When dropping `arc2` it will run the destructor for `MutatingDrop`,
> which mutates `value`. This is a problem, because the mutable reference
> supplied was derived from a `&`, that is not allowed in Rust.
> 

Is this an UB? I kinda wonder what's the real damage we can get, because
in this case, we just use a reference to carry a value of a pointer,
i.e.

	ptr -> reference -> ptr

I cannot think of any real damage compiler can make, but I'm happy to be
surprised ;-)

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Benno Lossin]

On 25.09.23 18:16, Boqun Feng wrote:
> On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
>> ```rust
>> struct MutatingDrop {
>>       value: i32,
>> }
>>
>> impl Drop for MutatingDrop {
>>       fn drop(&mut self) {
>>           self.value = 0;
>>       }
>> }
>>
>> let arc = Arc::new(MutatingDrop { value: 42 });
>> let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
>> let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
> 
> More precisely, here we did a
> 
> 	&WithRef<_> -> NonNull<WithRef<_>>
> 
> conversion, and later on, we may use the `NonNull<WithRef<_>>` in
> `drop` to get a `Box<WithRef<_>>`.

Indeed.

> 
>> drop(arc); // this decrements the reference count to 1
>> drop(arc2); // this decrements the reference count to 0, so it will drop it
>> ```
>> When dropping `arc2` it will run the destructor for `MutatingDrop`,
>> which mutates `value`. This is a problem, because the mutable reference
>> supplied was derived from a `&`, that is not allowed in Rust.
>>
> 
> Is this an UB? I kinda wonder what's the real damage we can get, because
> in this case, we just use a reference to carry a value of a pointer,
> i.e.
> 
> 	ptr -> reference -> ptr
> 
> I cannot think of any real damage compiler can make, but I'm happy to be
> surprised ;-)

This is UB, so anything can happen :)

-- 
Cheers,
Benno

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 05:00:45PM +0000, Benno Lossin wrote:
> On 25.09.23 18:16, Boqun Feng wrote:
> > On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
> >> ```rust
> >> struct MutatingDrop {
> >>       value: i32,
> >> }
> >>
> >> impl Drop for MutatingDrop {
> >>       fn drop(&mut self) {
> >>           self.value = 0;
> >>       }
> >> }
> >>
> >> let arc = Arc::new(MutatingDrop { value: 42 });
> >> let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
> >> let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
> > 
> > More precisely, here we did a
> > 
> > 	&WithRef<_> -> NonNull<WithRef<_>>
> > 
> > conversion, and later on, we may use the `NonNull<WithRef<_>>` in
> > `drop` to get a `Box<WithRef<_>>`.
> 
> Indeed.
> 

Can we workaround this issue by (ab)using the `UnsafeCell` inside
`WithRef<T>`?

impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
    fn from(b: &WithRef<T>) -> Self {
        // SAFETY: The existence of the references proves that
	// `b.refcount.get()` is a valid pointer to `WithRef<T>`.
	let ptr = unsafe { NonNull::new_unchecked(b.refcount.get().cast::<WithRef<T>>()) };

	// SAFETY: see the SAFETY above `let ptr = ..` line.
        ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
            .deref()
            .clone()
    }
}

This way, the raw pointer in the new Arc no longer derives from the
reference of `WithRef<T>`.

Regards,
Boqun

> > 
> >> drop(arc); // this decrements the reference count to 1
> >> drop(arc2); // this decrements the reference count to 0, so it will drop it
> >> ```
> >> When dropping `arc2` it will run the destructor for `MutatingDrop`,
> >> which mutates `value`. This is a problem, because the mutable reference
> >> supplied was derived from a `&`, that is not allowed in Rust.
> >>
> > 
> > Is this an UB? I kinda wonder what's the real damage we can get, because
> > in this case, we just use a reference to carry a value of a pointer,
> > i.e.
> > 
> > 	ptr -> reference -> ptr
> > 
> > I cannot think of any real damage compiler can make, but I'm happy to be
> > surprised ;-)
> 
> This is UB, so anything can happen :)
> 
> -- 
> Cheers,
> Benno
> 
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Benno Lossin]

On 25.09.23 20:51, Boqun Feng wrote:
> On Mon, Sep 25, 2023 at 05:00:45PM +0000, Benno Lossin wrote:
>> On 25.09.23 18:16, Boqun Feng wrote:
>>> On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
>>>> ```rust
>>>> struct MutatingDrop {
>>>>        value: i32,
>>>> }
>>>>
>>>> impl Drop for MutatingDrop {
>>>>        fn drop(&mut self) {
>>>>            self.value = 0;
>>>>        }
>>>> }
>>>>
>>>> let arc = Arc::new(MutatingDrop { value: 42 });
>>>> let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
>>>> let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
>>>
>>> More precisely, here we did a
>>>
>>> 	&WithRef<_> -> NonNull<WithRef<_>>
>>>
>>> conversion, and later on, we may use the `NonNull<WithRef<_>>` in
>>> `drop` to get a `Box<WithRef<_>>`.
>>
>> Indeed.
>>
> 
> Can we workaround this issue by (ab)using the `UnsafeCell` inside
> `WithRef<T>`?
> 
> impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
>      fn from(b: &WithRef<T>) -> Self {
>          // SAFETY: The existence of the references proves that
> 	// `b.refcount.get()` is a valid pointer to `WithRef<T>`.
> 	let ptr = unsafe { NonNull::new_unchecked(b.refcount.get().cast::<WithRef<T>>()) };
> 
> 	// SAFETY: see the SAFETY above `let ptr = ..` line.
>          ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
>              .deref()
>              .clone()
>      }
> }
> 
> This way, the raw pointer in the new Arc no longer derives from the
> reference of `WithRef<T>`.

No, the code above only obtains a pointer that has provenance valid
for a `bindings::refcount_t` (or type with the same layout, such as
`Opaque<bindings::refcount_t>`). But not the whole `WithRef<T>`, so accessing
it by reading/writing will still be UB.

-- 
Cheers,
Benno

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 09:03:52PM +0000, Benno Lossin wrote:
> On 25.09.23 20:51, Boqun Feng wrote:
> > On Mon, Sep 25, 2023 at 05:00:45PM +0000, Benno Lossin wrote:
> >> On 25.09.23 18:16, Boqun Feng wrote:
> >>> On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
> >>>> ```rust
> >>>> struct MutatingDrop {
> >>>>        value: i32,
> >>>> }
> >>>>
> >>>> impl Drop for MutatingDrop {
> >>>>        fn drop(&mut self) {
> >>>>            self.value = 0;
> >>>>        }
> >>>> }
> >>>>
> >>>> let arc = Arc::new(MutatingDrop { value: 42 });
> >>>> let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
> >>>> let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
> >>>
> >>> More precisely, here we did a
> >>>
> >>> 	&WithRef<_> -> NonNull<WithRef<_>>
> >>>
> >>> conversion, and later on, we may use the `NonNull<WithRef<_>>` in
> >>> `drop` to get a `Box<WithRef<_>>`.
> >>
> >> Indeed.
> >>
> > 
> > Can we workaround this issue by (ab)using the `UnsafeCell` inside
> > `WithRef<T>`?
> > 
> > impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> >      fn from(b: &WithRef<T>) -> Self {
> >          // SAFETY: The existence of the references proves that
> > 	// `b.refcount.get()` is a valid pointer to `WithRef<T>`.
> > 	let ptr = unsafe { NonNull::new_unchecked(b.refcount.get().cast::<WithRef<T>>()) };
> > 
> > 	// SAFETY: see the SAFETY above `let ptr = ..` line.
> >          ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
> >              .deref()
> >              .clone()
> >      }
> > }
> > 
> > This way, the raw pointer in the new Arc no longer derives from the
> > reference of `WithRef<T>`.
> 
> No, the code above only obtains a pointer that has provenance valid
> for a `bindings::refcount_t` (or type with the same layout, such as
> `Opaque<bindings::refcount_t>`). But not the whole `WithRef<T>`, so accessing
> it by reading/writing will still be UB.
> 

Hmm... but we do the similar thing in `Arc::from_raw()`, right?

    	pub unsafe fn from_raw(ptr: *const T) -> Self {
	    ..
	}

, what we have is a pointer to T, and we construct a pointer to
`ArcInner<T>/WithRef<T>`, in that function. Because the `sub` on pointer
gets away from provenance? If so, we can also do a sub(0) in the above
code.

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Alice Ryhl]

On 9/25/23 23:55, Boqun Feng wrote:
> On Mon, Sep 25, 2023 at 09:03:52PM +0000, Benno Lossin wrote:
>> On 25.09.23 20:51, Boqun Feng wrote:
>>> On Mon, Sep 25, 2023 at 05:00:45PM +0000, Benno Lossin wrote:
>>>> On 25.09.23 18:16, Boqun Feng wrote:
>>>>> On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
>>>>>> ```rust
>>>>>> struct MutatingDrop {
>>>>>>         value: i32,
>>>>>> }
>>>>>>
>>>>>> impl Drop for MutatingDrop {
>>>>>>         fn drop(&mut self) {
>>>>>>             self.value = 0;
>>>>>>         }
>>>>>> }
>>>>>>
>>>>>> let arc = Arc::new(MutatingDrop { value: 42 });
>>>>>> let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
>>>>>> let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
>>>>>
>>>>> More precisely, here we did a
>>>>>
>>>>> 	&WithRef<_> -> NonNull<WithRef<_>>
>>>>>
>>>>> conversion, and later on, we may use the `NonNull<WithRef<_>>` in
>>>>> `drop` to get a `Box<WithRef<_>>`.
>>>>
>>>> Indeed.
>>>>
>>>
>>> Can we workaround this issue by (ab)using the `UnsafeCell` inside
>>> `WithRef<T>`?
>>>
>>> impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
>>>       fn from(b: &WithRef<T>) -> Self {
>>>           // SAFETY: The existence of the references proves that
>>> 	// `b.refcount.get()` is a valid pointer to `WithRef<T>`.
>>> 	let ptr = unsafe { NonNull::new_unchecked(b.refcount.get().cast::<WithRef<T>>()) };
>>>
>>> 	// SAFETY: see the SAFETY above `let ptr = ..` line.
>>>           ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
>>>               .deref()
>>>               .clone()
>>>       }
>>> }
>>>
>>> This way, the raw pointer in the new Arc no longer derives from the
>>> reference of `WithRef<T>`.
>>
>> No, the code above only obtains a pointer that has provenance valid
>> for a `bindings::refcount_t` (or type with the same layout, such as
>> `Opaque<bindings::refcount_t>`). But not the whole `WithRef<T>`, so accessing
>> it by reading/writing will still be UB.
>>
> 
> Hmm... but we do the similar thing in `Arc::from_raw()`, right?
> 
>      	pub unsafe fn from_raw(ptr: *const T) -> Self {
> 	    ..
> 	}
> 
> , what we have is a pointer to T, and we construct a pointer to
> `ArcInner<T>/WithRef<T>`, in that function. Because the `sub` on pointer
> gets away from provenance? If so, we can also do a sub(0) in the above
> code.

Not sure what you mean. Operations on raw pointers leave provenance 
unchanged.

Alice

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 11:58:46PM +0200, Alice Ryhl wrote:
> On 9/25/23 23:55, Boqun Feng wrote:
> > On Mon, Sep 25, 2023 at 09:03:52PM +0000, Benno Lossin wrote:
> > > On 25.09.23 20:51, Boqun Feng wrote:
> > > > On Mon, Sep 25, 2023 at 05:00:45PM +0000, Benno Lossin wrote:
> > > > > On 25.09.23 18:16, Boqun Feng wrote:
> > > > > > On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
> > > > > > > ```rust
> > > > > > > struct MutatingDrop {
> > > > > > >         value: i32,
> > > > > > > }
> > > > > > > 
> > > > > > > impl Drop for MutatingDrop {
> > > > > > >         fn drop(&mut self) {
> > > > > > >             self.value = 0;
> > > > > > >         }
> > > > > > > }
> > > > > > > 
> > > > > > > let arc = Arc::new(MutatingDrop { value: 42 });
> > > > > > > let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
> > > > > > > let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
> > > > > > 
> > > > > > More precisely, here we did a
> > > > > > 
> > > > > > 	&WithRef<_> -> NonNull<WithRef<_>>
> > > > > > 
> > > > > > conversion, and later on, we may use the `NonNull<WithRef<_>>` in
> > > > > > `drop` to get a `Box<WithRef<_>>`.
> > > > > 
> > > > > Indeed.
> > > > > 
> > > > 
> > > > Can we workaround this issue by (ab)using the `UnsafeCell` inside
> > > > `WithRef<T>`?
> > > > 
> > > > impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> > > >       fn from(b: &WithRef<T>) -> Self {
> > > >           // SAFETY: The existence of the references proves that
> > > > 	// `b.refcount.get()` is a valid pointer to `WithRef<T>`.
> > > > 	let ptr = unsafe { NonNull::new_unchecked(b.refcount.get().cast::<WithRef<T>>()) };
> > > > 
> > > > 	// SAFETY: see the SAFETY above `let ptr = ..` line.
> > > >           ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
> > > >               .deref()
> > > >               .clone()
> > > >       }
> > > > }
> > > > 
> > > > This way, the raw pointer in the new Arc no longer derives from the
> > > > reference of `WithRef<T>`.
> > > 
> > > No, the code above only obtains a pointer that has provenance valid
> > > for a `bindings::refcount_t` (or type with the same layout, such as
> > > `Opaque<bindings::refcount_t>`). But not the whole `WithRef<T>`, so accessing
> > > it by reading/writing will still be UB.
> > > 
> > 
> > Hmm... but we do the similar thing in `Arc::from_raw()`, right?
> > 
> >      	pub unsafe fn from_raw(ptr: *const T) -> Self {
> > 	    ..
> > 	}
> > 
> > , what we have is a pointer to T, and we construct a pointer to
> > `ArcInner<T>/WithRef<T>`, in that function. Because the `sub` on pointer
> > gets away from provenance? If so, we can also do a sub(0) in the above
> > code.
> 
> Not sure what you mean. Operations on raw pointers leave provenance
> unchanged.

Let's look at the function from_raw(), the input is a pointer to T,
right? So you only have the provenance to T, but in that function, the
pointer is casted to a pointer to WithRef<T>/ArcInner<T>, that means you
have the provenance to the whole WithRef<T>/ArcInner<T>, right? My
question is: why isn't that a UB?

Regards,
Boqun

> 
> Alice
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 03:02:27PM -0700, Boqun Feng wrote:
> On Mon, Sep 25, 2023 at 11:58:46PM +0200, Alice Ryhl wrote:
> > On 9/25/23 23:55, Boqun Feng wrote:
> > > On Mon, Sep 25, 2023 at 09:03:52PM +0000, Benno Lossin wrote:
> > > > On 25.09.23 20:51, Boqun Feng wrote:
> > > > > On Mon, Sep 25, 2023 at 05:00:45PM +0000, Benno Lossin wrote:
> > > > > > On 25.09.23 18:16, Boqun Feng wrote:
> > > > > > > On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
> > > > > > > > ```rust
> > > > > > > > struct MutatingDrop {
> > > > > > > >         value: i32,
> > > > > > > > }
> > > > > > > > 
> > > > > > > > impl Drop for MutatingDrop {
> > > > > > > >         fn drop(&mut self) {
> > > > > > > >             self.value = 0;
> > > > > > > >         }
> > > > > > > > }
> > > > > > > > 
> > > > > > > > let arc = Arc::new(MutatingDrop { value: 42 });
> > > > > > > > let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
> > > > > > > > let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
> > > > > > > 
> > > > > > > More precisely, here we did a
> > > > > > > 
> > > > > > > 	&WithRef<_> -> NonNull<WithRef<_>>
> > > > > > > 
> > > > > > > conversion, and later on, we may use the `NonNull<WithRef<_>>` in
> > > > > > > `drop` to get a `Box<WithRef<_>>`.
> > > > > > 
> > > > > > Indeed.
> > > > > > 
> > > > > 
> > > > > Can we workaround this issue by (ab)using the `UnsafeCell` inside
> > > > > `WithRef<T>`?
> > > > > 
> > > > > impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> > > > >       fn from(b: &WithRef<T>) -> Self {
> > > > >           // SAFETY: The existence of the references proves that
> > > > > 	// `b.refcount.get()` is a valid pointer to `WithRef<T>`.
> > > > > 	let ptr = unsafe { NonNull::new_unchecked(b.refcount.get().cast::<WithRef<T>>()) };
> > > > > 
> > > > > 	// SAFETY: see the SAFETY above `let ptr = ..` line.
> > > > >           ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
> > > > >               .deref()
> > > > >               .clone()
> > > > >       }
> > > > > }
> > > > > 
> > > > > This way, the raw pointer in the new Arc no longer derives from the
> > > > > reference of `WithRef<T>`.
> > > > 
> > > > No, the code above only obtains a pointer that has provenance valid
> > > > for a `bindings::refcount_t` (or type with the same layout, such as
> > > > `Opaque<bindings::refcount_t>`). But not the whole `WithRef<T>`, so accessing
> > > > it by reading/writing will still be UB.
> > > > 
> > > 
> > > Hmm... but we do the similar thing in `Arc::from_raw()`, right?
> > > 
> > >      	pub unsafe fn from_raw(ptr: *const T) -> Self {
> > > 	    ..
> > > 	}
> > > 
> > > , what we have is a pointer to T, and we construct a pointer to
> > > `ArcInner<T>/WithRef<T>`, in that function. Because the `sub` on pointer
> > > gets away from provenance? If so, we can also do a sub(0) in the above
> > > code.
> > 
> > Not sure what you mean. Operations on raw pointers leave provenance
> > unchanged.
> 
> Let's look at the function from_raw(), the input is a pointer to T,
> right? So you only have the provenance to T, but in that function, the
> pointer is casted to a pointer to WithRef<T>/ArcInner<T>, that means you
> have the provenance to the whole WithRef<T>/ArcInner<T>, right? My
> question is: why isn't that a UB?
> 

Or how does it get away from provenance checkings?

Regards,
Boqun

> Regards,
> Boqun
> 
> > 
> > Alice
> > 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Benno Lossin]

On 26.09.23 00:02, Boqun Feng wrote:
> On Mon, Sep 25, 2023 at 11:58:46PM +0200, Alice Ryhl wrote:
>> On 9/25/23 23:55, Boqun Feng wrote:
>>> On Mon, Sep 25, 2023 at 09:03:52PM +0000, Benno Lossin wrote:
>>>> On 25.09.23 20:51, Boqun Feng wrote:
>>>>> On Mon, Sep 25, 2023 at 05:00:45PM +0000, Benno Lossin wrote:
>>>>>> On 25.09.23 18:16, Boqun Feng wrote:
>>>>>>> On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
>>>>>>>> ```rust
>>>>>>>> struct MutatingDrop {
>>>>>>>>          value: i32,
>>>>>>>> }
>>>>>>>>
>>>>>>>> impl Drop for MutatingDrop {
>>>>>>>>          fn drop(&mut self) {
>>>>>>>>              self.value = 0;
>>>>>>>>          }
>>>>>>>> }
>>>>>>>>
>>>>>>>> let arc = Arc::new(MutatingDrop { value: 42 });
>>>>>>>> let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
>>>>>>>> let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
>>>>>>>
>>>>>>> More precisely, here we did a
>>>>>>>
>>>>>>> 	&WithRef<_> -> NonNull<WithRef<_>>
>>>>>>>
>>>>>>> conversion, and later on, we may use the `NonNull<WithRef<_>>` in
>>>>>>> `drop` to get a `Box<WithRef<_>>`.
>>>>>>
>>>>>> Indeed.
>>>>>>
>>>>>
>>>>> Can we workaround this issue by (ab)using the `UnsafeCell` inside
>>>>> `WithRef<T>`?
>>>>>
>>>>> impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
>>>>>        fn from(b: &WithRef<T>) -> Self {
>>>>>            // SAFETY: The existence of the references proves that
>>>>> 	// `b.refcount.get()` is a valid pointer to `WithRef<T>`.
>>>>> 	let ptr = unsafe { NonNull::new_unchecked(b.refcount.get().cast::<WithRef<T>>()) };
>>>>>
>>>>> 	// SAFETY: see the SAFETY above `let ptr = ..` line.
>>>>>            ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
>>>>>                .deref()
>>>>>                .clone()
>>>>>        }
>>>>> }
>>>>>
>>>>> This way, the raw pointer in the new Arc no longer derives from the
>>>>> reference of `WithRef<T>`.
>>>>
>>>> No, the code above only obtains a pointer that has provenance valid
>>>> for a `bindings::refcount_t` (or type with the same layout, such as
>>>> `Opaque<bindings::refcount_t>`). But not the whole `WithRef<T>`, so accessing
>>>> it by reading/writing will still be UB.
>>>>
>>>
>>> Hmm... but we do the similar thing in `Arc::from_raw()`, right?
>>>
>>>       	pub unsafe fn from_raw(ptr: *const T) -> Self {
>>> 	    ..
>>> 	}
>>>
>>> , what we have is a pointer to T, and we construct a pointer to
>>> `ArcInner<T>/WithRef<T>`, in that function. Because the `sub` on pointer
>>> gets away from provenance? If so, we can also do a sub(0) in the above
>>> code.
>>
>> Not sure what you mean. Operations on raw pointers leave provenance
>> unchanged.
> 
> Let's look at the function from_raw(), the input is a pointer to T,
> right? So you only have the provenance to T, but in that function, the
> pointer is casted to a pointer to WithRef<T>/ArcInner<T>, that means you
> have the provenance to the whole WithRef<T>/ArcInner<T>, right? My
> question is: why isn't that a UB?

The pointer was originally derived by a call to `into_raw`:
```
     pub fn into_raw(self) -> *const T {
         let ptr = self.ptr.as_ptr();
         core::mem::forget(self);
         // SAFETY: The pointer is valid.
         unsafe { core::ptr::addr_of!((*ptr).data) }
     }
```
So in this function the origin (also the origin of the provenance)
of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
Raw pointers do not lose this provenance information when you cast
it and when using `addr_of`/`addr_of_mut`. So provenance is something
that is not really represented in the type system for raw pointers.

When doing a round trip through a reference though, the provenance is
newly assigned and thus would only be valid for a `T`:
```
let raw = arc.into_raw();
let reference = unsafe { &*raw };
let raw: *const T = reference;
let arc = unsafe { Arc::from_raw(raw) };
```
Miri would complain about the above code.

-- 
Cheers,
Benno

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 10:26:56PM +0000, Benno Lossin wrote:
> On 26.09.23 00:02, Boqun Feng wrote:
> > On Mon, Sep 25, 2023 at 11:58:46PM +0200, Alice Ryhl wrote:
> >> On 9/25/23 23:55, Boqun Feng wrote:
> >>> On Mon, Sep 25, 2023 at 09:03:52PM +0000, Benno Lossin wrote:
> >>>> On 25.09.23 20:51, Boqun Feng wrote:
> >>>>> On Mon, Sep 25, 2023 at 05:00:45PM +0000, Benno Lossin wrote:
> >>>>>> On 25.09.23 18:16, Boqun Feng wrote:
> >>>>>>> On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:
> >>>>>>>> ```rust
> >>>>>>>> struct MutatingDrop {
> >>>>>>>>          value: i32,
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> impl Drop for MutatingDrop {
> >>>>>>>>          fn drop(&mut self) {
> >>>>>>>>              self.value = 0;
> >>>>>>>>          }
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> let arc = Arc::new(MutatingDrop { value: 42 });
> >>>>>>>> let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
> >>>>>>>> let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2
> >>>>>>>
> >>>>>>> More precisely, here we did a
> >>>>>>>
> >>>>>>> 	&WithRef<_> -> NonNull<WithRef<_>>
> >>>>>>>
> >>>>>>> conversion, and later on, we may use the `NonNull<WithRef<_>>` in
> >>>>>>> `drop` to get a `Box<WithRef<_>>`.
> >>>>>>
> >>>>>> Indeed.
> >>>>>>
> >>>>>
> >>>>> Can we workaround this issue by (ab)using the `UnsafeCell` inside
> >>>>> `WithRef<T>`?
> >>>>>
> >>>>> impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> >>>>>        fn from(b: &WithRef<T>) -> Self {
> >>>>>            // SAFETY: The existence of the references proves that
> >>>>> 	// `b.refcount.get()` is a valid pointer to `WithRef<T>`.
> >>>>> 	let ptr = unsafe { NonNull::new_unchecked(b.refcount.get().cast::<WithRef<T>>()) };
> >>>>>
> >>>>> 	// SAFETY: see the SAFETY above `let ptr = ..` line.
> >>>>>            ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
> >>>>>                .deref()
> >>>>>                .clone()
> >>>>>        }
> >>>>> }
> >>>>>
> >>>>> This way, the raw pointer in the new Arc no longer derives from the
> >>>>> reference of `WithRef<T>`.
> >>>>
> >>>> No, the code above only obtains a pointer that has provenance valid
> >>>> for a `bindings::refcount_t` (or type with the same layout, such as
> >>>> `Opaque<bindings::refcount_t>`). But not the whole `WithRef<T>`, so accessing
> >>>> it by reading/writing will still be UB.
> >>>>
> >>>
> >>> Hmm... but we do the similar thing in `Arc::from_raw()`, right?
> >>>
> >>>       	pub unsafe fn from_raw(ptr: *const T) -> Self {
> >>> 	    ..
> >>> 	}
> >>>
> >>> , what we have is a pointer to T, and we construct a pointer to
> >>> `ArcInner<T>/WithRef<T>`, in that function. Because the `sub` on pointer
> >>> gets away from provenance? If so, we can also do a sub(0) in the above
> >>> code.
> >>
> >> Not sure what you mean. Operations on raw pointers leave provenance
> >> unchanged.
> > 
> > Let's look at the function from_raw(), the input is a pointer to T,
> > right? So you only have the provenance to T, but in that function, the
> > pointer is casted to a pointer to WithRef<T>/ArcInner<T>, that means you
> > have the provenance to the whole WithRef<T>/ArcInner<T>, right? My
> > question is: why isn't that a UB?
> 
> The pointer was originally derived by a call to `into_raw`:
> ```
>      pub fn into_raw(self) -> *const T {
>          let ptr = self.ptr.as_ptr();
>          core::mem::forget(self);
>          // SAFETY: The pointer is valid.
>          unsafe { core::ptr::addr_of!((*ptr).data) }
>      }
> ```
> So in this function the origin (also the origin of the provenance)
> of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
> Raw pointers do not lose this provenance information when you cast
> it and when using `addr_of`/`addr_of_mut`. So provenance is something
> that is not really represented in the type system for raw pointers.

Ah, I see, that's the thing I was missing. Now it makes much sense to
me, thank you both!

> 
> When doing a round trip through a reference though, the provenance is
> newly assigned and thus would only be valid for a `T`:
> ```
> let raw = arc.into_raw();
> let reference = unsafe { &*raw };
> let raw: *const T = reference;
> let arc = unsafe { Arc::from_raw(raw) };
> ```

Agreed. This example demonstrates the key point: the provenances of raw
pointers are decided at derive time.

Regards,
Boqun


> Miri would complain about the above code.
> 
> -- 
> Cheers,
> Benno
> 
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Mon, Sep 25, 2023 at 03:34:56PM -0700, Boqun Feng wrote:
[...]
> > >>>
> > >>> Hmm... but we do the similar thing in `Arc::from_raw()`, right?
> > >>>
> > >>>       	pub unsafe fn from_raw(ptr: *const T) -> Self {
> > >>> 	    ..
> > >>> 	}
> > >>>
> > >>> , what we have is a pointer to T, and we construct a pointer to
> > >>> `ArcInner<T>/WithRef<T>`, in that function. Because the `sub` on pointer
> > >>> gets away from provenance? If so, we can also do a sub(0) in the above
> > >>> code.
> > >>
> > >> Not sure what you mean. Operations on raw pointers leave provenance
> > >> unchanged.
> > > 
> > > Let's look at the function from_raw(), the input is a pointer to T,
> > > right? So you only have the provenance to T, but in that function, the
> > > pointer is casted to a pointer to WithRef<T>/ArcInner<T>, that means you
> > > have the provenance to the whole WithRef<T>/ArcInner<T>, right? My
> > > question is: why isn't that a UB?
> > 
> > The pointer was originally derived by a call to `into_raw`:
> > ```
> >      pub fn into_raw(self) -> *const T {
> >          let ptr = self.ptr.as_ptr();
> >          core::mem::forget(self);
> >          // SAFETY: The pointer is valid.
> >          unsafe { core::ptr::addr_of!((*ptr).data) }
> >      }
> > ```
> > So in this function the origin (also the origin of the provenance)
> > of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
> > Raw pointers do not lose this provenance information when you cast
> > it and when using `addr_of`/`addr_of_mut`. So provenance is something
> > that is not really represented in the type system for raw pointers.
> 
> Ah, I see, that's the thing I was missing. Now it makes much sense to
> me, thank you both!
> 
> > 
> > When doing a round trip through a reference though, the provenance is
> > newly assigned and thus would only be valid for a `T`:
> > ```
> > let raw = arc.into_raw();
> > let reference = unsafe { &*raw };
> > let raw: *const T = reference;
> > let arc = unsafe { Arc::from_raw(raw) };
> > ```
> 
> Agreed. This example demonstrates the key point: the provenances of raw
> pointers are decided at derive time.
> 

So the original problem the Alice brought up is also because of the
provenance, right? To get a `&WithRef<T>`, we reborrow the pointer to
get a `&`, and any pointer derived from that reference will have a
different (and read-only) provenance, which causes the problem. Like:

```rust
let raw = Box::into_raw(arc);
let reference = unsafe { &*raw }; // as_with_ref()
let raw: *mut T = reference as *const _ as  *mut _ ;
let arc = unsafe { Box::from_raw(raw) };
```

Regards,
Boqun

> Regards,
> Boqun
> 
> 
> > Miri would complain about the above code.
> > 
> > -- 
> > Cheers,
> > Benno
> > 
> > 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Gary Guo]

On Mon, 25 Sep 2023 22:26:56 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 26.09.23 00:02, Boqun Feng wrote:
> > On Mon, Sep 25, 2023 at 11:58:46PM +0200, Alice Ryhl wrote:  
> >> On 9/25/23 23:55, Boqun Feng wrote:  
> >>> On Mon, Sep 25, 2023 at 09:03:52PM +0000, Benno Lossin wrote:  
> >>>> On 25.09.23 20:51, Boqun Feng wrote:  
> >>>>> On Mon, Sep 25, 2023 at 05:00:45PM +0000, Benno Lossin wrote:  
> >>>>>> On 25.09.23 18:16, Boqun Feng wrote:  
> >>>>>>> On Mon, Sep 25, 2023 at 03:07:44PM +0000, Benno Lossin wrote:  
> >>>>>>>> ```rust
> >>>>>>>> struct MutatingDrop {
> >>>>>>>>          value: i32,
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> impl Drop for MutatingDrop {
> >>>>>>>>          fn drop(&mut self) {
> >>>>>>>>              self.value = 0;
> >>>>>>>>          }
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> let arc = Arc::new(MutatingDrop { value: 42 });
> >>>>>>>> let wr = arc.as_with_ref(); // this creates a shared `&` reference to the MutatingDrop
> >>>>>>>> let arc2: Arc<MutatingDrop> = wr.into(); // increments the reference count to 2  
> >>>>>>>
> >>>>>>> More precisely, here we did a
> >>>>>>>
> >>>>>>> 	&WithRef<_> -> NonNull<WithRef<_>>
> >>>>>>>
> >>>>>>> conversion, and later on, we may use the `NonNull<WithRef<_>>` in
> >>>>>>> `drop` to get a `Box<WithRef<_>>`.  
> >>>>>>
> >>>>>> Indeed.
> >>>>>>  
> >>>>>
> >>>>> Can we workaround this issue by (ab)using the `UnsafeCell` inside
> >>>>> `WithRef<T>`?
> >>>>>
> >>>>> impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> >>>>>        fn from(b: &WithRef<T>) -> Self {
> >>>>>            // SAFETY: The existence of the references proves that
> >>>>> 	// `b.refcount.get()` is a valid pointer to `WithRef<T>`.
> >>>>> 	let ptr = unsafe { NonNull::new_unchecked(b.refcount.get().cast::<WithRef<T>>()) };
> >>>>>
> >>>>> 	// SAFETY: see the SAFETY above `let ptr = ..` line.
> >>>>>            ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
> >>>>>                .deref()
> >>>>>                .clone()
> >>>>>        }
> >>>>> }
> >>>>>
> >>>>> This way, the raw pointer in the new Arc no longer derives from the
> >>>>> reference of `WithRef<T>`.  
> >>>>
> >>>> No, the code above only obtains a pointer that has provenance valid
> >>>> for a `bindings::refcount_t` (or type with the same layout, such as
> >>>> `Opaque<bindings::refcount_t>`). But not the whole `WithRef<T>`, so accessing
> >>>> it by reading/writing will still be UB.
> >>>>  
> >>>
> >>> Hmm... but we do the similar thing in `Arc::from_raw()`, right?
> >>>
> >>>       	pub unsafe fn from_raw(ptr: *const T) -> Self {
> >>> 	    ..
> >>> 	}
> >>>
> >>> , what we have is a pointer to T, and we construct a pointer to
> >>> `ArcInner<T>/WithRef<T>`, in that function. Because the `sub` on pointer
> >>> gets away from provenance? If so, we can also do a sub(0) in the above
> >>> code.  
> >>
> >> Not sure what you mean. Operations on raw pointers leave provenance
> >> unchanged.  
> > 
> > Let's look at the function from_raw(), the input is a pointer to T,
> > right? So you only have the provenance to T, but in that function, the
> > pointer is casted to a pointer to WithRef<T>/ArcInner<T>, that means you
> > have the provenance to the whole WithRef<T>/ArcInner<T>, right? My
> > question is: why isn't that a UB?  
> 
> The pointer was originally derived by a call to `into_raw`:
> ```
>      pub fn into_raw(self) -> *const T {
>          let ptr = self.ptr.as_ptr();
>          core::mem::forget(self);
>          // SAFETY: The pointer is valid.
>          unsafe { core::ptr::addr_of!((*ptr).data) }
>      }
> ```
> So in this function the origin (also the origin of the provenance)
> of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
> Raw pointers do not lose this provenance information when you cast
> it and when using `addr_of`/`addr_of_mut`. So provenance is something
> that is not really represented in the type system for raw pointers.
> 
> When doing a round trip through a reference though, the provenance is
> newly assigned and thus would only be valid for a `T`:
> ```
> let raw = arc.into_raw();
> let reference = unsafe { &*raw };
> let raw: *const T = reference;
> let arc = unsafe { Arc::from_raw(raw) };
> ```
> Miri would complain about the above code.
> 

One thing we can do is to opt from strict provenance, so:

```
let raw = arc.into_raw();
let _ = raw as usize; // expose the provenance of raw
let reference = unsafe { &*raw };
let raw = reference as *const T as usize as *const T;
let arc = unsafe { Arc::from_raw(raw) };
```

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Tue, Sep 26, 2023 at 04:26:59PM +0800, Gary Guo wrote:
> On Mon, 25 Sep 2023 22:26:56 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
[...]
> > 
> > The pointer was originally derived by a call to `into_raw`:
> > ```
> >      pub fn into_raw(self) -> *const T {
> >          let ptr = self.ptr.as_ptr();
> >          core::mem::forget(self);
> >          // SAFETY: The pointer is valid.
> >          unsafe { core::ptr::addr_of!((*ptr).data) }
> >      }
> > ```
> > So in this function the origin (also the origin of the provenance)
> > of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
> > Raw pointers do not lose this provenance information when you cast
> > it and when using `addr_of`/`addr_of_mut`. So provenance is something
> > that is not really represented in the type system for raw pointers.
> > 
> > When doing a round trip through a reference though, the provenance is
> > newly assigned and thus would only be valid for a `T`:
> > ```
> > let raw = arc.into_raw();
> > let reference = unsafe { &*raw };
> > let raw: *const T = reference;
> > let arc = unsafe { Arc::from_raw(raw) };
> > ```
> > Miri would complain about the above code.
> > 
> 
> One thing we can do is to opt from strict provenance, so:
> 

A few questions about strict provenance:

> ```
> let raw = arc.into_raw();
> let _ = raw as usize; // expose the provenance of raw

Should this be a expose_addr()?

> let reference = unsafe { &*raw };
> let raw = reference as *const T as usize as *const T;

and this is a from_exposed_addr{_mut}(), right?

> let arc = unsafe { Arc::from_raw(raw) };
> ```
> 

One step back, If we were to use strict provenance API (i.e.
expose_addr()/from_exposed_addr()), we could use it to "fix" the
original problem? By:

*	expose_addr() in as_with_ref()
*	from_exposed_addr() in `impl From<&WithRef<T>> for Arc`

right?

More steps back, is the original issue only a real issue under strict
provenance rules? Don't make me wrong, I like the ideas behind strict
provenance, I just want to check, if we don't enable strict provenance
(as a matter of fact, we don't do it today), will the original issue
found by Alice be a UB? Or is there a way to disable Miri's check on
strict provenance? IIUC, the cause of the original issue is that "you
cannot reborrow a pointer derived from a `&` to get a `&mut`, even when
there is no other alias to the same object". Maybe I'm still missing
something, but without strict provenance, is this a problem? Or is there
a provenance model of Rust without strict provenance?

Regards,
Boqun

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Alice Ryhl]

On Tue, Sep 26, 2023 at 5:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Sep 26, 2023 at 04:26:59PM +0800, Gary Guo wrote:
> > On Mon, 25 Sep 2023 22:26:56 +0000
> > Benno Lossin <benno.lossin@proton.me> wrote:
> >
> [...]
> > >
> > > The pointer was originally derived by a call to `into_raw`:
> > > ```
> > >      pub fn into_raw(self) -> *const T {
> > >          let ptr = self.ptr.as_ptr();
> > >          core::mem::forget(self);
> > >          // SAFETY: The pointer is valid.
> > >          unsafe { core::ptr::addr_of!((*ptr).data) }
> > >      }
> > > ```
> > > So in this function the origin (also the origin of the provenance)
> > > of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
> > > Raw pointers do not lose this provenance information when you cast
> > > it and when using `addr_of`/`addr_of_mut`. So provenance is something
> > > that is not really represented in the type system for raw pointers.
> > >
> > > When doing a round trip through a reference though, the provenance is
> > > newly assigned and thus would only be valid for a `T`:
> > > ```
> > > let raw = arc.into_raw();
> > > let reference = unsafe { &*raw };
> > > let raw: *const T = reference;
> > > let arc = unsafe { Arc::from_raw(raw) };
> > > ```
> > > Miri would complain about the above code.
> > >
> >
> > One thing we can do is to opt from strict provenance, so:
> >
>
> A few questions about strict provenance:
>
> > ```
> > let raw = arc.into_raw();
> > let _ = raw as usize; // expose the provenance of raw
>
> Should this be a expose_addr()?

Pointer to integer cast is equivalent to expose_addr.

> > let reference = unsafe { &*raw };
> > let raw = reference as *const T as usize as *const T;
>
> and this is a from_exposed_addr{_mut}(), right?

Integer to pointer cast is equivalent to from_exposed_addr.

> > let arc = unsafe { Arc::from_raw(raw) };
> > ```
> >
>
> One step back, If we were to use strict provenance API (i.e.
> expose_addr()/from_exposed_addr()), we could use it to "fix" the
> original problem? By:
>
> *       expose_addr() in as_with_ref()
> *       from_exposed_addr() in `impl From<&WithRef<T>> for Arc`
>
> right?
>
> More steps back, is the original issue only a real issue under strict
> provenance rules? Don't make me wrong, I like the ideas behind strict
> provenance, I just want to check, if we don't enable strict provenance
> (as a matter of fact, we don't do it today),

Outside of miri, strict provenance is not really something you enable.
It's a set of rules that are stricter than the real rules, that are
designed such that when you follow them, your code will be correct
under any conceivable memory model we might end up with. They will
never be the rules that the compiler actually uses.

I think by "opt out from strict provenance", Gary just meant "use
int2ptr and ptr2int casts to reset the provenance".

> will the original issue found by Alice be a UB?

Yes, it's UB under any ruleset that exists out there. There's no flag
to turn it off.

> Or is there a way to disable Miri's check on
> strict provenance? IIUC, the cause of the original issue is that "you
> cannot reborrow a pointer derived from a `&` to get a `&mut`, even when
> there is no other alias to the same object". Maybe I'm still missing
> something, but without strict provenance, is this a problem? Or is there
> a provenance model of Rust without strict provenance?

It's a problem under all of the memory models. The rule being violated
is exactly the same rule as the one behind this paragraph:

> Transmuting an & to &mut is Undefined Behavior. While certain usages may appear safe, note that the Rust optimizer is free to assume that a shared reference won't change through its lifetime and thus such transmutation will run afoul of those assumptions. So:
>
> Transmuting an & to &mut is always Undefined Behavior.
> No you can't do it.
> No you're not special.
From: https://doc.rust-lang.org/nomicon/transmutes.html

Alice

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Tue, Sep 26, 2023 at 05:41:17PM +0200, Alice Ryhl wrote:
> On Tue, Sep 26, 2023 at 5:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Tue, Sep 26, 2023 at 04:26:59PM +0800, Gary Guo wrote:
> > > On Mon, 25 Sep 2023 22:26:56 +0000
> > > Benno Lossin <benno.lossin@proton.me> wrote:
> > >
> > [...]
> > > >
> > > > The pointer was originally derived by a call to `into_raw`:
> > > > ```
> > > >      pub fn into_raw(self) -> *const T {
> > > >          let ptr = self.ptr.as_ptr();
> > > >          core::mem::forget(self);
> > > >          // SAFETY: The pointer is valid.
> > > >          unsafe { core::ptr::addr_of!((*ptr).data) }
> > > >      }
> > > > ```
> > > > So in this function the origin (also the origin of the provenance)
> > > > of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
> > > > Raw pointers do not lose this provenance information when you cast
> > > > it and when using `addr_of`/`addr_of_mut`. So provenance is something
> > > > that is not really represented in the type system for raw pointers.
> > > >
> > > > When doing a round trip through a reference though, the provenance is
> > > > newly assigned and thus would only be valid for a `T`:
> > > > ```
> > > > let raw = arc.into_raw();
> > > > let reference = unsafe { &*raw };
> > > > let raw: *const T = reference;
> > > > let arc = unsafe { Arc::from_raw(raw) };
> > > > ```
> > > > Miri would complain about the above code.
> > > >
> > >
> > > One thing we can do is to opt from strict provenance, so:
> > >
> >
> > A few questions about strict provenance:
> >
> > > ```
> > > let raw = arc.into_raw();
> > > let _ = raw as usize; // expose the provenance of raw
> >
> > Should this be a expose_addr()?
> 
> Pointer to integer cast is equivalent to expose_addr.
> 
> > > let reference = unsafe { &*raw };
> > > let raw = reference as *const T as usize as *const T;
> >
> > and this is a from_exposed_addr{_mut}(), right?
> 
> Integer to pointer cast is equivalent to from_exposed_addr.
> 

Got it, thanks!

> > > let arc = unsafe { Arc::from_raw(raw) };
> > > ```
> > >
> >
> > One step back, If we were to use strict provenance API (i.e.
> > expose_addr()/from_exposed_addr()), we could use it to "fix" the
> > original problem? By:
> >
> > *       expose_addr() in as_with_ref()
> > *       from_exposed_addr() in `impl From<&WithRef<T>> for Arc`
> >
> > right?
> >
> > More steps back, is the original issue only a real issue under strict
> > provenance rules? Don't make me wrong, I like the ideas behind strict
> > provenance, I just want to check, if we don't enable strict provenance
> > (as a matter of fact, we don't do it today),
> 
> Outside of miri, strict provenance is not really something you enable.
> It's a set of rules that are stricter than the real rules, that are
> designed such that when you follow them, your code will be correct
> under any conceivable memory model we might end up with. They will
> never be the rules that the compiler actually uses.
> 
> I think by "opt out from strict provenance", Gary just meant "use
> int2ptr and ptr2int casts to reset the provenance".
> 
> > will the original issue found by Alice be a UB?
> 
> Yes, it's UB under any ruleset that exists out there. There's no flag
> to turn it off.
> 
> > Or is there a way to disable Miri's check on
> > strict provenance? IIUC, the cause of the original issue is that "you
> > cannot reborrow a pointer derived from a `&` to get a `&mut`, even when
> > there is no other alias to the same object". Maybe I'm still missing
> > something, but without strict provenance, is this a problem? Or is there
> > a provenance model of Rust without strict provenance?
> 
> It's a problem under all of the memory models. The rule being violated
> is exactly the same rule as the one behind this paragraph:
> 
> > Transmuting an & to &mut is Undefined Behavior. While certain usages may appear safe, note that the Rust optimizer is free to assume that a shared reference won't change through its lifetime and thus such transmutation will run afoul of those assumptions. So:
> >
> > Transmuting an & to &mut is always Undefined Behavior.
> > No you can't do it.
> > No you're not special.
> From: https://doc.rust-lang.org/nomicon/transmutes.html

But here the difference it that we only derive a `*mut` from a `&`,
rather than transmute to a `&mut`, right? We only use `&` to get a
pointer value (a usize), so I don't think that rule applies here? Or in
other words, does the following implemenation look good to you?

	impl<T: ?Sized> Arc<T> {
	    pub fn as_with_ref(&self) -> &WithRef<T> {
		// expose
		let _ = self.ptr.as_ptr() as usize;
		unsafe { self.ptr.as_ref() }
	    }
	}

	impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
	    fn from(b: &WithRef<T>) -> Self {
		// from exposed
		let ptr = unsafe { NonNull::new_unchecked(b as *const _ as usize as *mut _) };
		// SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
		// guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
		// increment.
		ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
		    .deref()
		    .clone()
	    }
	}


An equivalent code snippet is as below (in case anyone wants to try it
in miri):
```rust
    let raw = Box::into_raw(arc);
    
    // as_with_ref()
    let _ = raw as usize;
    let reference = unsafe { &*raw };
    
    // from()
    let raw: *mut T = reference as *const _ as usize as  *mut _ ;

    // drop()
    let arc = unsafe { Box::from_raw(raw) };
```

Regards,
Boqun

> 
> Alice
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Benno Lossin]

On 26.09.23 18:35, Boqun Feng wrote:
> On Tue, Sep 26, 2023 at 05:41:17PM +0200, Alice Ryhl wrote:
>> On Tue, Sep 26, 2023 at 5:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>>>
>>> On Tue, Sep 26, 2023 at 04:26:59PM +0800, Gary Guo wrote:
>>>> On Mon, 25 Sep 2023 22:26:56 +0000
>>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>>
>>> [...]
>>>>>
>>>>> The pointer was originally derived by a call to `into_raw`:
>>>>> ```
>>>>>       pub fn into_raw(self) -> *const T {
>>>>>           let ptr = self.ptr.as_ptr();
>>>>>           core::mem::forget(self);
>>>>>           // SAFETY: The pointer is valid.
>>>>>           unsafe { core::ptr::addr_of!((*ptr).data) }
>>>>>       }
>>>>> ```
>>>>> So in this function the origin (also the origin of the provenance)
>>>>> of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
>>>>> Raw pointers do not lose this provenance information when you cast
>>>>> it and when using `addr_of`/`addr_of_mut`. So provenance is something
>>>>> that is not really represented in the type system for raw pointers.
>>>>>
>>>>> When doing a round trip through a reference though, the provenance is
>>>>> newly assigned and thus would only be valid for a `T`:
>>>>> ```
>>>>> let raw = arc.into_raw();
>>>>> let reference = unsafe { &*raw };
>>>>> let raw: *const T = reference;
>>>>> let arc = unsafe { Arc::from_raw(raw) };
>>>>> ```
>>>>> Miri would complain about the above code.
>>>>>
>>>>
>>>> One thing we can do is to opt from strict provenance, so:
>>>>
>>>
>>> A few questions about strict provenance:
>>>
>>>> ```
>>>> let raw = arc.into_raw();
>>>> let _ = raw as usize; // expose the provenance of raw
>>>
>>> Should this be a expose_addr()?
>>
>> Pointer to integer cast is equivalent to expose_addr.
>>
>>>> let reference = unsafe { &*raw };
>>>> let raw = reference as *const T as usize as *const T;
>>>
>>> and this is a from_exposed_addr{_mut}(), right?
>>
>> Integer to pointer cast is equivalent to from_exposed_addr.
>>
> 
> Got it, thanks!
> 
>>>> let arc = unsafe { Arc::from_raw(raw) };
>>>> ```
>>>>
>>>
>>> One step back, If we were to use strict provenance API (i.e.
>>> expose_addr()/from_exposed_addr()), we could use it to "fix" the
>>> original problem? By:
>>>
>>> *       expose_addr() in as_with_ref()
>>> *       from_exposed_addr() in `impl From<&WithRef<T>> for Arc`
>>>
>>> right?
>>>
>>> More steps back, is the original issue only a real issue under strict
>>> provenance rules? Don't make me wrong, I like the ideas behind strict
>>> provenance, I just want to check, if we don't enable strict provenance
>>> (as a matter of fact, we don't do it today),
>>
>> Outside of miri, strict provenance is not really something you enable.
>> It's a set of rules that are stricter than the real rules, that are
>> designed such that when you follow them, your code will be correct
>> under any conceivable memory model we might end up with. They will
>> never be the rules that the compiler actually uses.
>>
>> I think by "opt out from strict provenance", Gary just meant "use
>> int2ptr and ptr2int casts to reset the provenance".
>>
>>> will the original issue found by Alice be a UB?
>>
>> Yes, it's UB under any ruleset that exists out there. There's no flag
>> to turn it off.
>>
>>> Or is there a way to disable Miri's check on
>>> strict provenance? IIUC, the cause of the original issue is that "you
>>> cannot reborrow a pointer derived from a `&` to get a `&mut`, even when
>>> there is no other alias to the same object". Maybe I'm still missing
>>> something, but without strict provenance, is this a problem? Or is there
>>> a provenance model of Rust without strict provenance?
>>
>> It's a problem under all of the memory models. The rule being violated
>> is exactly the same rule as the one behind this paragraph:
>>
>>> Transmuting an & to &mut is Undefined Behavior. While certain usages may appear safe, note that the Rust optimizer is free to assume that a shared reference won't change through its lifetime and thus such transmutation will run afoul of those assumptions. So:
>>>
>>> Transmuting an & to &mut is always Undefined Behavior.
>>> No you can't do it.
>>> No you're not special.
>> From: https://doc.rust-lang.org/nomicon/transmutes.html
> 
> But here the difference it that we only derive a `*mut` from a `&`,
> rather than transmute to a `&mut`, right? We only use `&` to get a
> pointer value (a usize), so I don't think that rule applies here? Or in
> other words, does the following implemenation look good to you?
> 
> 	impl<T: ?Sized> Arc<T> {
> 	    pub fn as_with_ref(&self) -> &WithRef<T> {
> 		// expose
> 		let _ = self.ptr.as_ptr() as usize;
> 		unsafe { self.ptr.as_ref() }
> 	    }
> 	}
> 
> 	impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> 	    fn from(b: &WithRef<T>) -> Self {
> 		// from exposed
> 		let ptr = unsafe { NonNull::new_unchecked(b as *const _ as usize as *mut _) };
> 		// SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
> 		// guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
> 		// increment.
> 		ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
> 		    .deref()
> 		    .clone()
> 	    }
> 	}
> 
> 
> An equivalent code snippet is as below (in case anyone wants to try it
> in miri):
> ```rust
>      let raw = Box::into_raw(arc);
> 
>      // as_with_ref()
>      let _ = raw as usize;
>      let reference = unsafe { &*raw };
> 
>      // from()
>      let raw: *mut T = reference as *const _ as usize as  *mut _ ;
> 
>      // drop()
>      let arc = unsafe { Box::from_raw(raw) };
> ```

I don't understand why we are trying to use ptr2int to fix this.
Simply wrapping the `T` field inside `WithRef` with `UnsafeCell`
should be enough.

-- 
Cheers,
Benno

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Tue, Sep 26, 2023 at 05:15:52PM +0000, Benno Lossin wrote:
> On 26.09.23 18:35, Boqun Feng wrote:
> > On Tue, Sep 26, 2023 at 05:41:17PM +0200, Alice Ryhl wrote:
> >> On Tue, Sep 26, 2023 at 5:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >>>
> >>> On Tue, Sep 26, 2023 at 04:26:59PM +0800, Gary Guo wrote:
> >>>> On Mon, 25 Sep 2023 22:26:56 +0000
> >>>> Benno Lossin <benno.lossin@proton.me> wrote:
> >>>>
> >>> [...]
> >>>>>
> >>>>> The pointer was originally derived by a call to `into_raw`:
> >>>>> ```
> >>>>>       pub fn into_raw(self) -> *const T {
> >>>>>           let ptr = self.ptr.as_ptr();
> >>>>>           core::mem::forget(self);
> >>>>>           // SAFETY: The pointer is valid.
> >>>>>           unsafe { core::ptr::addr_of!((*ptr).data) }
> >>>>>       }
> >>>>> ```
> >>>>> So in this function the origin (also the origin of the provenance)
> >>>>> of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
> >>>>> Raw pointers do not lose this provenance information when you cast
> >>>>> it and when using `addr_of`/`addr_of_mut`. So provenance is something
> >>>>> that is not really represented in the type system for raw pointers.
> >>>>>
> >>>>> When doing a round trip through a reference though, the provenance is
> >>>>> newly assigned and thus would only be valid for a `T`:
> >>>>> ```
> >>>>> let raw = arc.into_raw();
> >>>>> let reference = unsafe { &*raw };
> >>>>> let raw: *const T = reference;
> >>>>> let arc = unsafe { Arc::from_raw(raw) };
> >>>>> ```
> >>>>> Miri would complain about the above code.
> >>>>>
> >>>>
> >>>> One thing we can do is to opt from strict provenance, so:
> >>>>
> >>>
> >>> A few questions about strict provenance:
> >>>
> >>>> ```
> >>>> let raw = arc.into_raw();
> >>>> let _ = raw as usize; // expose the provenance of raw
> >>>
> >>> Should this be a expose_addr()?
> >>
> >> Pointer to integer cast is equivalent to expose_addr.
> >>
> >>>> let reference = unsafe { &*raw };
> >>>> let raw = reference as *const T as usize as *const T;
> >>>
> >>> and this is a from_exposed_addr{_mut}(), right?
> >>
> >> Integer to pointer cast is equivalent to from_exposed_addr.
> >>
> > 
> > Got it, thanks!
> > 
> >>>> let arc = unsafe { Arc::from_raw(raw) };
> >>>> ```
> >>>>
> >>>
> >>> One step back, If we were to use strict provenance API (i.e.
> >>> expose_addr()/from_exposed_addr()), we could use it to "fix" the
> >>> original problem? By:
> >>>
> >>> *       expose_addr() in as_with_ref()
> >>> *       from_exposed_addr() in `impl From<&WithRef<T>> for Arc`
> >>>
> >>> right?
> >>>
> >>> More steps back, is the original issue only a real issue under strict
> >>> provenance rules? Don't make me wrong, I like the ideas behind strict
> >>> provenance, I just want to check, if we don't enable strict provenance
> >>> (as a matter of fact, we don't do it today),
> >>
> >> Outside of miri, strict provenance is not really something you enable.
> >> It's a set of rules that are stricter than the real rules, that are
> >> designed such that when you follow them, your code will be correct
> >> under any conceivable memory model we might end up with. They will
> >> never be the rules that the compiler actually uses.
> >>
> >> I think by "opt out from strict provenance", Gary just meant "use
> >> int2ptr and ptr2int casts to reset the provenance".
> >>
> >>> will the original issue found by Alice be a UB?
> >>
> >> Yes, it's UB under any ruleset that exists out there. There's no flag
> >> to turn it off.
> >>
> >>> Or is there a way to disable Miri's check on
> >>> strict provenance? IIUC, the cause of the original issue is that "you
> >>> cannot reborrow a pointer derived from a `&` to get a `&mut`, even when
> >>> there is no other alias to the same object". Maybe I'm still missing
> >>> something, but without strict provenance, is this a problem? Or is there
> >>> a provenance model of Rust without strict provenance?
> >>
> >> It's a problem under all of the memory models. The rule being violated
> >> is exactly the same rule as the one behind this paragraph:
> >>
> >>> Transmuting an & to &mut is Undefined Behavior. While certain usages may appear safe, note that the Rust optimizer is free to assume that a shared reference won't change through its lifetime and thus such transmutation will run afoul of those assumptions. So:
> >>>
> >>> Transmuting an & to &mut is always Undefined Behavior.
> >>> No you can't do it.
> >>> No you're not special.
> >> From: https://doc.rust-lang.org/nomicon/transmutes.html
> > 
> > But here the difference it that we only derive a `*mut` from a `&`,
> > rather than transmute to a `&mut`, right? We only use `&` to get a
> > pointer value (a usize), so I don't think that rule applies here? Or in
> > other words, does the following implemenation look good to you?
> > 
> > 	impl<T: ?Sized> Arc<T> {
> > 	    pub fn as_with_ref(&self) -> &WithRef<T> {
> > 		// expose
> > 		let _ = self.ptr.as_ptr() as usize;
> > 		unsafe { self.ptr.as_ref() }
> > 	    }
> > 	}
> > 
> > 	impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> > 	    fn from(b: &WithRef<T>) -> Self {
> > 		// from exposed
> > 		let ptr = unsafe { NonNull::new_unchecked(b as *const _ as usize as *mut _) };
> > 		// SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
> > 		// guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
> > 		// increment.
> > 		ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
> > 		    .deref()
> > 		    .clone()
> > 	    }
> > 	}
> > 
> > 
> > An equivalent code snippet is as below (in case anyone wants to try it
> > in miri):
> > ```rust
> >      let raw = Box::into_raw(arc);
> > 
> >      // as_with_ref()
> >      let _ = raw as usize;
> >      let reference = unsafe { &*raw };
> > 
> >      // from()
> >      let raw: *mut T = reference as *const _ as usize as  *mut _ ;
> > 
> >      // drop()
> >      let arc = unsafe { Box::from_raw(raw) };
> > ```
> 
> I don't understand why we are trying to use ptr2int to fix this.
> Simply wrapping the `T` field inside `WithRef` with `UnsafeCell`
> should be enough.
> 

To me (and maybe the same for Wedson), it's actually Ok that we don't do
the change (i.e. the ArcBorrow -> &WithRef) at all. It's more a
code/concept simplification.

Fixing with an `UnsafeCell` seems more like a workaround to me, because
there is no interior mutable requirement here, so I just don't want to
patch something unnecessary here just to make things work.

Put it in another way, if `UnsafeCell` can fix this and no interior
mutability is needed, we probably can fix this with another way or there
is an API like `UnsafeCell` is missing here.

Sorry for being stubborn here ;-) But I really want to find a better
solution for the similar problems.

What's the shortcoming of ptr2int?

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Benno Lossin]

On 26.09.23 19:43, Boqun Feng wrote:
> On Tue, Sep 26, 2023 at 05:15:52PM +0000, Benno Lossin wrote:
>> On 26.09.23 18:35, Boqun Feng wrote:
>>> On Tue, Sep 26, 2023 at 05:41:17PM +0200, Alice Ryhl wrote:
>>>> On Tue, Sep 26, 2023 at 5:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>>>>>
>>>>> On Tue, Sep 26, 2023 at 04:26:59PM +0800, Gary Guo wrote:
>>>>>> On Mon, 25 Sep 2023 22:26:56 +0000
>>>>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>>>>
>>>>> [...]
>>>>>>>
>>>>>>> The pointer was originally derived by a call to `into_raw`:
>>>>>>> ```
>>>>>>>        pub fn into_raw(self) -> *const T {
>>>>>>>            let ptr = self.ptr.as_ptr();
>>>>>>>            core::mem::forget(self);
>>>>>>>            // SAFETY: The pointer is valid.
>>>>>>>            unsafe { core::ptr::addr_of!((*ptr).data) }
>>>>>>>        }
>>>>>>> ```
>>>>>>> So in this function the origin (also the origin of the provenance)
>>>>>>> of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
>>>>>>> Raw pointers do not lose this provenance information when you cast
>>>>>>> it and when using `addr_of`/`addr_of_mut`. So provenance is something
>>>>>>> that is not really represented in the type system for raw pointers.
>>>>>>>
>>>>>>> When doing a round trip through a reference though, the provenance is
>>>>>>> newly assigned and thus would only be valid for a `T`:
>>>>>>> ```
>>>>>>> let raw = arc.into_raw();
>>>>>>> let reference = unsafe { &*raw };
>>>>>>> let raw: *const T = reference;
>>>>>>> let arc = unsafe { Arc::from_raw(raw) };
>>>>>>> ```
>>>>>>> Miri would complain about the above code.
>>>>>>>
>>>>>>
>>>>>> One thing we can do is to opt from strict provenance, so:
>>>>>>
>>>>>
>>>>> A few questions about strict provenance:
>>>>>
>>>>>> ```
>>>>>> let raw = arc.into_raw();
>>>>>> let _ = raw as usize; // expose the provenance of raw
>>>>>
>>>>> Should this be a expose_addr()?
>>>>
>>>> Pointer to integer cast is equivalent to expose_addr.
>>>>
>>>>>> let reference = unsafe { &*raw };
>>>>>> let raw = reference as *const T as usize as *const T;
>>>>>
>>>>> and this is a from_exposed_addr{_mut}(), right?
>>>>
>>>> Integer to pointer cast is equivalent to from_exposed_addr.
>>>>
>>>
>>> Got it, thanks!
>>>
>>>>>> let arc = unsafe { Arc::from_raw(raw) };
>>>>>> ```
>>>>>>
>>>>>
>>>>> One step back, If we were to use strict provenance API (i.e.
>>>>> expose_addr()/from_exposed_addr()), we could use it to "fix" the
>>>>> original problem? By:
>>>>>
>>>>> *       expose_addr() in as_with_ref()
>>>>> *       from_exposed_addr() in `impl From<&WithRef<T>> for Arc`
>>>>>
>>>>> right?
>>>>>
>>>>> More steps back, is the original issue only a real issue under strict
>>>>> provenance rules? Don't make me wrong, I like the ideas behind strict
>>>>> provenance, I just want to check, if we don't enable strict provenance
>>>>> (as a matter of fact, we don't do it today),
>>>>
>>>> Outside of miri, strict provenance is not really something you enable.
>>>> It's a set of rules that are stricter than the real rules, that are
>>>> designed such that when you follow them, your code will be correct
>>>> under any conceivable memory model we might end up with. They will
>>>> never be the rules that the compiler actually uses.
>>>>
>>>> I think by "opt out from strict provenance", Gary just meant "use
>>>> int2ptr and ptr2int casts to reset the provenance".
>>>>
>>>>> will the original issue found by Alice be a UB?
>>>>
>>>> Yes, it's UB under any ruleset that exists out there. There's no flag
>>>> to turn it off.
>>>>
>>>>> Or is there a way to disable Miri's check on
>>>>> strict provenance? IIUC, the cause of the original issue is that "you
>>>>> cannot reborrow a pointer derived from a `&` to get a `&mut`, even when
>>>>> there is no other alias to the same object". Maybe I'm still missing
>>>>> something, but without strict provenance, is this a problem? Or is there
>>>>> a provenance model of Rust without strict provenance?
>>>>
>>>> It's a problem under all of the memory models. The rule being violated
>>>> is exactly the same rule as the one behind this paragraph:
>>>>
>>>>> Transmuting an & to &mut is Undefined Behavior. While certain usages may appear safe, note that the Rust optimizer is free to assume that a shared reference won't change through its lifetime and thus such transmutation will run afoul of those assumptions. So:
>>>>>
>>>>> Transmuting an & to &mut is always Undefined Behavior.
>>>>> No you can't do it.
>>>>> No you're not special.
>>>> From: https://doc.rust-lang.org/nomicon/transmutes.html
>>>
>>> But here the difference it that we only derive a `*mut` from a `&`,
>>> rather than transmute to a `&mut`, right? We only use `&` to get a
>>> pointer value (a usize), so I don't think that rule applies here? Or in
>>> other words, does the following implemenation look good to you?
>>>
>>> 	impl<T: ?Sized> Arc<T> {
>>> 	    pub fn as_with_ref(&self) -> &WithRef<T> {
>>> 		// expose
>>> 		let _ = self.ptr.as_ptr() as usize;
>>> 		unsafe { self.ptr.as_ref() }
>>> 	    }
>>> 	}
>>>
>>> 	impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
>>> 	    fn from(b: &WithRef<T>) -> Self {
>>> 		// from exposed
>>> 		let ptr = unsafe { NonNull::new_unchecked(b as *const _ as usize as *mut _) };
>>> 		// SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
>>> 		// guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
>>> 		// increment.
>>> 		ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
>>> 		    .deref()
>>> 		    .clone()
>>> 	    }
>>> 	}
>>>
>>>
>>> An equivalent code snippet is as below (in case anyone wants to try it
>>> in miri):
>>> ```rust
>>>       let raw = Box::into_raw(arc);
>>>
>>>       // as_with_ref()
>>>       let _ = raw as usize;
>>>       let reference = unsafe { &*raw };
>>>
>>>       // from()
>>>       let raw: *mut T = reference as *const _ as usize as  *mut _ ;
>>>
>>>       // drop()
>>>       let arc = unsafe { Box::from_raw(raw) };
>>> ```
>>
>> I don't understand why we are trying to use ptr2int to fix this.
>> Simply wrapping the `T` field inside `WithRef` with `UnsafeCell`
>> should be enough.
>>
> 
> To me (and maybe the same for Wedson), it's actually Ok that we don't do
> the change (i.e. the ArcBorrow -> &WithRef) at all. It's more a
> code/concept simplification.

That would be fine for me as well.

> Fixing with an `UnsafeCell` seems more like a workaround to me, because
> there is no interior mutable requirement here, so I just don't want to
> patch something unnecessary here just to make things work.

To me there is, though it is very subtle and can be missed easily.

> Put it in another way, if `UnsafeCell` can fix this and no interior
> mutability is needed, we probably can fix this with another way or there
> is an API like `UnsafeCell` is missing here.
> 
> Sorry for being stubborn here ;-) But I really want to find a better
> solution for the similar problems.
> 
> What's the shortcoming of ptr2int?

To me ptr2int seems like a workaround... I do not like
the concept of ptr2int in general, since it is specifying very
hacky behavior. How would you explain the code line

```rust
// expose addr
let _ = ptr as usize;
```

to someone who has no idea of this problem? I think that
line of code has some actual hacky vibes not adding `UnsafeCell`
around the field.

-- 
Cheers,
Benno

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Alice Ryhl]

On 9/26/23 19:43, Boqun Feng wrote:
> On Tue, Sep 26, 2023 at 05:15:52PM +0000, Benno Lossin wrote:
>> On 26.09.23 18:35, Boqun Feng wrote:
>>> On Tue, Sep 26, 2023 at 05:41:17PM +0200, Alice Ryhl wrote:
>>>> On Tue, Sep 26, 2023 at 5:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>>>>>
>>>>> On Tue, Sep 26, 2023 at 04:26:59PM +0800, Gary Guo wrote:
>>>>>> On Mon, 25 Sep 2023 22:26:56 +0000
>>>>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>>>>
>>>>> [...]
>>>>>>>
>>>>>>> The pointer was originally derived by a call to `into_raw`:
>>>>>>> ```
>>>>>>>        pub fn into_raw(self) -> *const T {
>>>>>>>            let ptr = self.ptr.as_ptr();
>>>>>>>            core::mem::forget(self);
>>>>>>>            // SAFETY: The pointer is valid.
>>>>>>>            unsafe { core::ptr::addr_of!((*ptr).data) }
>>>>>>>        }
>>>>>>> ```
>>>>>>> So in this function the origin (also the origin of the provenance)
>>>>>>> of the pointer is `ptr` which is of type `NonNull<WithRef<T>>`.
>>>>>>> Raw pointers do not lose this provenance information when you cast
>>>>>>> it and when using `addr_of`/`addr_of_mut`. So provenance is something
>>>>>>> that is not really represented in the type system for raw pointers.
>>>>>>>
>>>>>>> When doing a round trip through a reference though, the provenance is
>>>>>>> newly assigned and thus would only be valid for a `T`:
>>>>>>> ```
>>>>>>> let raw = arc.into_raw();
>>>>>>> let reference = unsafe { &*raw };
>>>>>>> let raw: *const T = reference;
>>>>>>> let arc = unsafe { Arc::from_raw(raw) };
>>>>>>> ```
>>>>>>> Miri would complain about the above code.
>>>>>>>
>>>>>>
>>>>>> One thing we can do is to opt from strict provenance, so:
>>>>>>
>>>>>
>>>>> A few questions about strict provenance:
>>>>>
>>>>>> ```
>>>>>> let raw = arc.into_raw();
>>>>>> let _ = raw as usize; // expose the provenance of raw
>>>>>
>>>>> Should this be a expose_addr()?
>>>>
>>>> Pointer to integer cast is equivalent to expose_addr.
>>>>
>>>>>> let reference = unsafe { &*raw };
>>>>>> let raw = reference as *const T as usize as *const T;
>>>>>
>>>>> and this is a from_exposed_addr{_mut}(), right?
>>>>
>>>> Integer to pointer cast is equivalent to from_exposed_addr.
>>>>
>>>
>>> Got it, thanks!
>>>
>>>>>> let arc = unsafe { Arc::from_raw(raw) };
>>>>>> ```
>>>>>>
>>>>>
>>>>> One step back, If we were to use strict provenance API (i.e.
>>>>> expose_addr()/from_exposed_addr()), we could use it to "fix" the
>>>>> original problem? By:
>>>>>
>>>>> *       expose_addr() in as_with_ref()
>>>>> *       from_exposed_addr() in `impl From<&WithRef<T>> for Arc`
>>>>>
>>>>> right?
>>>>>
>>>>> More steps back, is the original issue only a real issue under strict
>>>>> provenance rules? Don't make me wrong, I like the ideas behind strict
>>>>> provenance, I just want to check, if we don't enable strict provenance
>>>>> (as a matter of fact, we don't do it today),
>>>>
>>>> Outside of miri, strict provenance is not really something you enable.
>>>> It's a set of rules that are stricter than the real rules, that are
>>>> designed such that when you follow them, your code will be correct
>>>> under any conceivable memory model we might end up with. They will
>>>> never be the rules that the compiler actually uses.
>>>>
>>>> I think by "opt out from strict provenance", Gary just meant "use
>>>> int2ptr and ptr2int casts to reset the provenance".
>>>>
>>>>> will the original issue found by Alice be a UB?
>>>>
>>>> Yes, it's UB under any ruleset that exists out there. There's no flag
>>>> to turn it off.
>>>>
>>>>> Or is there a way to disable Miri's check on
>>>>> strict provenance? IIUC, the cause of the original issue is that "you
>>>>> cannot reborrow a pointer derived from a `&` to get a `&mut`, even when
>>>>> there is no other alias to the same object". Maybe I'm still missing
>>>>> something, but without strict provenance, is this a problem? Or is there
>>>>> a provenance model of Rust without strict provenance?
>>>>
>>>> It's a problem under all of the memory models. The rule being violated
>>>> is exactly the same rule as the one behind this paragraph:
>>>>
>>>>> Transmuting an & to &mut is Undefined Behavior. While certain usages may appear safe, note that the Rust optimizer is free to assume that a shared reference won't change through its lifetime and thus such transmutation will run afoul of those assumptions. So:
>>>>>
>>>>> Transmuting an & to &mut is always Undefined Behavior.
>>>>> No you can't do it.
>>>>> No you're not special.
>>>> From: https://doc.rust-lang.org/nomicon/transmutes.html
>>>
>>> But here the difference it that we only derive a `*mut` from a `&`,
>>> rather than transmute to a `&mut`, right? We only use `&` to get a
>>> pointer value (a usize), so I don't think that rule applies here? Or in
>>> other words, does the following implemenation look good to you?
>>>
>>> 	impl<T: ?Sized> Arc<T> {
>>> 	    pub fn as_with_ref(&self) -> &WithRef<T> {
>>> 		// expose
>>> 		let _ = self.ptr.as_ptr() as usize;
>>> 		unsafe { self.ptr.as_ref() }
>>> 	    }
>>> 	}
>>>
>>> 	impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
>>> 	    fn from(b: &WithRef<T>) -> Self {
>>> 		// from exposed
>>> 		let ptr = unsafe { NonNull::new_unchecked(b as *const _ as usize as *mut _) };
>>> 		// SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
>>> 		// guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
>>> 		// increment.
>>> 		ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
>>> 		    .deref()
>>> 		    .clone()
>>> 	    }
>>> 	}
>>>
>>>
>>> An equivalent code snippet is as below (in case anyone wants to try it
>>> in miri):
>>> ```rust
>>>       let raw = Box::into_raw(arc);
>>>
>>>       // as_with_ref()
>>>       let _ = raw as usize;
>>>       let reference = unsafe { &*raw };
>>>
>>>       // from()
>>>       let raw: *mut T = reference as *const _ as usize as  *mut _ ;
>>>
>>>       // drop()
>>>       let arc = unsafe { Box::from_raw(raw) };
>>> ```
>>
>> I don't understand why we are trying to use ptr2int to fix this.
>> Simply wrapping the `T` field inside `WithRef` with `UnsafeCell`
>> should be enough.
>>
> 
> To me (and maybe the same for Wedson), it's actually Ok that we don't do
> the change (i.e. the ArcBorrow -> &WithRef) at all. It's more a
> code/concept simplification.

Not making the change is fine with me.

> Fixing with an `UnsafeCell` seems more like a workaround to me, because
> there is no interior mutable requirement here, so I just don't want to
> patch something unnecessary here just to make things work.

I don't think it's a just a workaround. We're dealing with shared 
references to something, but they can be used to modify the value under 
some circumstances. That seems like rather standard interior mutability.

> Put it in another way, if `UnsafeCell` can fix this and no interior
> mutability is needed, we probably can fix this with another way or there
> is an API like `UnsafeCell` is missing here.
> 
> Sorry for being stubborn here ;-) But I really want to find a better
> solution for the similar problems.
> 
> What's the shortcoming of ptr2int?

To me, that's way more of a hack than using UnsafeCell is.

> Regards,
> Boqun
> 
>> -- 
>> Cheers,
>> Benno
>>
>>

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Boqun Feng]

On Tue, Sep 26, 2023 at 05:15:52PM +0000, Benno Lossin wrote:
[...]
> > 
> > But here the difference it that we only derive a `*mut` from a `&`,
> > rather than transmute to a `&mut`, right? We only use `&` to get a
> > pointer value (a usize), so I don't think that rule applies here? Or in
> > other words, does the following implemenation look good to you?
> > 
> > 	impl<T: ?Sized> Arc<T> {
> > 	    pub fn as_with_ref(&self) -> &WithRef<T> {
> > 		// expose
> > 		let _ = self.ptr.as_ptr() as usize;
> > 		unsafe { self.ptr.as_ref() }
> > 	    }
> > 	}
> > 
> > 	impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> > 	    fn from(b: &WithRef<T>) -> Self {
> > 		// from exposed
> > 		let ptr = unsafe { NonNull::new_unchecked(b as *const _ as usize as *mut _) };
> > 		// SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
> > 		// guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
> > 		// increment.
> > 		ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
> > 		    .deref()
> > 		    .clone()
> > 	    }
> > 	}
> > 
> > 
> > An equivalent code snippet is as below (in case anyone wants to try it
> > in miri):
> > ```rust
> >      let raw = Box::into_raw(arc);
> > 
> >      // as_with_ref()
> >      let _ = raw as usize;
> >      let reference = unsafe { &*raw };
> > 
> >      // from()
> >      let raw: *mut T = reference as *const _ as usize as  *mut _ ;
> > 
> >      // drop()
> >      let arc = unsafe { Box::from_raw(raw) };
> > ```
> 
> I don't understand why we are trying to use ptr2int to fix this.
> Simply wrapping the `T` field inside `WithRef` with `UnsafeCell`
> should be enough.
> 

BTW, how do you fix this with only wrapping `T` field in `WithRef`?

Let say `WithRef` is defined as:

struct WithRef<T> {
    refcount: Opaque<bindings::refcount_t>,
    data: UnsafeCell<T>,
}


impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
    fn from(b: &WithRef<T>) -> Self {
        let data_ptr: *mut T = b.data.get();

	let ptr = ?; // how to get a pointer to `WithRef<T>` with the
                     // provenance to the whole data?

        ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
            .deref()
            .clone()
    }
}

The `data_ptr` above only has provenance to part of the struct for the
similar reason that my proposal of (ab)using `b.refcount.get()`. Am I
missing something here?

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 
> 

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Alice Ryhl]

On 9/26/23 20:20, Boqun Feng wrote:
> On Tue, Sep 26, 2023 at 05:15:52PM +0000, Benno Lossin wrote:
> [...]
>>>
>>> But here the difference it that we only derive a `*mut` from a `&`,
>>> rather than transmute to a `&mut`, right? We only use `&` to get a
>>> pointer value (a usize), so I don't think that rule applies here? Or in
>>> other words, does the following implemenation look good to you?
>>>
>>> 	impl<T: ?Sized> Arc<T> {
>>> 	    pub fn as_with_ref(&self) -> &WithRef<T> {
>>> 		// expose
>>> 		let _ = self.ptr.as_ptr() as usize;
>>> 		unsafe { self.ptr.as_ref() }
>>> 	    }
>>> 	}
>>>
>>> 	impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
>>> 	    fn from(b: &WithRef<T>) -> Self {
>>> 		// from exposed
>>> 		let ptr = unsafe { NonNull::new_unchecked(b as *const _ as usize as *mut _) };
>>> 		// SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
>>> 		// guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
>>> 		// increment.
>>> 		ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
>>> 		    .deref()
>>> 		    .clone()
>>> 	    }
>>> 	}
>>>
>>>
>>> An equivalent code snippet is as below (in case anyone wants to try it
>>> in miri):
>>> ```rust
>>>       let raw = Box::into_raw(arc);
>>>
>>>       // as_with_ref()
>>>       let _ = raw as usize;
>>>       let reference = unsafe { &*raw };
>>>
>>>       // from()
>>>       let raw: *mut T = reference as *const _ as usize as  *mut _ ;
>>>
>>>       // drop()
>>>       let arc = unsafe { Box::from_raw(raw) };
>>> ```
>>
>> I don't understand why we are trying to use ptr2int to fix this.
>> Simply wrapping the `T` field inside `WithRef` with `UnsafeCell`
>> should be enough.
>>
> 
> BTW, how do you fix this with only wrapping `T` field in `WithRef`?
> 
> Let say `WithRef` is defined as:
> 
> struct WithRef<T> {
>      refcount: Opaque<bindings::refcount_t>,
>      data: UnsafeCell<T>,
> }
> 
> 
> impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
>      fn from(b: &WithRef<T>) -> Self {
>          let data_ptr: *mut T = b.data.get();
> 
> 	let ptr = ?; // how to get a pointer to `WithRef<T>` with the
>                       // provenance to the whole data?
> 
>          ManuallyDrop::new(unsafe { Arc::from_inner(ptr) })
>              .deref()
>              .clone()
>      }
> }
> 
> The `data_ptr` above only has provenance to part of the struct for the
> similar reason that my proposal of (ab)using `b.refcount.get()`. Am I
> missing something here?

Easiest is to wrap the entire thing:

struct WithRef<T>(UnsafeCell<ArcInner<T>>);

struct ArcInner<T> {
     refcount: Opaque<bindings::refcount_t>,
     data: T,
}

Alice

Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

[Alice Ryhl]

>> I'm concerned about this change, because an `&WithRef<T>` only has
>> immutable permissions for the allocation. No pointer derived from it
>> may be used to modify the value in the Arc, however, the drop
>> implementation of Arc will do exactly that. 
> 
> That is indeed a problem. We could put the value in an `UnsafeCell`, but
> that would lose us niche optimizations and probably also other optimizations.

This is an option. Niche optimizations don't matter for `WithRef` since
it's never directly wrapped with `Option`.

> > It also means that we
> > can't convert an Arc with refcount 1 into a UniqueArc.
> 
> I think you still can, since to do that you would consume the `Arc<T>` by
> value, thus guaranteeing that no references (and thus no `&WithRef<T>`) exist.
> So I think this would still be fine.

The problem is that if you have an `&WithRef<T>` and use that to create
an `Arc<T>`, then the raw pointer in the `Arc<T>` was created from an
immutable reference, so the same restrictions apply to that `Arc<T>`.
And if you convert it into an `UniqueArc<T>`, then the same restrictions
also apply to the `UniqueArc<T>` because it's raw pointer was derived
from the immutable reference.

Alice