[PATCH 0/3] rust: sync: add SetOnce::try_get_or_populate()

[PATCH 0/3] rust: sync: add SetOnce::try_get_or_populate()

[Alice Ryhl]

The SetOnce::populate() method does not internally synchronize callers
that fail to populate the value with the successful call. This means
that naive loops using as_ref() and populate() can lead to spinning on
the initialization, which is best avoided. Thus, provide a helper that
avoids this issue using a user-provided lock.

One potential alternative is to change populate() so that the failing
caller actually does synchronize with the successful call to populate().
However, this is somewhat tricky:

* There are users of SetOnce that construct it in const context, and we
  currently don't have the ability to do that for most locks, so we
  cannot easily add a lock to SetOnce.
* Just spinning on the atomic is undesirable unless we disable
  preemption in the success path. If we do disable preemption, then that
  raises complications for handling the PREEMPT_RT case.
* It also raises questions about deadlocks if populate() is called from
  irqs.

By using a user-provided lock, we do not have to worry about these
issues inside SetOnce.

This series is based on char-misc-next.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Alice Ryhl (3):
      rust: sync: return `Result<&T, T>` from `SetOnce::populate()`
      rust: sync: add SetOnce::try_get_or_populate()
      rust_binder: use SetOnce::try_get_or_populate()

 drivers/android/binder/process.rs | 16 +++--------
 rust/kernel/module_param.rs       |  8 +++---
 rust/kernel/sync/set_once.rs      | 58 ++++++++++++++++++++++++++++++++-------
 3 files changed, 56 insertions(+), 26 deletions(-)
---
base-commit: 2cedf2272f1bb42471e646868ac572cc5752bd91
change-id: 20260721-setonce-populate-f2a9b61aeb1d

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

[PATCH 1/3] rust: sync: return `Result<&T, T>` from `SetOnce::populate()`

[Alice Ryhl]

When `populate()` succeeds, there's no way infallible way to get the
value that was just inserted. By returning &T in this case, such
infallible access methods become possible.

Additionally, when `populate()` fails, the provided value is dropped.
This has two disadvantages:

1. If the caller holds a lock, the value is dropped under said lock.
2. If the caller wishes to use the same value for something else, they
   can't, because it's lost.

Changing the return value to Result<&T, T> handles all of these cases.

Rust Binder is updated to avoid a warning about an unused Result.
Additionally, ModuleParam is updated to correctly translate the new
return value to the right target values.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/process.rs |  5 +++--
 rust/kernel/module_param.rs       |  8 ++++----
 rust/kernel/sync/set_once.rs      | 15 +++++++++------
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index 1778628d8acd..d486bf7c0b8a 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -1801,8 +1801,9 @@ pub(crate) fn poll(
 
                 let poll = PollCondVarBox::new(c"Process::poll", kernel::static_lock_class!())?;
                 // Reuse our existing lock to synchronize callers initializing.
-                let _guard = this.node_refs.lock();
-                this.poll.populate(poll);
+                let guard = this.node_refs.lock();
+                let _ret = this.poll.populate(poll);
+                drop(guard);
             };
 
             table.register_wait(file, poll);
diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
index 6541af218390..8f0bd085badf 100644
--- a/rust/kernel/module_param.rs
+++ b/rust/kernel/module_param.rs
@@ -77,10 +77,10 @@ pub trait ModuleParam: Sized + Copy {
         // SAFETY: By function safety requirements, this access is safe.
         let container = unsafe { &*((*param).__bindgen_anon_1.arg.cast::<SetOnce<T>>()) };
 
-        container
-            .populate(new_value)
-            .then_some(0)
-            .ok_or(kernel::error::code::EEXIST)
+        match container.populate(new_value) {
+            Ok(_) => Ok(0),
+            Err(_) => Err(EEXIST),
+        }
     })
 }
 
diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
index 139cef05e935..a78f8c8e87db 100644
--- a/rust/kernel/sync/set_once.rs
+++ b/rust/kernel/sync/set_once.rs
@@ -31,12 +31,12 @@
 /// assert_eq!(None, value.as_ref());
 ///
 /// let status = value.populate(42u8);
-/// assert_eq!(true, status);
+/// assert_eq!(Ok(&42u8), 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!(Err(101u8), status);
 /// assert_eq!(Some(&42u8), value.as_ref());
 /// assert_eq!(Some(42u8), value.copy());
 /// ```
