Messages in this thread | | | Date | Sat, 15 Jul 2023 13:38:18 +0000 | From | Benno Lossin <> | Subject | Re: [PATCH v1] rust: add improved version of `ForeignOwnable::borrow_mut` |
| |
On 10.07.23 09:46, Alice Ryhl wrote: > Previously, the `ForeignOwnable` trait had a method called `borrow_mut` > that was intended to provide mutable access to the inner value. However, > the method accidentally made it possible to change the address of the > object being modified, which usually isn't what we want. (And when we > want that, it can be done by calling `from_foreign` and `into_foreign`, > like how the old `borrow_mut` was implemented.) > > In this patch, we introduce an alternate definition of `borrow_mut` that > solves the previous problem. Conceptually, given a pointer type `P` that > implements `ForeignOwnable`, the `borrow_mut` method gives you the same > kind of access as an `&mut P` would, except that it does not let you > change the pointer `P` itself. > > This is analogous to how the existing `borrow` method provides the same > kind of access to the inner value as an `&P`. > > Note that for types like `Arc`, having an `&mut Arc<T>` only gives you > immutable access to the inner `T`. This is because mutable references > assume exclusive access, but there might be other handles to the same > reference counted value, so the access isn't exclusive. The `Arc` type > implements this by making `borrow_mut` return the same type as `borrow`. > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
-- Cheers, Benno
> --- > > This patch depends on https://lore.kernel.org/all/20230706094615.3080784-1-aliceryhl@google.com/ > > rust/kernel/sync/arc.rs | 31 +++++++++----- > rust/kernel/types.rs | 93 ++++++++++++++++++++++++++++++----------- > 2 files changed, 89 insertions(+), 35 deletions(-) > > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs > index d479f8da8f38..1c2fb36906b6 100644 > --- a/rust/kernel/types.rs > +++ b/rust/kernel/types.rs > @@ -20,66 +20,111 @@ > /// This trait is meant to be used in cases when Rust objects are stored in C objects and > /// eventually "freed" back to Rust. > pub trait ForeignOwnable: Sized { > - /// Type of values borrowed between calls to [`ForeignOwnable::into_foreign`] and > - /// [`ForeignOwnable::from_foreign`]. > + /// Type used to immutably borrow a value that is currently foreign-owned. > type Borrowed<'a>; > > + /// Type used to mutably borrow a value that is currently foreign-owned. > + type BorrowedMut<'a>; > + > /// Converts a Rust-owned object to a foreign-owned one. > /// > /// The foreign representation is a pointer to void. > fn into_foreign(self) -> *const core::ffi::c_void; > > - /// Borrows a foreign-owned object. > - /// > - /// # Safety > - /// > - /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for > - /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet. > - unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Self::Borrowed<'a>; > - > /// Converts a foreign-owned object back to a Rust-owned one. > /// > /// # Safety > /// > - /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for > - /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet. > - /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for > - /// this object must have been dropped. > + /// The provided pointer must have been returned by a previous call to [`into_foreign`], and it > + /// must not be passed to `from_foreign` more than once. > + /// > + /// [`into_foreign`]: Self::into_foreign > unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self; > + > + /// Borrows a foreign-owned object immutably. > + /// > + /// This method provides a way to access a foreign-owned value from Rust immutably. It provides > + /// you with exactly the same abilities as an `&Self` when the value is Rust-owned. > + /// > + /// # Safety > + /// > + /// The provided pointer must have been returned by a previous call to [`into_foreign`], and if > + /// the pointer is ever passed to [`from_foreign`], then that call must happen after the end of > + /// the lifetime 'a. > + /// > + /// [`into_foreign`]: Self::into_foreign > + /// [`from_foreign`]: Self::from_foreign > + unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Self::Borrowed<'a>; > + > + /// Borrows a foreign-owned object mutably. > + /// > + /// This method provides a way to access a foreign-owned value from Rust mutably. It provides > + /// you with exactly the same abilities as an `&mut Self` when the value is Rust-owned, except > + /// that this method does not let you swap the foreign-owned object for another. (That is, it > + /// does not let you change the address of the void pointer that the foreign code is storing.) > + /// > + /// Note that for types like [`Arc`], an `&mut Arc<T>` only gives you immutable access to the > + /// inner value, so this method also only provides immutable access in that case. > + /// > + /// In the case of `Box<T>`, this method gives you the ability to modify the inner `T`, but it > + /// does not let you change the box itself. That is, you cannot change which allocation the box > + /// points at. > + /// > + /// # Safety > + /// > + /// The provided pointer must have been returned by a previous call to [`into_foreign`], and if > + /// the pointer is ever passed to [`from_foreign`], then that call must happen after the end of > + /// the lifetime 'a. > + /// > + /// The lifetime 'a must not overlap with the lifetime of any other call to [`borrow`] or > + /// `borrow_mut` on the same object. > + /// > + /// [`into_foreign`]: Self::into_foreign > + /// [`from_foreign`]: Self::from_foreign > + /// [`borrow`]: Self::borrow > + /// [`Arc`]: crate::sync::Arc > + unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> Self::BorrowedMut<'a>; > } > > impl<T: 'static> ForeignOwnable for Box<T> { > type Borrowed<'a> = &'a T; > + type BorrowedMut<'a> = &'a mut T; > > fn into_foreign(self) -> *const core::ffi::c_void { > Box::into_raw(self) as _ > } > > - unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T { > - // SAFETY: The safety requirements for this function ensure that the object is still alive, > - // so it is safe to dereference the raw pointer. > - // The safety requirements of `from_foreign` also ensure that the object remains alive for > - // the lifetime of the returned value. > - unsafe { &*ptr.cast() } > - } > - > unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { > // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous > // call to `Self::into_foreign`. > unsafe { Box::from_raw(ptr as _) } > } > + > + unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T { > + // SAFETY: The safety requirements of this method ensure that the object remains alive and > + // immutable for the duration of 'a. > + unsafe { &*ptr.cast() } > + } > + > + unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> &'a mut T { > + // SAFETY: The safety requirements of this method ensure that the pointer is valid and that > + // nothing else will access the value for the duration of 'a. > + unsafe { &mut *ptr.cast_mut().cast() } > + } > } > > impl ForeignOwnable for () { > type Borrowed<'a> = (); > + type BorrowedMut<'a> = (); > > fn into_foreign(self) -> *const core::ffi::c_void { > core::ptr::NonNull::dangling().as_ptr() > } > > - unsafe fn borrow<'a>(_: *const core::ffi::c_void) -> Self::Borrowed<'a> {} > - > unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {} > + > + unsafe fn borrow<'a>(_: *const core::ffi::c_void) -> Self::Borrowed<'a> {} > + unsafe fn borrow_mut<'a>(_: *const core::ffi::c_void) -> Self::BorrowedMut<'a> {} > } > > /// Runs a cleanup function/closure when dropped. > > 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) } > + } > } > > base-commit: d2e3115d717197cb2bc020dd1f06b06538474ac3 > prerequisite-patch-id: b493b9015cb19f599c4bc03127733193b11ca822 > -- > 2.41.0.255.g8b1d071c50-goog >
| |