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 <[email protected]> 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/[email protected]?part=2
