linux-kernel - Re: [PATCH v1] rust: add improved version of `ForeignOwnable::borrow

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: <87msyi406u.fsf@metaspace.dk>
Date:   Wed, 23 Aug 2023 10:43:18 +0200
From:   "Andreas Hindborg (Samsung)" <nmi@...aspace.dk>
To:     Alice Ryhl <aliceryhl@...gle.com>
Cc:     alex.gaynor@...il.com, benno.lossin@...ton.me,
        bjorn3_gh@...tonmail.com, boqun.feng@...il.com, gary@...yguo.net,
        linux-kernel@...r.kernel.org, ojeda@...nel.org,
        patches@...ts.linux.dev, rust-for-linux@...r.kernel.org,
        wedsonaf@...il.com
Subject: Re: [PATCH v1] rust: add improved version of
 `ForeignOwnable::borrow_mut`


Alice Ryhl <aliceryhl@...gle.com> writes:

> Andreas Hindborg <nmi@...aspace.dk> writes:
>>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>>> index 172f563976a9..f152a562c9c3 100644
>>> --- a/rust/kernel/sync/arc.rs
>>> +++ b/rust/kernel/sync/arc.rs
>>> @@ -232,26 +232,35 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
>>>  
>>>  impl<T: 'static> ForeignOwnable for Arc<T> {
>>>      type Borrowed<'a> = ArcBorrow<'a, T>;
>>> +    // Mutable access to the `Arc` does not give any extra abilities over
>>> +    // immutable access.
>>> +    type BorrowedMut<'a> = ArcBorrow<'a, 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> {
>>> -        // 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();
>>> -
>>> -        // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
>>> -        // for the lifetime of the returned value.
>>> -        unsafe { ArcBorrow::new(inner) }
>>> -    }
>>> -
>>>      unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
>>>          // SAFETY: By the safety requirement of this function, we know that `ptr` came from
>>>          // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and
>>>          // holds a reference count increment that is transferrable to us.
>>> -        unsafe { Self::from_inner(NonNull::new(ptr as _).unwrap()) }
>>> +        unsafe { Self::from_inner(NonNull::new_unchecked(ptr as _)) }
>>>      }
>>> +
>>> +    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 = unsafe { NonNull::new_unchecked(ptr as *mut ArcInner<T>) };
>>> +
>>> +        // SAFETY: The safety requirements ensure that we will not give up our
>>> +        // foreign-owned refcount while the `ArcBorrow` is still live.
>>> +        unsafe { ArcBorrow::new(inner) }
>>> +    }
>>> +
>>> +    unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
>>> +        // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety
>>> +        // requirements for `borrow`.
>>> +        unsafe { Self::borrow(ptr) }
>>> +    }
>>
>> I am not sure this makes sense. How about splitting the trait in two,
>> immutable and mutable and only implementing the immutable one or Arc?
>
> I used this design based on what would make sense for a linked list. The
> idea is that we can have two different types of cursors for a linked
> list: immutable and mutable. The immutable cursor lets you:
>
>  * move around the linked list
>  * access the values using `borrow`
>
> The mutable cursor lets you:
>
>  * move around the linked list
>  * delete or add items to the list
>  * access the values using `borrow_mut`
>
> The mutable cursor gives you extra abilities beyond the `borrow` vs
> `borrow_mut` distinction, so we want to provide both types of cursors
> even if the pointer type is Arc. To do that, we need a trait that
> defines what it means to have mutable access to an Arc.

I don't see how that prevents this trait from being split in two?

BR Andreas