Message-Id: <DB9FX5XAK4JJ.3GTCC6Z5EHARV@kernel.org>
Date: Fri, 11 Jul 2025 20:34:07 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>
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>, "Gary Guo"
 <gary@...yguo.net>, 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>,
 "Alan Stern" <stern@...land.harvard.edu>
Subject: Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

On Fri Jul 11, 2025 at 3:51 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 03:34:47PM +0200, Benno Lossin wrote:
>> On Fri Jul 11, 2025 at 3:22 PM CEST, Boqun Feng wrote:
>> > On Fri, Jul 11, 2025 at 10:03:07AM +0200, Benno Lossin wrote:
>> > [...]
>> >> > +
>> >> > +    /// Returns a pointer to the underlying atomic variable.
>> >> > +    ///
>> >> > +    /// Extra safety requirement on using the return pointer: the operations done via the pointer
>> >> > +    /// cannot cause data races defined by [`LKMM`].
>> >> 
>> >> I don't think this is correct. I could create an atomic and then share
>> >> it with the C side via this function, since I have exclusive access, the
>> >> writes to this pointer don't need to be atomic.
>> >> 
>> >
>> > that's why it says "the operations done via the pointer cannot cause
>> > data races .." instead of saying "it must be atomic".
>> 
>> Ah right I misread... But then the safety requirement is redundant? Data
>> races are already UB...
>> 
>> >> We also don't document additional postconditions like this... If you
>> >
>> > Please see how Rust std document their `as_ptr()`:
>> >
>> > 	https://doc.rust-lang.org/std/sync/atomic/struct.AtomicI32.html#method.as_ptr
>> >
>> > It mentions that "Doing non-atomic reads and writes on the resulting
>> > integer can be a data race." (although the document is a bit out of
>> > date, since non-atomic read and atomic read are no longer data race now,
>> > see [1])
>> 
>> That's very different from the comment you wrote though. It's not an
>> additional safety requirement, but rather a note to users of the API
>> that they should be careful with the returned pointer.
>> 
>> > I think we can use the similar document structure here: providing more
>> > safety requirement on the returning pointers, and...
>> >
>> >> really would have to do it like this (which you shouldn't given the
>> >> example above), you would have to make this function `unsafe`, otherwise
>> >> there is no way to ensure that people adhere to it (since it isn't part
>> >> of the safety docs).
>> >> 
>> >
>> > ...since dereferencing pointers is always `unsafe`, users need to avoid
>> > data races anyway, hence this is just additional information that helps
>> > reasoning.
>> 
>> I disagree.
>> 
>> As mentioned above, data races are already forbidden for raw pointers.
>> We should indeed add a note that says that non-atomic operations might
>> result in data races. But that's very different from adding an
>> additional safety requirement for using the pointer.
>> 
>> And I don't think that we can add additional safety requirements to
>> dereferencing a raw pointer without an additional `unsafe` block.
>> 
>
> So all your disagreement is about the "extra safety requirement" part?
> How about I drop that:
>
>     /// Returns a pointer to the underlying atomic `T`.
>     pub const fn as_ptr(&self) -> *mut T {
>         self.0.get()
>     }

Yes that's what I had in mind.

> ? I tried to add something additional information:
>
> /// Note that non-atomic reads and writes via the returned pointer may
> /// cause data races if racing with atomic reads and writes per [LKMM].
>
> but that seems redundant, because as you said, data races are UB anyway.

Yeah... I don't think the stdlib docs are too useful on this function:

    Doing non-atomic reads and writes on the resulting integer can be a data
    race. This method is mostly useful for FFI, where the function signature
    may use *mut i32 instead of &AtomicI32.
    
    Returning an *mut pointer from a shared reference to this atomic is safe
    because the atomic types work with interior mutability. All
    modifications of an atomic change the value through a shared reference,
    and can do so safely as long as they use atomic operations. Any use of
    the returned raw pointer requires an unsafe block and still has to
    uphold the same restriction: operations on it must be atomic.

You can mention the use of this function for FFI. People might then be
discouraged from using it for other things where it doesn't make sense.

---
Cheers,
Benno

Powered by blists - more mailing lists