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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a438231d-8a31-4a3f-932b-3f1e79fa33d9@ryhl.io>
Date:   Tue, 26 Sep 2023 23:27:42 +0200
From:   Alice Ryhl <alice@...l.io>
To:     Boqun Feng <boqun.feng@...il.com>,
        Benno Lossin <benno.lossin@...ton.me>
Cc:     Alice Ryhl <aliceryhl@...gle.com>, Gary Guo <gary@...yguo.net>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        rust-for-linux@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Andreas Hindborg <a.hindborg@...sung.com>,
        linux-kernel@...r.kernel.org,
        Wedson Almeida Filho <walmeida@...rosoft.com>
Subject: Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of
 `WithRef`



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