| 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: <87ms9mvgu4.fsf@kernel.org>
Date: Wed, 02 Jul 2025 15:54:11 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Alice Ryhl" <aliceryhl@...gle.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
<alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
<gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>,
"Masahiro Yamada" <masahiroy@...nel.org>, "Nathan Chancellor"
<nathan@...nel.org>, "Luis Chamberlain" <mcgrof@...nel.org>, "Danilo
Krummrich" <dakr@...nel.org>, "Benno Lossin" <lossin@...nel.org>,
"Nicolas Schier" <nicolas.schier@...ux.dev>, "Trevor Gross"
<tmgross@...ch.edu>, "Adam Bratschi-Kaye" <ark.email@...il.com>,
<rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-kbuild@...r.kernel.org>, "Petr Pavlu" <petr.pavlu@...e.com>,
"Sami Tolvanen" <samitolvanen@...gle.com>, "Daniel Gomez"
<da.gomez@...sung.com>, "Simona Vetter" <simona.vetter@...ll.ch>, "Greg
KH" <gregkh@...uxfoundation.org>, "Fiona Behrens" <me@...enk.dev>,
"Daniel Almeida" <daniel.almeida@...labora.com>,
<linux-modules@...r.kernel.org>
Subject: Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
"Alice Ryhl" <aliceryhl@...gle.com> writes:
> On Wed, Jul 2, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>>
>> Introduce the `OnceLock` type, a container that can only be written once.
>> The container uses an internal atomic to synchronize writes to the internal
>> value.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>
> This type provides no way to wait for initialization to finish if it's
> ongoing. Do you not need that?
I don't, and in my use case it would cause a deadlock to wait. Anyway,
it might be useful to others. Would you add it now, or wait for a user?
>
>> ---
>> rust/kernel/sync.rs | 1 +
>> rust/kernel/sync/once_lock.rs | 104 ++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 105 insertions(+)
>>
>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>> index c7c0e552bafe..f2ee07315091 100644
>> --- a/rust/kernel/sync.rs
>> +++ b/rust/kernel/sync.rs
>> @@ -15,6 +15,7 @@
>> mod condvar;
>> pub mod lock;
>> mod locked_by;
>> +pub mod once_lock;
>
> I would add a re-export so that users can import this as kernel::sync::OnceLock.
OK.
>
>> pub mod poll;
>> pub mod rcu;
>>
>> diff --git a/rust/kernel/sync/once_lock.rs b/rust/kernel/sync/once_lock.rs
>> new file mode 100644
>> index 000000000000..cd311bea3919
>> --- /dev/null
>> +++ b/rust/kernel/sync/once_lock.rs
>> @@ -0,0 +1,104 @@
>> +//! A container that can be initialized at most once.
>> +
>> +use super::atomic::ordering::Acquire;
>> +use super::atomic::ordering::Release;
>> +use super::atomic::Atomic;
>> +use kernel::types::Opaque;
>> +
>> +/// A container that can be populated at most once. Thread safe.
>> +///
>> +/// Once the a [`OnceLock`] is populated, it remains populated by the same object for the
>> +/// lifetime `Self`.
>> +///
>> +/// # Invariants
>> +///
>> +/// `init` tracks the state of the container:
>> +///
>> +/// - If the container is empty, `init` is `0`.
>> +/// - If the container is mutably accessed, `init` is `1`.
>
> I would phrase this as "being initialized" instead of "mutably
> accessed". I initially thought this was talking about someone calling
> a &mut self method.
Makes sense, I will change that.
>
>> +/// - If the container is populated and ready for shared access, `init` is `2`.
>> +///
>> +/// # Example
>> +///
>> +/// ```
>> +/// # use kernel::sync::once_lock::OnceLock;
>> +/// let value = OnceLock::new();
>> +/// assert_eq!(None, value.as_ref());
>> +///
>> +/// let status = value.populate(42u8);
>> +/// assert_eq!(true, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +///
>> +/// let status = value.populate(101u8);
>> +/// assert_eq!(false, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +/// ```
>> +pub struct OnceLock<T> {
>> + init: Atomic<u32>,
>> + value: Opaque<T>,
>
> Opaque does not destroy the inner value. You are missing a destructor.
Oops.
>
>> +}
>> +
>> +impl<T> Default for OnceLock<T> {
>> + fn default() -> Self {
>> + Self::new()
>> + }
>> +}
>> +
>> +impl<T> OnceLock<T> {
>> + /// Create a new [`OnceLock`].
>> + ///
>> + /// The returned instance will be empty.
>> + pub const fn new() -> Self {
>> + // INVARIANT: The container is empty and we set `init` to `0`.
>> + Self {
>> + value: Opaque::uninit(),
>> + init: Atomic::new(0),
>> + }
>> + }
>> +
>> + /// Get a reference to the contained object.
>> + ///
>> + /// Returns [`None`] if this [`OnceLock`] is empty.
>> + pub fn as_ref(&self) -> Option<&T> {
>> + if self.init.load(Acquire) == 2 {
>> + // SAFETY: As determined by the load above, the object is ready for shared access.
>> + Some(unsafe { &*self.value.get() })
>> + } else {
>> + None
>> + }
>> + }
>> +
>> + /// Populate the [`OnceLock`].
>> + ///
>> + /// Returns `true` if the [`OnceLock`] was successfully populated.
>> + pub fn populate(&self, value: T) -> bool {
>> + // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
>> + // `init`.
>> + if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
>
> This acquire can be Relaxed. All other accesses to self.value
> synchronize with the release store below, so you do not need acquire
> here to obtain exclusive access.
Right, thanks.
Best regards,
Andreas Hindborg
Powered by blists - more mailing lists