@@ -78,8 +78,9 @@ pub fn as_ref(&self) -> Option<&T> {
 
     /// Populate the [`SetOnce`].
     ///
-    /// Returns `true` if the [`SetOnce`] was successfully populated.
-    pub fn populate(&self, value: T) -> bool {
+    /// Returns `Ok(value)` if the [`SetOnce`] was successfully populated with the provided value.
+    /// Otherwise returns an error containing the value that this call attempted to insert.
+    pub fn populate(&self, value: T) -> Result<&T, T> {
         // INVARIANT: If the swap succeeds:
         //  - We increase `init`.
         //  - We write the valid value `1` to `init`.
@@ -95,9 +96,11 @@ pub fn populate(&self, value: T) -> bool {
             //  - We release our exclusive access to `self.value` and it is now valid for shared
             //    access.
             self.init.store(2, Release);
-            true
+            // SAFETY: By the type invariants of `Self`, the value is initialized and will stay
+            // that way.
+            Ok(unsafe { &*self.value.get().cast() })
         } else {
-            false
+            Err(value)
         }
     }
 

-- 
2.55.0.229.g6434b31f56-goog

Re: [PATCH 1/3] rust: sync: return `Result<&T, T>` from `SetOnce::populate()`

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] `ModuleParamAccess` lacks `#[repr(C)]`, leading to potential Undefined Behavior and memory corruption when its pointer is cast to `SetOnce<T>`.
--

--- Patch [1]: [PATCH 1/3] rust: sync: return `Result<&T, T>` from `SetOnce::populate()` ---
commit e0d10f7a2c42bcda24ee9851cc2fda3d4c4bc480
Author: Alice Ryhl <aliceryhl@google.com>

rust: sync: return Result<&T, T> from SetOnce::populate()

This commit changes the return value of SetOnce::populate() from a boolean
to a Result to allow infallible access to the inserted value on success.
It also ensures the inserted value is not dropped under a lock if populate()
fails, and updates ModuleParam and Binder to handle the new return type.

> diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
> index 6541af218390c..8f0bd085badfe 100644
> --- a/rust/kernel/module_param.rs
> +++ b/rust/kernel/module_param.rs
> @@ -77,10 +77,10 @@ pub trait ModuleParam: Sized + Copy {
>          // SAFETY: By function safety requirements, this access is safe.
>          let container = unsafe { &*((*param).__bindgen_anon_1.arg.cast::<SetOnce<T>>()) };

[Severity: High]
This is a pre-existing issue, but since we are modifying this code block, is
it safe to cast this void pointer directly to SetOnce<T>?

Looking at the definition of ModuleParamAccess<T>, it lacks a #[repr(C)]
annotation:

rust/kernel/module_param.rs:ModuleParamAccess {
    value: SetOnce<T>,
    default: T,
}

Since it uses the default Rust layout, the compiler is free to reorder these
fields. When as_void_ptr() passes a pointer to the entire struct into the
C kernel parameters API:

rust/kernel/module_param.rs:ModuleParamAccess::as_void_ptr() {
    core::ptr::from_ref(self).cast_mut().cast()
}

And set_param casts it back directly to SetOnce<T> (assuming it sits at
offset 0), could this lead to memory corruption if the compiler decides to
place default before value?

> -        container
> -            .populate(new_value)
> -            .then_some(0)
> -            .ok_or(kernel::error::code::EEXIST)
> +        match container.populate(new_value) {
> +            Ok(_) => Ok(0),
> +            Err(_) => Err(EEXIST),
> +        }
>      })
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-setonce-populate-v1-0-fa7455c26c42@google.com?part=1

[PATCH 2/3] rust: sync: add SetOnce::try_get_or_populate()

[Alice Ryhl]

The SetOnce::populate() method does not internally synchronize callers
that fail to populate the value with the successful call. This means
that naive loops using as_ref() and populate() can lead to spinning on
the initialization, which is best avoided. Thus, provide a helper that
avoids this issue using a user-provided lock.

One potential alternative is to change populate() so that the failing
caller actually does synchronize with the successful call to populate().
However, this is somewhat tricky:

* There are users of SetOnce that construct it in const context, and we
  currently don't have the ability to do that for most locks, so we
  cannot easily add a lock to SetOnce.
* Just spinning on the atomic is undesirable unless we disable
  preemption in the success path. If we do disable preemption, then that
  raises complications for handling the PREEMPT_RT case.
* It also raises questions about deadlocks if populate() is called from
  irqs.

By using a user-provided lock, we do not have to worry about these
issues inside SetOnce.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync/set_once.rs | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
index a78f8c8e87db..d9cc598a8d78 100644
--- a/rust/kernel/sync/set_once.rs
+++ b/rust/kernel/sync/set_once.rs
@@ -2,11 +2,18 @@
 
 //! A container that can be initialized at most once.
 
-use super::atomic::{
-    ordering::{Acquire, Relaxed, Release},
-    Atomic,
-};
 use core::{cell::UnsafeCell, mem::MaybeUninit};
+use kernel::sync::{
+    atomic::{
+        ordering::{
+            Acquire,
+            Relaxed,
+            Release, //
+        },
+        Atomic, //
+    },
+    lock, //
+};
 
 /// A container that can be populated at most once. Thread safe.
 ///
@@ -104,6 +111,34 @@ pub fn populate(&self, value: T) -> Result<&T, T> {
         }
     }
 
