Message-Id: <DB3YRHR9RN8Z.29926G08T7KZ0@kernel.org>
Date: Sat, 05 Jul 2025 10:04:04 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>
Cc: "Gary Guo" <gary@...yguo.net>, <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>, "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 Sat Jul 5, 2025 at 1:21 AM CEST, Boqun Feng wrote:
> On Sat, Jul 05, 2025 at 12:38:05AM +0200, Benno Lossin wrote:
> [..]
>> >> >   (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().
>> >> 
>> >> I remember that this was required to support types like `(u8, u16)`? If
>> >
>> > My bad, I forgot to put the link to [1]...
>> >
>> > [1]: https://lore.kernel.org/rust-for-linux/20250621123212.66fb016b.gary@garyguo.net/
>> >
>> > Basically, without requiring from_repr() and into_repr() to act as a
>> > transmute(), you can have weird types in Atomic<T>.
>> 
>> Ah right, I forgot some context... Is this really a problem? I mean it's
>
> It's not a problem for safety, so it's not a safety requirement. But I
> really don't see a reason why we want to support this. Not supporting
> this makes the atomic implementation reasoning easier.

Yeah.

>> weird sure, but if someone needs this, then it's fine?
>> 
>
> They can always play the !value game outside atomic, i.e. !value before
> store and !value after load, so I don't think it's reasonable request.

That's true, yeah let's forbid this :)

>> > `(u8, u16)` (in case it's not clear to other audience, it's tuple with a
>> > `u8` and a `u16` in it, so there is a 8-bit hole) is not going to
>> > support until we have something like a `Atomic<MaybeUninit<i32>>`.
>> 
>> Ahh right we also had this issue, could you also include that in your
>> writeup? :)
>> 
>
> Sure, I will put it in a limitation section maybe.
>
>> >> yes, then it would be good to include a paragraph like the one above for
>> >> enums :)
>> >> 
>> >> > * 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.
>> >> 
>> >> I'm not sure this point is correct, but I'm an atomics noob, so maybe
>> >> Gary should take a look at this :)
>> >> 
>> >
>> > Basically, what I'm trying to prove is that we can have a provenance-
>> > preserved Atomic<*mut T> implementation based on the C atomics. Either
>> > that is true, or we should write our own atomic pointer implementation.
>> 
>> That much I remembered :) But since you were going into the specifics
>> above, I think we should try to be correct. But maybe natural language
>> is the wrong medium for that, just write the rust code and we'll see...
>> 
>
> I don't thinking writing rust code can help us here other than duplicate
> my reasoning above, so like:
>
>     ipml *mut() {
>         pub fn xchg(ptr: *mut *mut (), new: *mut ()) -> *mut () {
> 	    // SAFTEY: ..
> 	    // `atomic_long_xchg()` is implemented as asm(), so it can
> 	    // be treated as a normal pointer swap() hence preserve the
> 	    // provenance.

Oh I think Gary was talking specifically about Rust's `asm!`. I don't
know if C asm is going to play the same way... (inside LLVM they
probably are the same thing, but in the abstract machine?)

> 	    unsafe { atomic_long_xchg(ptr.cast::<atomic_long_t>(), new as ffi:c_long) }
> 	}
>
>         pub fn cmpxchg(ptr: *mut *mut (), old: *mut (), new: *mut ()) -> *mut () {
> 	    // SAFTEY: ..
> 	    // `atomic_long_xchg()` is implemented as asm(), so it can
> 	    // be treated as a normal pointer compare_exchange() hence preserve the
> 	    // provenance.
> 	    unsafe { atomic_long_cmpxchg(ptr.cast::<atomic_long_t>(), old as ffi::c_long, new as ffi:c_long) }
> 	}
>
> 	<do it for a lot of functions>
>     }
>
> So I don't think that approach is worth doing. Again the provenance
> preserving is a global property, either we have it as whole or we don't
> have it, and adding precise comment of each function call won't change
> the result. I don't see much difference between reasoning about a set of
> functions vs. reasoning one function by one function with the same
> reasoning.
>
> If we have a reason to believe that C atomic doesn't support this we
> just need to move to our own implementation. I know you (and probably
> Gary) may feel the reasoning about provenance preserving a bit handwavy,

YES :)

> but this is probably the best we can get, and it's technically better

I think we can at improve the safety docs situation.

> than using Rust native atomics, because that's just UB and no one would
> help you.

I'm not arguing using those :)

> (I made a copy-pasta on purpose above, just to make another point why
> writing each function out is not worth)

Yeah that's true, but at the moment that safety comment is on the `impl`
block? I don't think that's the right place...

---
Cheers,
Benno

Powered by blists - more mailing lists