Message-ID: <aFUHWtE2dm3_-Rbs@mango>
Date: Fri, 20 Jun 2025 07:01:49 +0000
From: Oliver Mangold <oliver.mangold@...me>
To: Benno Lossin <lossin@...nel.org>
Cc: 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>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 1/5] rust: types: Add Ownable/Owned types

On 250618 2322, Benno Lossin wrote:
> On Tue Jun 17, 2025 at 11:58 AM CEST, Oliver Mangold wrote:
> > On 250514 1132, Benno Lossin wrote:
> >> On Fri May 2, 2025 at 11:02 AM CEST, Oliver Mangold wrote:
> >> > +///
> >> > +/// # 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>`]."?
> >
> > Let me think. The safety requirements here talk about safety of
> > implementing the trait.  But if you have a `Self` which is not wrapped, you
> > still cannot create an `Owned<Self>` in safe code. It's different from an
> > `AlwaysRefCounted`, where an `ARef<Self>` can be created from a `&Self`.
> 
> That might be true, but AFAIK this trait is designed to be used for
> stuff that has a `create_foo` and `destroy_foo` function in C returning
> and taking a raw pointer to `foo` respectively. So creating it on the
> stack doesn't make sense.

I didn't mean creating one on the stack, but keeping it in a raw pointer or
`NonNull<T>`, not bothering to wrap in in an `Owned<T>`. But doesn't
matter. In any case in v11 (which predates your answer), I moved this
requirement to `Owned::from_raw()`, as, you asked below, which should be
okay as that function is the only way to create an `Owned<T>`. But I can
add the "needs to be stored as `Owned<Self>`" requirement, if you think it
is important.


> If we do want to make this trait more general, then we can do so, but
> this is my current understanding.
> 
> >> And then ask in
> >> `Owned::from_raw` for a pointer that is valid indefinitely (or at least
> >> until `release` is called).
> >
> > So, hmm, I think one could even move this safety requirement to `Owned::from_raw()`.
> >
> >> > +/// - 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.
> >
> > Together with the above, this would leave to safety requirements for `Ownable.
> > Make `Ownable` a safe trait, then? Instead of safety requirements just add an invariant:
> >
> >     # Invariant
> >
> >     An `Owned<Self>` represents a unique reference to a `Self`, thus holding
> >     an `Owned<Self>` or `&mut Owned<Self>` allows one to assume that the object
> >     is not accessed concurrently from elsewhere.
> >
> > Not sure what is best. Would that make sense?
> 
> Making it safe makes sense, when we can move all requirements to
> `Owned::from_raw`. I don't think the invariants section makes sense, how
> would the trait have any influence in that when `Owned::from_raw`
> already guarantees it?

I think you are right on that. Let's not do that.

Best,

Oliver

Powered by blists - more mailing lists