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

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

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

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

[Alexandre Courbot]

On Wed Jul 22, 2026 at 6:16 PM JST, Alice Ryhl wrote:
> 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>

From the API perspective, this still leaves the option of calling the
method concurrently with different locks. What happens in this case?

> +    where
> +        B: lock::Backend,
> +        F: FnOnce() -> Result<T, E>,
> +    {
> +        if let Some(value) = self.as_ref() {
> +            return Ok(value);
> +        }
> +
> +        let mut to_insert = f()?;

This means that `f` can run more than once for a given `SetOnce`, which
can lead to problems depending on `f`'s' side-effects.

In the GEM shmem case, we would create a second `SGTableMap`, and since
`SGTableMap` assumes it is the sole owner, the last instance to drop
would create a use-after-free.

Now this sounds more like a problem with `SGTableMap`, but if we cannot
avoid calling `f` at least twice then I think it would help if this was
documented.

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

[Alexandre Courbot]

On Wed Jul 22, 2026 at 6:16 PM JST, Alice Ryhl wrote:
> 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.

Does the GEM shmem module also need to be updated for the same reason?

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

[Alexandre Courbot]

On Wed Jul 22, 2026 at 6:16 PM JST, Alice Ryhl wrote:
> 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>

If would be nice if the series could also include a patch updating the
GEM shmem module, which is another potential user of
`try_get_or_populate` - in particular since the current implementation
doesn't seem to be a perfect match yet (see my comments on patch 2) so
it would be an opportunity to refine it.

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

[Boqun Feng]

On Wed, Jul 29, 2026 at 11:05:55PM +0900, Alexandre Courbot wrote:
[...]
> > +    /// 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>
> 
> From the API perspective, this still leaves the option of calling the
> method concurrently with different locks. What happens in this case?
> 

Calling this function with different locks is not a correctness issue
(SetOnce::populate() already handle the synchronization). User may want
to synchronize different groups of writers with different locks.

Regards,
Boqun

> > +    where
> > +        B: lock::Backend,
> > +        F: FnOnce() -> Result<T, E>,
> > +    {
> > +        if let Some(value) = self.as_ref() {
> > +            return Ok(value);
> > +        }
> > +
> > +        let mut to_insert = f()?;
[...]

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

[Alice Ryhl]

On Wed, Jul 29, 2026 at 11:05:55PM +0900, Alexandre Courbot wrote:
> On Wed Jul 22, 2026 at 6:16 PM JST, Alice Ryhl wrote:
> > +    where
> > +        B: lock::Backend,
> > +        F: FnOnce() -> Result<T, E>,
> > +    {
> > +        if let Some(value) = self.as_ref() {
> > +            return Ok(value);
> > +        }
> > +
> > +        let mut to_insert = f()?;
> 
> This means that `f` can run more than once for a given `SetOnce`, which
> can lead to problems depending on `f`'s' side-effects.
> 
> In the GEM shmem case, we would create a second `SGTableMap`, and since
> `SGTableMap` assumes it is the sole owner, the last instance to drop
> would create a use-after-free.
> 
> Now this sounds more like a problem with `SGTableMap`, but if we cannot
> avoid calling `f` at least twice then I think it would help if this was
> documented.

Hmm ... this is the behavior I want in Binder. Actually creating the
value is an allocation, but my lock is a spinlock so I cannot invoke f()
under the lock. This means that each caller will make their own
allocation, and then we throw away any extras if there are concurrent
callers.

If GEM shmem wants f() invoked under the lock, then that's just a
different operation than the one Binder wants.

Do you think we should add both?

Alice

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

[Alexandre Courbot]

On Thu Jul 30, 2026 at 6:11 PM JST, Alice Ryhl wrote:
> On Wed, Jul 29, 2026 at 11:05:55PM +0900, Alexandre Courbot wrote:
>> On Wed Jul 22, 2026 at 6:16 PM JST, Alice Ryhl wrote:
>> > +    where
>> > +        B: lock::Backend,
>> > +        F: FnOnce() -> Result<T, E>,
>> > +    {
>> > +        if let Some(value) = self.as_ref() {
>> > +            return Ok(value);
>> > +        }
>> > +
>> > +        let mut to_insert = f()?;
>> 
>> This means that `f` can run more than once for a given `SetOnce`, which
>> can lead to problems depending on `f`'s' side-effects.
>> 
>> In the GEM shmem case, we would create a second `SGTableMap`, and since
>> `SGTableMap` assumes it is the sole owner, the last instance to drop
>> would create a use-after-free.
>> 
>> Now this sounds more like a problem with `SGTableMap`, but if we cannot
>> avoid calling `f` at least twice then I think it would help if this was
>> documented.
>
> Hmm ... this is the behavior I want in Binder. Actually creating the
> value is an allocation, but my lock is a spinlock so I cannot invoke f()
> under the lock. This means that each caller will make their own
> allocation, and then we throw away any extras if there are concurrent
> callers.
>
> If GEM shmem wants f() invoked under the lock, then that's just a
> different operation than the one Binder wants.
>
> Do you think we should add both?

Possibly... but this would introduce a potential footgun that sleeps
while holding a spinlock, unless we limit the run-f-under-lock version
to work only with mutexes.

Maybe the proper solution is to fix `SGTableMap` so it supports multiple
instantiations. It is, after all, a bit footgunny on its own.

In any case, the current versions should warn users about the fact that
`f` can be called more than once imho.