linux-kernel - Re: [PATCH v10 1/5] rust: types: Add Ownable/Owned types

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] [day] [month] [year] [list]

Message-Id: <D9VS2Q4IX7LH.3JLXQUMWYJ2FP@kernel.org>
Date: Wed, 14 May 2025 11:32:19 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Oliver Mangold" <oliver.mangold@...me>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Asahi Lina" <lina@...hilina.net>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 1/5] rust: types: Add Ownable/Owned types

On Fri May 2, 2025 at 11:02 AM CEST, Oliver Mangold wrote:
> +/// Types that may be owned by Rust code or borrowed, but have a lifetime managed by C code.
> +///
> +/// It allows such types to define their own custom destructor function to be called when
> +/// a Rust-owned reference is dropped.
> +///
> +/// This is usually implemented by wrappers to existing structures on the C side of the code.

The docs should mention `AlwaysRefCounted` and when to use it instead of
this trait. We should probably also backlink from `AlwaysRefCounted` to
`Ownable`.

> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that:
> +/// - Any objects owned by Rust as [`Owned<T>`] stay alive while that owned reference exists (i.e.
> +///   until the [`release()`](Ownable::release) trait method is called).

I don't immediately understand what this means. How about "Any value of
type `Self` needs to be stored as [`Owned<Self>`]."? And then ask in
`Owned::from_raw` for a pointer that is valid indefinitely (or at least
until `release` is called).

> +/// - That the C code follows the usual mutable reference requirements. That is, the kernel will
> +///   never mutate the [`Ownable`] (excluding internal mutability that follows the usual rules)
> +///   while Rust owns it.

I feel like this requirement is better put on the `Owned::from_raw`
function.

> +pub unsafe trait Ownable {
> +    /// Releases the object (frees it or returns it to foreign ownership).
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the object is no longer referenced after this call.
> +    unsafe fn release(this: NonNull<Self>);
> +}
> +
> +/// A subtrait of Ownable that asserts that an [`Owned<T>`] or `&mut Owned<T>` Rust reference
> +/// may be dereferenced into a `&mut T`.

The "A subtrait of Ownable that asserts" sounds a bit clumsy to me, how
about "Type where [`Owned<Self>`] derefs to `&mut Self`."?

> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that access to a `&mut T` is safe, implying that it is okay to call
> +/// [`core::mem::swap`] on the `Ownable`. This excludes pinned types (meaning: most kernel types).

I don't like that we put this requirement here, since it's actually
something that should be asserted by `Owned::from_raw`.
The reason for that is that anyone can call `Owned::from_raw` with a
pointer pointing to `Self` and there is no safety requirement on that
function that ensures the correctness of the `DerefMut` impl.

> +pub unsafe trait OwnableMut: Ownable {}

I don't like the name, but at the same time I also have no good
suggestion :( I'll think some more about it.

---
Cheers,
Benno