linux-kernel - Re: [PATCH v7 6/9] rust: sync: atomic: Add the framework of arithmetic operations

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: <DBCQUAA42DHH.23BNUVOKS38UI@kernel.org>
Date: Tue, 15 Jul 2025 17:45:34 +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 v7 6/9] rust: sync: atomic: Add the framework of
 arithmetic operations

On Tue Jul 15, 2025 at 3:33 PM CEST, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 01:21:20PM +0200, Benno Lossin wrote:
>> On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
>> > +/// Types that support atomic add operations.
>> > +///
>> > +/// # Safety
>> > +///
>> > +/// Wrapping adding any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
>> 
>> I don't like "wrapping adding", either we use "`wrapping_add`" or we use
>> some other phrasing.
>> 
>
> Let's use `wrapping_add` then.
>
>     /// `wrapping_add` any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
>     /// any value of type `Self::Repr` obtained through transmuting a value of type `Self` to must
>     /// yield a value with a bit pattern also valid for `Self`.
>
>> > +pub unsafe trait AllowAtomicAdd<Rhs = Self>: AllowAtomic {
>> 
>> Why `Allow*`? I think `AtomicAdd` is better?
>> 
>
> To be consistent with `AllowAtomic` (the super trait), if we use
> `AtomicAdd` here, should we change `AllowAtomic` to `AtomicBase`?

Ideally, we would name that trait just `Atomic` :) But it then
conflicts with the `Atomic<T>` struct (this would be motivation to put
them in different modules :). I like `AtomicBase` better than
`AllowAtomic`, but maybe there is a better name, how about `AtomicType`?

>> > +    /// Converts `Rhs` into the `Delta` type of the atomic implementation.
>> > +    fn rhs_into_delta(rhs: Rhs) -> <Self::Repr as AtomicImpl>::Delta;
>> > +}
>> > +
>> >  impl<T: AllowAtomic> Atomic<T> {
>> >      /// Creates a new atomic `T`.
>> >      pub const fn new(v: T) -> Self {
>> > @@ -462,3 +474,100 @@ fn try_cmpxchg<Ordering: ordering::Any>(&self, old: &mut T, new: T, _: Ordering)
>> >          ret
>> >      }
>> >  }
>> > +
>> > +impl<T: AllowAtomic> Atomic<T>
>> > +where
>> > +    T::Repr: AtomicHasArithmeticOps,
>> > +{
>> > +    /// Atomic add.
>> > +    ///
>> > +    /// Atomically updates `*self` to `(*self).wrapping_add(v)`.
>> > +    ///
>> > +    /// # Examples
>> > +    ///
>> > +    /// ```
>> > +    /// use kernel::sync::atomic::{Atomic, Relaxed};
>> > +    ///
>> > +    /// let x = Atomic::new(42);
>> > +    ///
>> > +    /// assert_eq!(42, x.load(Relaxed));
>> > +    ///
>> > +    /// x.add(12, Relaxed);
>> > +    ///
>> > +    /// assert_eq!(54, x.load(Relaxed));
>> > +    /// ```
>> > +    #[inline(always)]
>> > +    pub fn add<Rhs, Ordering: ordering::RelaxedOnly>(&self, v: Rhs, _: Ordering)
>> > +    where
>> > +        T: AllowAtomicAdd<Rhs>,
>> > +    {
>> > +        let v = T::rhs_into_delta(v);
>> > +        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is a valid
>> > +        // pointer of `T::Repr` for reads and valid for writes of values transmutable to `T`.
>> > +        let a = self.as_ptr().cast::<T::Repr>();
>> > +
>> > +        // `*self` remains valid after `atomic_add()` because of the safety requirement of
>> > +        // `AllowAtomicAdd`.
>> 
>> This part should be moved to the CAST comment above, since we're not
>> only writing a value transmuted from `T` into `*self`.
>> 
>
> Hmm.. the CAST comment should explain why a pointer of `T` can be a
> valid pointer of `T::Repr` because the atomic_add() below is going to
> read through the pointer and write value back. The comment starting with
> "`*self`" explains the value written is a valid `T`, therefore
> conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
> into `a`.

I see, my interpretation was that if we put it on the cast, then the
operation that `atomic_add` does also is valid.

But I think this comment should either be part of the `CAST` or the
`SAFETY` comment. Going by your interpretation, it would make more sense
in the SAFETY one, since there you justify that you're actually writing
a value of type `T`.

---
Cheers,
Benno

> Basically
>
> // CAST
> let a = ..
>
> ^ explains what `a` is a valid for and why it's valid.
>
> // `*self` remains
>
> ^ explains that we write a valid value to `a`.
>
>
> So I don't think we need to move?