Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Wed, 25 Oct 2023 21:51:28 +0000
To:     Boqun Feng <boqun.feng@gmail.com>
From:   Benno Lossin <benno.lossin@proton.me>
Cc:     rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arch@vger.kernel.org, llvm@lists.linux.dev,
        Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Gary Guo <gary@garyguo.net>,
        =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>,
        Andreas Hindborg <a.hindborg@samsung.com>,
        Alice Ryhl <aliceryhl@google.com>,
        Alan Stern <stern@rowland.harvard.edu>,
        Andrea Parri <parri.andrea@gmail.com>,
        Will Deacon <will@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Nicholas Piggin <npiggin@gmail.com>,
        David Howells <dhowells@redhat.com>,
        Jade Alglave <j.alglave@ucl.ac.uk>,
        Luc Maranget <luc.maranget@inria.fr>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Akira Yokosawa <akiyks@gmail.com>,
        Daniel Lustig <dlustig@nvidia.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        kent.overstreet@gmail.com,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        elver@google.com, Matthew Wilcox <willy@infradead.org>,
        Dave Chinner <david@fromorbit.com>,
        linux-fsdevel@vger.kernel.org
Subject: Re: [RFC] rust: types: Add read_once and write_once
Message-ID: <VEhpQqgTM0U-aNYRUQ89ICIHW9Eehr66yw92DrmBZcZOah2mLqlz24HxEwDwYPVDOnaigDiZDEVl5mWqZJxoAtRheqTMjzpxuTKe0sr1uZs=@proton.me>
In-Reply-To: <20231025195339.1431894-1-boqun.feng@gmail.com>
References: <20231025195339.1431894-1-boqun.feng@gmail.com>
Feedback-ID: 71780778:user:proton
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

> In theory, `read_volatile` and `write_volatile` in Rust can have UB in
> case of the data races [1]. However, kernel uses volatiles to implement

I would not write "In theory", but rather state that data races involving
`read_volatile` is documented to still be UB.

> READ_ONCE() and WRITE_ONCE(), and expects races on these marked accesses

Missing "`"?

> don't cause UB. And they are proven to have a lot of usages in kernel.
>=20
> To close this gap, `read_once` and `write_once` are introduced, they
> have the same semantics as `READ_ONCE` and `WRITE_ONCE` especially
> regarding data races under the assumption that `read_volatile` and

I would separate implementation from specification. We specify
`read_once` and `write_once` to have the same semantics as `READ_ONCE`
and `WRITE_ONCE`. But we implement them using
`read_volatile`/`write_volatile`, so we might still encounter UB, but it
is still a sort of best effort. As soon as we have the actual thing in
Rust, we will switch the implementation.

> `write_volatile` have the same behavior as a volatile pointer in C from
> a compiler point of view.
>=20
> Longer term solution is to work with Rust language side for a better way
> to implement `read_once` and `write_once`. But so far, it should be good
> enough.
>=20
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://doc.rust-lang.org/std/ptr/fn.read_volatile.html#safety [1]
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>=20
> Notice I also make the primitives only work on T: Copy, since I don't
> think Rust side and C side will use a !Copy type to communicate, but we
> can always remove that constrait later.
>=20
>=20
>  rust/kernel/prelude.rs |  2 ++
>  rust/kernel/types.rs   | 43 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
>=20
> diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> index ae21600970b3..351ad182bc63 100644
> --- a/rust/kernel/prelude.rs
> +++ b/rust/kernel/prelude.rs
> @@ -38,3 +38,5 @@
>  pub use super::init::{InPlaceInit, Init, PinInit};
>=20
>  pub use super::current;
> +
> +pub use super::types::{read_once, write_once};

Do we really want people to use these so often that they should be in
the prelude?

Sure there will not really be any name conflicts, but I think an
explicit import might make sense.

> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index d849e1979ac7..b0872f751f97 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs

I don't think this should go into `types.rs`. But I do not have a good
name for the new module.

> @@ -432,3 +432,46 @@ pub enum Either<L, R> {
>      /// Constructs an instance of [`Either`] containing a value of type =
`R`.
>      Right(R),
>  }
> +
> +/// (Concurrent) Primitives to interact with C side, which are considere=
d as marked access:
> +///
> +/// tools/memory-memory/Documentation/access-marking.txt
> +

Accidental empty line? Or is this meant as a comment for both
functions?

> +/// The counter part of C `READ_ONCE()`.
> +///
> +/// The semantics is exactly the same as `READ_ONCE()`, especially when =
used for intentional data
> +/// races.

It would be great if these semantics are either linked or spelled out
here. Ideally both.

> +///
> +/// # Safety
> +///
> +/// * `src` must be valid for reads.
> +/// * `src` must be properly aligned.
> +/// * `src` must point to a properly initialized value of value `T`.
> +#[inline(always)]
> +pub unsafe fn read_once<T: Copy>(src: *const T) -> T {

Why only `T: Copy`?

> +    // SAFETY: the read is valid because of the function's safety requir=
ement, plus the assumption
> +    // here is that 1) a volatile pointer dereference in C and 2) a `rea=
d_volatile` in Rust have the
> +    // same semantics, so this function should have the same behavior as=
 `READ_ONCE()` regarding
> +    // data races.

I would explicitly state that we might have UB here due to data races.
But that we have not seen any invalid codegen and thus assume there to
be no UB (you might also want to write this in the commit message).

--
Cheers,
Benno

> +    unsafe { src.read_volatile() }
> +}
> +
> +/// The counter part of C `WRITE_ONCE()`.
> +///
> +/// The semantics is exactly the same as `WRITE_ONCE()`, especially when=
 used for intentional data
> +/// races.
> +///
> +/// # Safety
> +///
> +/// * `dst` must be valid for writes.
> +/// * `dst` must be properly aligned.
> +#[inline(always)]
> +pub unsafe fn write_once<T: Copy>(dst: *mut T, value: T) {
> +    // SAFETY: the write is valid because of the function's safety requi=
rement, plus the assumption
> +    // here is that 1) a write to a volatile pointer dereference in C an=
d 2) a `write_volatile` in
> +    // Rust have the same semantics, so this function should have the sa=
me behavior as
> +    // `WRITE_ONCE()` regarding data races.
> +    unsafe {
> +        core::ptr::write_volatile(dst, value);
> +    }
> +}
> --
> 2.41.0