linux-kernel - Re: [RFC v2 04/13] rust: sync: atomic: Add generic atomics

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: <Z1yVUmGBIxKH1xR-@tardis.local>
Date: Fri, 13 Dec 2024 12:13:06 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: rust-for-linux@...r.kernel.org, rcu@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	llvm@...ts.linux.dev, lkmm@...ts.linux.dev,
	Miguel Ojeda <ojeda@...nel.org>,	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	Alan Stern <stern@...land.harvard.edu>,
	Andrea Parri <parri.andrea@...il.com>,	Will Deacon <will@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Nicholas Piggin <npiggin@...il.com>,	David Howells <dhowells@...hat.com>,
	Jade Alglave <j.alglave@....ac.uk>,	Luc Maranget <luc.maranget@...ia.fr>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Akira Yokosawa <akiyks@...il.com>,	Daniel Lustig <dlustig@...dia.com>,
	Joel Fernandes <joel@...lfernandes.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,	kent.overstreet@...il.com,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, elver@...gle.com,
	Mark Rutland <mark.rutland@....com>,
	Thomas Gleixner <tglx@...utronix.de>,	Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>,
	Catalin Marinas <catalin.marinas@....com>,	torvalds@...ux-foundation.org,
 linux-arm-kernel@...ts.infradead.org,	linux-fsdevel@...r.kernel.org,
 Trevor Gross <tmgross@...ch.edu>,	dakr@...hat.com,
 Frederic Weisbecker <frederic@...nel.org>,
	Neeraj Upadhyay <neeraj.upadhyay@...nel.org>,
	Josh Triplett <josh@...htriplett.org>,
	Uladzislau Rezki <urezki@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Zqiang <qiang.zhang1211@...il.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,	Albert Ou <aou@...s.berkeley.edu>,
 linux-riscv@...ts.infradead.org
Subject: Re: [RFC v2 04/13] rust: sync: atomic: Add generic atomics

On Fri, Dec 13, 2024 at 03:32:47PM +0100, Alice Ryhl wrote:
> On Thu, Dec 12, 2024 at 6:34 PM Boqun Feng <boqun.feng@...il.com> wrote:
> >
> > On Thu, Dec 12, 2024 at 11:57:07AM +0100, Alice Ryhl wrote:
> > [...]
> > > > diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
> > > > new file mode 100644
> > > > index 000000000000..204da38e2691
> > > > --- /dev/null
> > > > +++ b/rust/kernel/sync/atomic/generic.rs
> > > > @@ -0,0 +1,253 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +//! Generic atomic primitives.
> > > > +
> > > > +use super::ops::*;
> > > > +use super::ordering::*;
> > > > +use crate::types::Opaque;
> > > > +
> > > > +/// A generic atomic variable.
> > > > +///
> > > > +/// `T` must impl [`AllowAtomic`], that is, an [`AtomicImpl`] has to be chosen.
> > > > +///
> > > > +/// # Invariants
> > > > +///
> > > > +/// Doing an atomic operation while holding a reference of [`Self`] won't cause a data race, this
> > > > +/// is guaranteed by the safety requirement of [`Self::from_ptr`] and the extra safety requirement
> > > > +/// of the usage on pointers returned by [`Self::as_ptr`].
> > > > +#[repr(transparent)]
> > > > +pub struct Atomic<T: AllowAtomic>(Opaque<T>);
> > > > +
> > > > +// SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
> > > > +unsafe impl<T: AllowAtomic> Sync for Atomic<T> {}
> > >
> > > Surely it should also be Send?
> > >
> >
> > It's `Send` here because `Opaque<T>` is `Send` when `T` is `Send`. And
> > in patch #9, I changed the definition of `AllowAtomic`, which is not a
> > subtrait of `Send` anymore, and an `impl Send` block was added there.
> >
> > > > +/// Atomics that support basic atomic operations.
> > > > +///
> > > > +/// TODO: Unless the `impl` is a `#[repr(transparet)]` new type of an existing [`AllowAtomic`], the
> > > > +/// impl block should be only done in atomic mod. And currently only basic integer types can
> > > > +/// implement this trait in atomic mod.
> > >
> > > What's up with this TODO? Can't you just write an appropriate safety
> > > requirement?
> > >
> >
> > Because the limited scope of types that allows atomic is an artificial
> > choice, i.e. we want to start with a limited number of types and make
> > forward progress, and the types that we don't want to support atomics
> > for now are not because of safety reasons, but more of a lack of
> > users/motivations. So I don't think this is something we should use
> > safety requirement to describe.
> 
> I found the wording very confusing. Could you reword it to say
> something about future possibilities?
> 

Sure, how about:

/// TODO: Currently the [`AllowAtomic`] types are restricted within
/// basic integer types (and their transparent new types). In the
/// future, we could extend the scope to more data types when there is a
/// clear and meaningful usage, but for now, [`AllowAtomic`] should only
/// be implemented inside atomic mod for the restricted types mentioned
/// above.

?

> > > > +/// # Safety
> > > > +///
> > > > +/// [`Self`] must have the same size and alignment as [`Self::Repr`].
> > > > +pub unsafe trait AllowAtomic: Sized + Send + Copy {
> > > > +    /// The backing atomic implementation type.
> > > > +    type Repr: AtomicImpl;
> > > > +
> > > > +    /// Converts into a [`Self::Repr`].
> > > > +    fn into_repr(self) -> Self::Repr;
> > > > +
> > > > +    /// Converts from a [`Self::Repr`].
> > > > +    fn from_repr(repr: Self::Repr) -> Self;
> > >
> > > What do you need these methods for?
> > >
> >
> > Converting a `AtomicImpl` value (currently only `i32` and `i64`) to a
> > `AllowAtomic` value without using transmute in `impl` block of
> > `Atomic<T>`. Any better idea?
> 
> You could use transmute?
> 

In a draft version, I did use transmute, but Benno commented that he
wanted to avoid arbitrary transmute as hard as possible (if I didn't
misunderstand him). Hence these two functions are provided. Now think
about it, I don't think doing either way (transmute or *_repr()
function) would affect most of users, since most of users won't need to 
impl `AllowAtomic` themselves, therefore I think keeping it as it is is
fine. Do you have any user observable concern of defining these
functions?

Regards,
Boqun

> Alice