| 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
| ||
|
Message-Id: <DB8HQLY48DFX.3PBBUTQLV14PC@kernel.org>
Date: Thu, 10 Jul 2025 17:46:56 +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 2/9] rust: sync: Add basic atomic operation mapping
framework
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.
> $(
> 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.
Yeah I also meant hardcoding them.
>> And then we could also put the safety comments inline:
>>
>> /// Basic atomic operations.
>> 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[acquire](ptr: *mut Self) -> Self {
>> // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
>> // cause data race per LKMM.
>> unsafe { bindings::#call(ptr.cast()) }
>> }
>>
>> /// Atomic read
>
> Copy-pasta ;-)
>
>> ///
>> /// # 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 set[release](ptr: *mut Self, v: Self) {
>> // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
>> // cause data race per LKMM.
>> unsafe { bindings::#call(ptr.cast(), v) }
>> }
>> }
>>
>> 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
copy pasting them everywhere isn't really a good solution either...
---
Cheers,
Benno
Powered by blists - more mailing lists