linux-kernel - Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aG_nT3H8J-h2qwr5@Mac.home>
Date: Thu, 10 Jul 2025 09:16:15 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <lossin@...nel.org>
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 2/9] rust: sync: Add basic atomic operation mapping
 framework

On Thu, Jul 10, 2025 at 05:46:56PM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 5:12 PM CEST, Boqun Feng wrote:
> > On Thu, Jul 10, 2025 at 01:04:38PM +0200, Benno Lossin wrote:
> >> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> >> > +declare_and_impl_atomic_methods!(
> >> > +    AtomicHasBasicOps ("Basic atomic operations") {
> >> > +        read[acquire](ptr: *mut Self) -> Self {
> >> > +            call(ptr.cast())
> >> > +        }
> >> > +
> >> > +        set[release](ptr: *mut Self, v: Self) {
> >> > +            call(ptr.cast(), v)
> >> > +        }
> >> > +    }
> >> 
> >> I think this would look a bit better:
> >> 
> >>     /// Basic atomic operations.
> >>     pub trait AtomicHasBasicOps {
> >>         unsafe fn read[acquire](ptr: *mut Self) -> Self {
> >>             bindings::#call(ptr.cast())
> >>         }
> >> 
> >>         unsafe fn set[release](ptr: *mut Self, v: Self) {
> >>             bindings::#call(ptr.cast(), v)
> >>         }
> >>     }
> >> 
> >
> > Make sense, I've made `pub trait`, `bindings::#` and `unsafe fn`
> > hard-coded:
> >
> > macro_rules! declare_and_impl_atomic_methods {
> >     (#[doc = $doc:expr] pub trait $ops:ident {
> 
> You should allow any kind of attribute (and multiple), that makes it
> much simpler.
> 

I didn't know I could do that, updated:

    ($(#[$attr:meta])* pub trait $ops:ident {
        $(
            unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
                bindings::#call($($arg:tt)*)
            }
        )*
    }) => {
        $(#[$attr])*

Thanks!

> >         $(
> >             unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
> >                 bindings::#call($($arg:tt)*)
> >             }
> >         )*
> >     }) => {
> >
> > It shouldn't be very hard to make use of the actual visibility or
> > unsafe, but we currently don't have other visibility or safe function,
> > so it's simple to keep it as it is.
[..]
> >> I'm not sure if this is worth it, but for reading the definitions of
> >> these operations directly in the code this is going to be a lot more
> >> readable. I don't think it's too bad to duplicate it.
> >> 
> >> I'm also not fully satisfied with the safety comment on
> >> `bindings::#call`...
> >> 
> >
> > Based on the above, I'm not going to do the change (i.e. duplicating
> > the safe comments and improve them), and I would make an issue tracking
> > it, and we can revisit it when we have time. Sounds good?
> 
> Sure, I feel like some kind of method duplication macro might be much
> better here, like:
> 
>     multi_functions! {
>         pub trait AtomicHasBasicOps {
>             /// Atomic read
>             ///
>             /// # Safety
>             /// - Any pointer passed to the function has to be a valid pointer
>             /// - Accesses must not cause data races per LKMM:
>             ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
>             ///   - Atomic write racing with normal read or normal write is a data race, unless the
>             ///     normal access is done from the C side and considered immune to data races, e.g.
>             ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
>             unsafe fn [<read, read_acquire>](ptr: *mut Self) -> Self;
> 
>             // ...
>         }
>     }
> 
> And then also allow it on impls. I don't really like the idea of
> duplicating and thus hiding the safety docs... But I also see that just

At least the rustdoc has safety section for each function. ;-)

> copy pasting them everywhere isn't really a good solution either...
> 

Yeah, perhaps there is no immediate resolution, but open to any
suggestion.

Regards,
Boqun

> ---
> Cheers,
> Benno