[PATCH v3 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[PATCH v3 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[Dirk Behme]

Add some missing intra doc linkage `[...]`.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---

Changes in v3: Move the non-linkage artifact to patch 2/2.

 rust/kernel/types.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index c3dc798221dbc..af30e9c0ebccb 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -271,7 +271,7 @@ 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>`] is meant to be used with FFI objects that are never interpreted by Rust code.
 ///
 /// 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:
@@ -286,7 +286,7 @@ fn drop(&mut self) {
 /// This has to be used for all values that the C side has access to, because it can't be ensured
 /// that the C side is adhering to the usual constraints that Rust needs.
 ///
-/// Using `Opaque<T>` allows to continue to use references on the Rust side even for values shared
+/// Using [`Opaque<T>`] allows to continue to use references on the Rust side even for values shared
 /// with C.
 ///
 /// # Examples
-- 
2.48.0

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

[Dirk Behme]

In the discussion [1] some `Opaque` documentation updates have
been proposed. In that discussion it was clarified that `Opaque`
is intended to be used for (partial) uninitialized or changing C
structs. And which consequences this has for using raw pointers
or the destruction. Improve the `Opaque` documentation by adding
these conclusions from that discussion.

Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

---
Changes in v3:
   * Add the "terse" proposals.
   * Move the non-linkage artifact from patch 1/2 to here.

 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.
+///
+/// Further, [`Opaque<T>`] is `!Unpin` and will not run the drop method of the contained `T`
+/// when dropped.
 ///
-/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
-///   [`bool`]).
+/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
+/// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
+/// This is useful for C structs that are not fully initialized (yet) or might change their
+/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
+/// Rust has for a value of type `T`:
+///
+/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
+///   `3` for a [`bool`]).
+/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
+///   valid *right now*.
 /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
 /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
 ///   the same value.
 /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
+/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
+///   be uninitialized, as described above.
 ///
 /// This has to be used for all values that the C side has access to, because it can't be ensured
 /// that the C side is adhering to the usual constraints that Rust needs.
-- 
2.48.0

Re: [PATCH v3 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[Andreas Hindborg]

"Dirk Behme" <dirk.behme@de.bosch.com> writes:

> Add some missing intra doc linkage `[...]`.
>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

"Dirk Behme" <dirk.behme@de.bosch.com> writes:

> In the discussion [1] some `Opaque` documentation updates have
> been proposed. In that discussion it was clarified that `Opaque`
> is intended to be used for (partial) uninitialized or changing C
> structs. And which consequences this has for using raw pointers
> or the destruction. Improve the `Opaque` documentation by adding
> these conclusions from that discussion.
>
> Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>
> ---
> Changes in v3:
>    * Add the "terse" proposals.
>    * Move the non-linkage artifact from patch 1/2 to here.
>
>  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.

> +///
> +/// Further, [`Opaque<T>`] is `!Unpin` and will not run the drop method of the contained `T`
> +/// when dropped.

Could you link `Unpin` here?

>  ///
> -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
> -///   [`bool`]).
> +/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
> +/// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
> +/// This is useful for C structs that are not fully initialized (yet) or might change their
> +/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
> +/// Rust has for a value of type `T`:
> +///
> +/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
> +///   `3` for a [`bool`]).
> +/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
> +///   valid *right now*.
>  /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
>  /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
>  ///   the same value.
>  /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).

Link `Sync` here. You might need a [`!Sync`]: `Sync` at the bottom for
it to work.

> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
> +///   be uninitialized, as described above.


Best regards,
Andreas Hindborg

Re: [PATCH v3 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[Alice Ryhl]

On Wed, Mar 05, 2025 at 06:34:37AM +0100, Dirk Behme wrote:
> Add some missing intra doc linkage `[...]`.
> 
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

>  rust/kernel/types.rs | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index c3dc798221dbc..af30e9c0ebccb 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -271,7 +271,7 @@ 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>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>  ///
>  /// 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:
> @@ -286,7 +286,7 @@ fn drop(&mut self) {
>  /// This has to be used for all values that the C side has access to, because it can't be ensured
>  /// that the C side is adhering to the usual constraints that Rust needs.
>  ///
> -/// Using `Opaque<T>` allows to continue to use references on the Rust side even for values shared
> +/// Using [`Opaque<T>`] allows to continue to use references on the Rust side even for values shared
>  /// with C.
>  ///
>  /// # Examples
> -- 
> 2.48.0
> 

Re: [PATCH v3 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[Fiona Behrens]

Dirk Behme <dirk.behme@de.bosch.com> writes:

> Add some missing intra doc linkage `[...]`.
>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

Reviewed-by: Fiona Behrens <me@kloenk.dev>

> ---
>
> Changes in v3: Move the non-linkage artifact to patch 2/2.
>
>  rust/kernel/types.rs | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index c3dc798221dbc..af30e9c0ebccb 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -271,7 +271,7 @@ 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>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>  ///
>  /// 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:
> @@ -286,7 +286,7 @@ fn drop(&mut self) {
>  /// This has to be used for all values that the C side has access to, because it can't be ensured
>  /// that the C side is adhering to the usual constraints that Rust needs.
>  ///
> -/// Using `Opaque<T>` allows to continue to use references on the Rust side even for values shared
> +/// Using [`Opaque<T>`] allows to continue to use references on the Rust side even for values shared
>  /// with C.
>  ///
>  /// # Examples

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

[Boqun Feng]

On Wed, Mar 05, 2025 at 06:34:38AM +0100, Dirk Behme wrote:
> In the discussion [1] some `Opaque` documentation updates have
> been proposed. In that discussion it was clarified that `Opaque`
> is intended to be used for (partial) uninitialized or changing C
> structs. And which consequences this has for using raw pointers
> or the destruction. Improve the `Opaque` documentation by adding
> these conclusions from that discussion.
> 
> Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> 
> ---
> Changes in v3:
>    * Add the "terse" proposals.
>    * Move the non-linkage artifact from patch 1/2 to here.
> 
>  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.
> +///
> +/// Further, [`Opaque<T>`] is `!Unpin` and will not run the drop method of the contained `T`
> +/// when dropped.

I would move "will not run the drop method of contained `T` when
dropped" to the "Initialization invariant" above because they are
logically related.

Regards,
Boqun

>  ///
> -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
> -///   [`bool`]).
> +/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
> +/// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
> +/// This is useful for C structs that are not fully initialized (yet) or might change their
> +/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
> +/// Rust has for a value of type `T`:
> +///
> +/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
> +///   `3` for a [`bool`]).
> +/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
> +///   valid *right now*.
>  /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
>  /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
>  ///   the same value.
>  /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
> +///   be uninitialized, as described above.
>  ///
>  /// This has to be used for all values that the C side has access to, because it can't be ensured
>  /// that the C side is adhering to the usual constraints that Rust needs.
> -- 
> 2.48.0
> 

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

[Boqun Feng]

On Wed, Mar 05, 2025 at 08:47:42AM +0100, Andreas Hindborg wrote:
> "Dirk Behme" <dirk.behme@de.bosch.com> writes:
> 
> > In the discussion [1] some `Opaque` documentation updates have
> > been proposed. In that discussion it was clarified that `Opaque`
> > is intended to be used for (partial) uninitialized or changing C
> > structs. And which consequences this has for using raw pointers
> > or the destruction. Improve the `Opaque` documentation by adding
> > these conclusions from that discussion.
> >
> > Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
> > Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
> > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> >
> > ---
> > Changes in v3:
> >    * Add the "terse" proposals.
> >    * Move the non-linkage artifact from patch 1/2 to here.
> >
> >  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

Regards,
Boqun

> > +///
> > +/// Further, [`Opaque<T>`] is `!Unpin` and will not run the drop method of the contained `T`
> > +/// when dropped.
> 
> Could you link `Unpin` here?
> 
> >  ///
> > -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
> > -///   [`bool`]).
> > +/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
> > +/// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
> > +/// This is useful for C structs that are not fully initialized (yet) or might change their
> > +/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
> > +/// Rust has for a value of type `T`:
> > +///
> > +/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
> > +///   `3` for a [`bool`]).
> > +/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
> > +///   valid *right now*.
> >  /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
> >  /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
> >  ///   the same value.
> >  /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
> 
> Link `Sync` here. You might need a [`!Sync`]: `Sync` at the bottom for
> it to work.
> 
> > +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
> > +///   be uninitialized, as described above.
> 
> 
> Best regards,
> Andreas Hindborg
> 
> 
> 

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

[Andreas Hindborg]

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

> On Wed, Mar 05, 2025 at 08:47:42AM +0100, Andreas Hindborg wrote:
>> "Dirk Behme" <dirk.behme@de.bosch.com> writes:
>>
>> > In the discussion [1] some `Opaque` documentation updates have
>> > been proposed. In that discussion it was clarified that `Opaque`
>> > is intended to be used for (partial) uninitialized or changing C
>> > structs. And which consequences this has for using raw pointers
>> > or the destruction. Improve the `Opaque` documentation by adding
>> > these conclusions from that discussion.
>> >
>> > Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> > Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
>> > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> >
>> > ---
>> > Changes in v3:
>> >    * Add the "terse" proposals.
>> >    * Move the non-linkage artifact from patch 1/2 to here.
>> >
>> >  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?


Best regards,
Andreas Hindborg

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

[Boqun Feng]

On Wed, Mar 05, 2025 at 06:32:11PM +0100, Andreas Hindborg wrote:
> "Boqun Feng" <boqun.feng@gmail.com> writes:
> 
> > On Wed, Mar 05, 2025 at 08:47:42AM +0100, Andreas Hindborg wrote:
> >> "Dirk Behme" <dirk.behme@de.bosch.com> writes:
> >>
> >> > In the discussion [1] some `Opaque` documentation updates have
> >> > been proposed. In that discussion it was clarified that `Opaque`
> >> > is intended to be used for (partial) uninitialized or changing C
> >> > structs. And which consequences this has for using raw pointers
> >> > or the destruction. Improve the `Opaque` documentation by adding
> >> > these conclusions from that discussion.
> >> >
> >> > Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
> >> > Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
> >> > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> >> >
> >> > ---
> >> > Changes in v3:
> >> >    * Add the "terse" proposals.
> >> >    * Move the non-linkage artifact from patch 1/2 to here.
> >> >
> >> >  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.

Thoughts?

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
> 

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

[Andreas Hindborg]

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

> On Wed, Mar 05, 2025 at 06:32:11PM +0100, Andreas Hindborg wrote:
>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>>
>> > On Wed, Mar 05, 2025 at 08:47:42AM +0100, Andreas Hindborg wrote:
>> >> "Dirk Behme" <dirk.behme@de.bosch.com> writes:
>> >>
>> >> > In the discussion [1] some `Opaque` documentation updates have
>> >> > been proposed. In that discussion it was clarified that `Opaque`
>> >> > is intended to be used for (partial) uninitialized or changing C
>> >> > structs. And which consequences this has for using raw pointers
>> >> > or the destruction. Improve the `Opaque` documentation by adding
>> >> > these conclusions from that discussion.
>> >> >
>> >> > Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> >> > Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
>> >> > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> >> >
>> >> > ---
>> >> > Changes in v3:
>> >> >    * Add the "terse" proposals.
>> >> >    * Move the non-linkage artifact from patch 1/2 to here.
>> >> >
>> >> >  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.


Best regards,
Andreas Hindborg

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

[Boqun Feng]

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 = ... };

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).

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.

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
> 

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

[Andreas Hindborg]

"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

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

[Alice Ryhl]

On Wed, Mar 05, 2025 at 10:07:33PM +0100, Andreas Hindborg wrote:
> 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.

There's a really important wrinckle here. Having multiple aliasing &mut
Opaque<T> does not trigger immediate UB, but that doesn't mean that such
mutable references are safe to hand out to end-users of the API. The end
user could call `swap` on the mutable references, and which would be
really bad if it uses intrusive linked lists.

I.e., you could only safely hand out Pin<&mut Opaque<T>> to the end-user
of the API.

But of course you still need the guarantee. Pin is just a library type,
and if having a &mut T is immediate UB, then so is having a Pin<&mut T>.

Alice

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

[Benno Lossin]

On Thu Mar 6, 2025 at 10:42 AM CET, Andreas Hindborg wrote:
> "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.

I disagree, to me "aliasing pointer" means having two pointers that
point to the same value and that you can use in an interleaved fashion
for writes and reads.

> 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.

The rules aren't completely set and IIRC you *can* create multiple
references as long as the value they point to is not read or written to
while they exist. But I wouldn't bet anything on that...

> 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.

Yes it is legal to have multiple `&mut Opaque<T>` pointing to the same
value (and writing/reading through the `get` method or directly by
having another `Opaque<T>` value).

> 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.

This wording doesn't sound right to me, since it's still illegal to have
multiple `&mut T` that point to a value inside of an `Opaque<T>`.

---
Cheers,
Benno

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

[Boqun Feng]

On Thu, Mar 06, 2025 at 10:45:00AM +0000, Benno Lossin wrote:
> On Thu Mar 6, 2025 at 10:42 AM CET, Andreas Hindborg wrote:
> > "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.
> 
> I disagree, to me "aliasing pointer" means having two pointers that
> point to the same value and that you can use in an interleaved fashion
> for writes and reads.
> 
> > 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.
> 
> The rules aren't completely set and IIRC you *can* create multiple
> references as long as the value they point to is not read or written to
> while they exist. But I wouldn't bet anything on that...
> 
> > 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.
> 
> Yes it is legal to have multiple `&mut Opaque<T>` pointing to the same
> value (and writing/reading through the `get` method or directly by
> having another `Opaque<T>` value).
> 
> > 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.
> 
> This wording doesn't sound right to me, since it's still illegal to have
> multiple `&mut T` that point to a value inside of an `Opaque<T>`.
> 

Which wording proposed so far would sound right to you? Or you have a
proposal that could describe this rigorously?

Regards,
Boqun

> ---
> Cheers,
> Benno
> 

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

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Wed, Mar 05, 2025 at 10:07:33PM +0100, Andreas Hindborg wrote:
>> 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.
>
> There's a really important wrinckle here. Having multiple aliasing &mut
> Opaque<T> does not trigger immediate UB, but that doesn't mean that such
> mutable references are safe to hand out to end-users of the API. The end
> user could call `swap` on the mutable references, and which would be
> really bad if it uses intrusive linked lists.
>
> I.e., you could only safely hand out Pin<&mut Opaque<T>> to the end-user
> of the API.
>
> But of course you still need the guarantee. Pin is just a library type,
> and if having a &mut T is immediate UB, then so is having a Pin<&mut T>.

Right, this is a really important caveat. Thanks.



Best regards,
Andreas Hindborg

Re: [PATCH v3 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[Miguel Ojeda]

On Wed, Mar 5, 2025 at 6:35 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
> Add some missing intra doc linkage `[...]`.
>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

Applied (this one only) to `rust-next` -- thanks everyone!

    [ Reworded. - Miguel ]

Cheers,
Miguel

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

[Miguel Ojeda]

On Fri, Mar 7, 2025 at 9:22 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Right, this is a really important caveat. Thanks.

Yeah, we should mention it. Ideally with an example of a concrete `T`
that is safe/not safe to swap.

Cheers,
Miguel