Re: [PATCH v3 2/2] rust: types: `Opaque` doc: Add some destructor description

[Andreas Hindborg <a.hindborg@kernel.org>]

"Boqun Feng" <boqun.feng@gmail.com> writes:

> On Wed, Mar 05, 2025 at 10:07:33PM +0100, Andreas Hindborg wrote:
>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>>
> [...]
>> >> >> >  rust/kernel/types.rs | 26 +++++++++++++++++++++-----
>> >> >> >  1 file changed, 21 insertions(+), 5 deletions(-)
>> >> >> >
>> >> >> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> >> >> > index af30e9c0ebccb..f370cdb48a648 100644
>> >> >> > --- a/rust/kernel/types.rs
>> >> >> > +++ b/rust/kernel/types.rs
>> >> >> > @@ -271,17 +271,33 @@ fn drop(&mut self) {
>> >> >> >
>> >> >> >  /// Stores an opaque value.
>> >> >> >  ///
>> >> >> > -/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>> >> >> > +/// [`Opaque<T>`] opts out of the following Rust language invariants for the contained `T`
>> >> >> > +/// by using [`UnsafeCell`]:
>> >> >> >  ///
>> >> >> > -/// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
>> >> >> > -/// It gets rid of all the usual assumptions that Rust has for a value:
>> >> >> > +/// * Initialization invariant - the contained value is allowed to be uninitialized and
>> >> >> > +///   contain invalid bit patterns.
>> >> >> > +/// * Immutability invariant - [`Opaque<T>`] allows interior mutability.
>> >> >> > +/// * Uniqueness invariant - [`Opaque<T>`] allows aliasing of shared references.
>> >> >>
>> >> >> This last one is wrong (I know it's probably my fault, sorry). It should be:
>> >> >>
>> >> >>   /// * Uniqueness invariant - [`Opaque<T>`] allows aliasing of **exclusive** references.
>> >> >>
>> >> >
>> >> > Hmm... are we trying to say "`&mut Opaque<T>` still cannot mean
>> >> > noalias" here? If so I feel the wording might be confusing. I would
>> >> > suggest we don't use "uniqueness" here. Maybe something like:
>> >> >
>> >> > * Always aliased invariant - `&mut` [`Opaque<T>`] is not necessarily a
>> >> >   unique pointer, and thus the compiler cannot just make aliasing
>> >> >   assumptions.
>> >> >
>> >> > (I use the wording from UnsafePinned[1], maybe there could be better
>> >> > wording)
>> >> >
>> >> > [1]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#summary
>> >>
>> >> I like my wording better. It says the same with fewer words, and we have
>> >> a more wordy section below anyway. We could combine:
>> >>
>> >>  * Uniqueness invariant - [`Opaque<T>`] allows aliasing of **exclusive**
>> >>    references. That is, `&mut` [`Opaque<T>`] is not necessarily a unique
>> >>    pointer.
>> >>
>> >>
>> >> How is that?
>> >>
>> >
>> > I think my biggest problem is the word "allows", the phrase "allows
>> > aliasing" sounds like Opaque<T> provide something *optional*, however,
>> > as I understand it, Opaque<T> here just disallows certain optimization
>> > from compiler, in other words, programmers want to use `Opaque<T>` to
>> > forbid something, hence "allows" may not be the good word to use here?
>> >
>> > How about:
>> >
>> >   * Uniqueness invariant - [`Opaque<T>`] disallow alias assumptions
>> >     made by compiler on an *exclusive* references.
>>
>> I don't object, but I feel like we should be able to define this within
>> the context of the Rust language, without going into details about
>> compiler internals.
>>
>> Without `Opaque` it is illegal to have aliased `&mut T`. With `Opaque`,
>> it is legal to have aliased `&mut Opaque<T>`. Or at least that is my
>> understanding.
>>
>
> Oh, I see. That's why you used "allows", however, allow me to explain
> why it looks confusing to me. For `&mut T`, we can still have an
> aliasing pointer (a pointer that points to the same variable as the
> mutable reference):
>
> 	let mut x = ...;
>
> 	let mut_ref = &mut x;
> 	let aliased_ptr = mut_ref as *mut _;
>
> it's just that if we dereference the pointer while the `&mut T` exists,
> it's UB, e.g.
>
> 	unsafe { *aliased_ptr = ... };

Right.

>
> and `Opaque<T>` allows above to be not a UB (there still are rules like
> we cannot have data races, violating these rules would still be UB).

Yes.

> So to me "aliasing" can mean "having an aliasing pointer", and that as
> shown above can be provided by both non-Opaque and Opaque cases. I
> wonder whether "allow aliased/aliasing accesses" is better than "allow
> aliasing" here.

Yes, I think that would be better.

I had another thing in mind when I wrote the text, and I am wondering if
it is incorrect. As far as I know it is normally illegal to have more
than one `&mut T` to the same `T`, even if no access occur through these
references.

With `Opaque<T>`, is it still illegal to have two `&mut Opaque<T>` to
the same `T` in existence? I thought it was not illegal in this case.

At any rate, your wording is probably the most accurate:

  * Uniqueness invariant - [`Opaque<T>`] disallow alias assumptions
    made by compiler on an *exclusive* references.

Alternatively, if we do not want to refer to compiler assumptions:

  * Uniqueness invariant - [`Opaque<T>`] makes aliasing accesses to the
    underlying `T` well defined.



Best regards,
Andreas Hindborg