Message-ID: <aGg4sIORQiG02IoD@Mac.home>
Date: Fri, 4 Jul 2025 13:25:20 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Gary Guo <gary@...yguo.net>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	lkmm@...ts.linux.dev, linux-arch@...r.kernel.org,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>, Will Deacon <will@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Mark Rutland <mark.rutland@....com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Lyude Paul <lyude@...hat.com>, Ingo Molnar <mingo@...nel.org>,
	Mitchell Levy <levymitchell0@...il.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v5 04/10] rust: sync: atomic: Add generic atomics

On Mon, Jun 23, 2025 at 12:09:21PM -0700, Boqun Feng wrote:
> On Mon, Jun 23, 2025 at 07:30:19PM +0100, Gary Guo wrote:
> > On Sun, 22 Jun 2025 22:19:44 -0700
> > Boqun Feng <boqun.feng@...il.com> wrote:
> > 
> > > On Sat, Jun 21, 2025 at 12:32:12PM +0100, Gary Guo wrote:
> > > [...]
> > > > > +#[repr(transparent)]
> > > > > +pub struct Atomic<T: AllowAtomic>(Opaque<T>);  
> > > > 
> > > > This should store `Opaque<T::Repr>` instead.
> > > >   
> > > 
> > > "should" is a strong word ;-) If we still use `into_repr`/`from_repr`
> > > it's a bit impossible, because Atomic::new() wants to be a const
> > > function, so it requires const_trait_impl I believe.
> > > 
> > > If we require transmutability as a safety requirement for `AllowAtomic`,
> > > then either `T` or `T::Repr` is fine.
> > > 
> > > > The implementation below essentially assumes that this is
> > > > `Opaque<T::Repr>`:
> > > > * atomic ops cast this to `*mut T::Repr`
> > > > * load/store operates on `T::Repr` then converts to `T` with
> > > >   `T::from_repr`/`T::into_repr`.
> > > >   
> > > 
> > > Note that we only require one direction of strong transmutability, that
> > > is: for every `T`, it must be able to safe transmute to a `T::Repr`, for
> > > `T::Repr` -> `T` transmutation, only if it's a result of a `transmute<T,
> > > T::Repr>()`. This is mostly due to potential support for unit-only enum.  
> > > E.g. using an atomic variable to represent a finite state.
> > > 
> > > > Note tha the transparent new types restriction on `AllowAtomic` is not
> > > > sufficient for this, as I can define
> > > >   
> > > 
> > > Nice catch! I do agree we should disallow `MyWeirdI32`, and I also agree
> > > that we should put transmutability as safety requirement for
> > > `AllowAtomic`. However, I would suggest we still keep
> > > `into_repr`/`from_repr`, and require the implementation to make them
> > > provide the same results as transmute(), as a correctness precondition
> > > (instead of a safety precondition), in other words, you can still write
> > > a `MyWeirdI32`, and it won't cause safety issues, but it'll be
> > > incorrect.
> > > 
> > > The reason why I think we should keep `into_repr`/`from_repr` but add
> > > a correctness precondition is that they are easily to implement as safe
> > > code for basic types, so it'll be better than a transmute() call. Also
> > > considering `Atomic<*mut T>`, would transmuting between integers and
> > > pointers act the same as expose_provenance() and
> > > from_exposed_provenance()?
> > 
> > Okay, this is more problematic than I thought then. For pointers, you
> 
> Welcome to my nightmare ;-)
> 
> > cannot just transmute between from pointers to usize (which is its
> > Repr):
> > * Transmuting from pointer to usize discards provenance
> > * Transmuting from usize to pointer gives invalid provenance
> > 
> > We want neither behaviour, so we must store `usize` directly and
> > always call into repr functions.
> > 
> 
> If we store `usize`, how can we support the `get_mut()` then? E.g.
> 
>     static V: i32 = 32;
> 
>     let mut x = Atomic::new(&V as *const i32 as *mut i32);
>     // ^ assume we expose_provenance() in new().
> 
>     let ptr: &mut *mut i32 = x.get_mut(); // which is `&mut self.0.get()`.
> 
>     let ptr_val = *ptr; // Does `ptr_val` have the proper provenance?
> 

There are a few off-list discussions, and I've been trying some
experiment myself, here are a few points/concepts that will help future
discussion or documentation, so I put it down here:

* Round-trip transmutability (thank Benno for the name!).

  We realize this should be a safety requirement of `AllowAtomic` type
  (i.e. the type that can be put in a Atomic<T>). What it means is:

  - If `T: AllowAtomic`, transmute() from `T` to `T::Repr` is always
    safe and
  - if a value of `T::Repr` is a result of transmute() from `T` to
    `T::Repr`, then `transmute()` for that value to `T` is also safe.

  This essentially means a valid bit pattern of `T: AllowAtomic` has to
  be a valid bit pattern of `T::Repr`.

  This is needed because the atomic framework operates on `T::Repr` to
  implement atomic operations on `T`.

  Note that this is more relaxed than bi-direct transmutability (i.e.
  transmute() between `T` and `T::Repr`) because we want to support
  atomic type over unit-only enums:

    #[repr(i32)]
    pub enum State {
        Init = 0,
	Working = 1,
	Done = 2,
    }

  This should be really helpful to support atomics as states, for
  example:

    https://lore.kernel.org/rust-for-linux/20250702-module-params-v3-v14-1-5b1cc32311af@kernel.org/

* transmute()-equivalent from_repr() and into_repr().

  (This is not a safety requirement)

  from_repr() and into_repr(), if exist, should behave like transmute()
  on the bit pattern of the results, in other words, bit patterns of `T`
  or `T::Repr` should stay the same before and after these operations.

  Of course if we remove them and replace with transmute(), same result.

  This reflects the fact that customized atomic types should store
  unmodified bit patterns into atomic variables, and this make atomic
  operations don't have weird behavior [1] when combined with new(),
  from_ptr() and get_mut().

* Provenance preservation.

  (This is not a safety requirement for Atomic itself)

  For a `Atomic<*mut T>`, it should preserve the provenance of the
  pointer that has been stored into it, i.e. the load result from a
  `Atomic<*mut T>` should have the same provenance.

  Technically, without this, `Atomic<*mut T>` still work without any
  safety issue itself, but the user of it must maintain the provenance
  themselves before store or after load.

  And it turns out it's not very hard to prove the current
  implementation achieve this:

  - For a non-atomic operation done on the atomic variable, they are
    already using pointer operation, so the provenance has been
    preserved.
  - For an atomic operation, since they are done via inline asm code, in
    Rust's abstract machine, they can be treated as pointer read and
    write:

    a) A load of the atomic can be treated as a pointer read and then
       exposing the provenance.
    b) A store of the atomic can be treated as a pointer write with a
       value created with the exposed provenance.

    And our implementation, thanks to no arbitrary type coercion,
    already guarantee that for each a) there is a from_repr() after and
    for each b) there is a into_repr() before. And from_repr() acts as
    a with_exposed_provenance() and into_repr() acts as a
    expose_provenance(). Hence the provenance is preserved.

  Note this is a global property and it has to proven at `Atomic<T>`
  level.

Regards,
Boqun

Powered by blists - more mailing lists