linux-kernel - Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

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: <CAH5fLgjZ91xFo4hV4dPnDXLFr9jX3na60tVt_KuNU_c6WhhzAA@mail.gmail.com>
Date: Wed, 23 Oct 2024 11:28:13 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Abdiel Janulgue <abdiel.janulgue@...il.com>
Cc: rust-for-linux@...r.kernel.org, dakr@...hat.com, 
	linux-kernel@...r.kernel.org, airlied@...hat.com, 
	miguel.ojeda.sandonis@...il.com, boqun.feng@...il.com
Subject: Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

On Wed, Oct 23, 2024 at 12:49 AM Abdiel Janulgue
<abdiel.janulgue@...il.com> wrote:
>
> Add the 'Owned' type, a simple smart pointer type that owns the
> underlying data.
>
> An object implementing `Ownable' can constructed by wrapping it in
> `Owned`, which has the advantage of allowing fine-grained control
> over it's resource allocation and deallocation.
>
> Co-developed-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
> ---
>  rust/kernel/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index ced143600eb1..3f632916bd4d 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -429,3 +429,65 @@ pub enum Either<L, R> {
>      /// Constructs an instance of [`Either`] containing a value of type `R`.
>      Right(R),
>  }
> +
> +/// A smart pointer that owns the underlying data `T`.
> +///
> +/// This is a simple smart pointer that owns the underlying data. Typically, this would be
> +/// returned as a wrapper for `T` in `T`'s constructor.
> +/// When an object adds an option of being constructed this way, in addition to implementing
> +/// `Drop`, it implements `Ownable` as well, thus having finer-grained control in where
> +/// resource allocation and deallocation happens.
> +///
> +/// # Invariants
> +///
> +/// The pointer is always valid and owns the underlying data.
> +pub struct Owned<T: Ownable> {
> +    ptr: NonNull<T>,
> +}
> +
> +impl<T: Ownable> Owned<T> {
> +    /// Creates a new smart pointer that owns `T`.
> +    ///
> +    /// # Safety
> +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> +    /// in other words, no other entity should free the underlying data.
> +    pub unsafe fn to_owned(ptr: *mut T) -> Self {

Please rename this function to from_raw to match the name used by
other similar functions.

Also, I don't love this wording. We don't really want to guarantee
that it is unique. For example, pages have one primary owner, but
there can be others who also have refcounts to the page, so it's not
really unique. I think you just want to say that `ptr` must point at a
valid value of type `T`, and it must remain valid until `ptr_drop` is
called.

> +impl<T: Ownable> Deref for Owned<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl<T: Ownable> DerefMut for Owned<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_mut() }
> +    }
> +}

We only want Deref, not DerefMut. DerefMut both requires uniqueness in
a way that is stronger than what we can really promise, and it also
implies that the value is *not* pinned, but we generally want to use
Owned with pinned things. Thus, we can't use DerefMut.

> +/// An Ownable type is a type that can be put into `Owned<T>`, and when `Owned<T>` drops,
> +/// `ptr_drop` will be called.
> +pub unsafe trait Ownable {
> +    /// # Safety
> +    /// This could only be called in the `Owned::drop` function.
> +    unsafe fn ptr_drop(ptr: *mut Self);
> +}
> +
> +impl<T: Ownable> Drop for Owned<T> {
> +    fn drop(&mut self) {
> +       // SAFETY: In Owned<T>::drop.
> +       unsafe {
> +           <T as Ownable>::ptr_drop(self.ptr.as_mut());

This uses NonNull::as_mut which creates a mutable reference. You
should use NonNull::as_ptr instead.

Also this code will look better if you move the semicolon so it is
outside of the unsafe block.


Alice