+    /// Get the value, or populate it if it's missing.
+    ///
+    /// This method is useful to avoid spinning on the internal atomic state. If all writers call
+    /// this method with the same lock, then they are synchronized with each other and it's
+    /// guaranteed that no caller will attempt to invoke [`SetOnce::populate`] more than once.
+    pub fn try_get_or_populate<F, E, U, B>(&self, lock: &lock::Lock<U, B>, f: F) -> Result<&T, E>
+    where
+        B: lock::Backend,
+        F: FnOnce() -> Result<T, E>,
+    {
+        if let Some(value) = self.as_ref() {
+            return Ok(value);
+        }
+
+        let mut to_insert = f()?;
+        loop {
+            if let Some(value) = self.as_ref() {
+                return Ok(value);
+            }
+
+            let _guard = lock.lock();
+            match self.populate(to_insert) {
+                Ok(value) => return Ok(value),
+                Err(ret) => to_insert = ret,
+            }
+        }
+    }
+
     /// Get a copy of the contained object.
     ///
     /// Returns [`None`] if the [`SetOnce`] is empty.

-- 
2.55.0.229.g6434b31f56-goog

Re: [PATCH 2/3] rust: sync: add SetOnce::try_get_or_populate()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Optimistic, unlocked execution of the initialization closure `f()` leads to duplicate side-effects and spurious TOCTOU initialization failures.
--

--- Patch [2]: [PATCH 2/3] rust: sync: add SetOnce::try_get_or_populate() ---
commit fa04209af44f9b7b6642f550eaec6a8a1f95a58a
Author: Alice Ryhl <aliceryhl@google.com>

rust: sync: add SetOnce::try_get_or_populate()

This commit introduces a try_get_or_populate() helper to SetOnce. The new
method allows callers to use a user-provided lock to avoid spinning on the
internal atomic state during concurrent initialization.

> diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
> index a78f8c8e87dbf..d9cc598a8d78f 100644
> --- a/rust/kernel/sync/set_once.rs
> +++ b/rust/kernel/sync/set_once.rs

[ ... ]

> @@ -104,6 +111,34 @@ pub fn populate(&self, value: T) -> Result<&T, T> {
>          }
>      }
>  
> +    /// Get the value, or populate it if it's missing.
> +    ///
> +    /// This method is useful to avoid spinning on the internal atomic state. If all writers call
> +    /// this method with the same lock, then they are synchronized with each other and it's
> +    /// guaranteed that no caller will attempt to invoke [`SetOnce::populate`] more than once.
> +    pub fn try_get_or_populate<F, E, U, B>(&self, lock: &lock::Lock<U, B>, f: F) -> Result<&T, E>
> +    where
> +        B: lock::Backend,
> +        F: FnOnce() -> Result<T, E>,
> +    {
> +        if let Some(value) = self.as_ref() {
> +            return Ok(value);
> +        }
> +
> +        let mut to_insert = f()?;
         ^^^^

[Severity: High]
Could this introduce a regression where evaluating f() outside the lock
risks duplicate side-effects?

If multiple threads call try_get_or_populate() concurrently on an
uninitialized SetOnce, they could both bypass the initial as_ref() check
and execute f() simultaneously. If f() performs operations like allocating
hardware resources or registering IDs, wouldn't those occur multiple times
before reaching the lock?

Additionally, if thread A successfully initializes the value inside the lock
but thread B subsequently fails during its concurrent evaluation of f(), the
? operator will immediately return the error. Does this cause thread B to
erroneously fail and propagate the error, even though the SetOnce is now
fully populated by thread A?

> +        loop {
> +            if let Some(value) = self.as_ref() {
> +                return Ok(value);
> +            }
> +
> +            let _guard = lock.lock();
> +            match self.populate(to_insert) {
> +                Ok(value) => return Ok(value),
> +                Err(ret) => to_insert = ret,
> +            }
> +        }
> +    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-setonce-populate-v1-0-fa7455c26c42@google.com?part=2

[PATCH 3/3] rust_binder: use SetOnce::try_get_or_populate()

[Alice Ryhl]

Since this method has just been added, use it instead of open coding the
loop. This also has the side effect of dropping the PollCondVarBox
outside of the node_refs lock when two threads initialize it in
parallel.

Suggested-by: Boqun Feng <boqun@kernel.org>
Link: https://lore.kernel.org/all/alJHCkMIcnXYPNoJ@tardis.local/
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/process.rs | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index d486bf7c0b8a..5f8779badd3d 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -1793,21 +1793,12 @@ pub(crate) fn poll(
         table: PollTable<'_>,
     ) -> Result<u32> {
         let thread = this.get_current_thread()?;
-        {
-            let poll = loop {
-                if let Some(poll) = this.poll.as_ref() {
-                    break poll;
-                }
 
-                let poll = PollCondVarBox::new(c"Process::poll", kernel::static_lock_class!())?;
-                // Reuse our existing lock to synchronize callers initializing.
-                let guard = this.node_refs.lock();
-                let _ret = this.poll.populate(poll);
-                drop(guard);
-            };
+        let poll = this.poll.try_get_or_populate(&this.node_refs, || {
+            PollCondVarBox::new(c"Process::poll", kernel::static_lock_class!())
+        })?;
+        table.register_wait(file, poll);
 
-            table.register_wait(file, poll);
-        }
         let (from_proc, mut mask) = thread.poll()?;
         if mask == 0 && from_proc && !this.inner.lock().work.is_empty() {
             mask |= bindings::POLLIN;

-- 
2.55.0.229.g6434b31f56-goog