linux-kernel - Re: [PATCH v3] rust: lock: Export Guard::do

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <DBL2U6WPW5TO.1KXQ8V64MRRHU@kernel.org>
Date: Fri, 25 Jul 2025 12:51:00 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Lyude Paul" <lyude@...hat.com>, <rust-for-linux@...r.kernel.org>,
 "Thomas Gleixner" <tglx@...utronix.de>, "Boqun Feng"
 <boqun.feng@...il.com>, <linux-kernel@...r.kernel.org>
Cc: "Peter Zijlstra" <peterz@...radead.org>, "Ingo Molnar"
 <mingo@...hat.com>, "Will Deacon" <will@...nel.org>, "Waiman Long"
 <longman@...hat.com>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Danilo Krummrich" <dakr@...nel.org>
Subject: Re: [PATCH v3] rust: lock: Export Guard::do_unlocked()

On Thu Jul 24, 2025 at 11:15 PM CEST, Lyude Paul wrote:
> In RVKMS, I discovered a silly issue where as a result of our HrTimer for
> vblank emulation and our vblank enable/disable callbacks sharing a
> spinlock, it was possible to deadlock while trying to disable the vblank
> timer.
>
> The solution for this ended up being simple: keep track of when the HrTimer
> could potentially acquire the shared spinlock, and simply drop the spinlock
> temporarily from our vblank enable/disable callbacks when stopping the
> timer. And do_unlocked() ended up being perfect for this.

In this case, couldn't you also just add another function to stopping
the timer that takes the lock guard (instead of locking the lock itself)?

> Since this seems like it's useful, let's export this for use by the rest of
> the world and write short documentation for it.
>
> Signed-off-by: Lyude Paul <lyude@...hat.com>
>
> ---
> V2:
> * Fix documentation for do_unlocked
> * Add an example
> V3:
> * Documentation changes from Miguel

Please wait a couple days before sending a new version, thanks!

>  rust/kernel/sync/lock.rs | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index e82fa5be289c1..800cdd16dce6e 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -228,7 +228,40 @@ pub fn lock_ref(&self) -> &'a Lock<T, B> {
>          self.lock
>      }
>  
> -    pub(crate) fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
> +    /// Releases this [`Guard`]'s lock temporarily, executes `cb` and then re-acquires it.
> +    ///
> +    /// This can be useful for situations where you may need to do a temporary unlock dance to avoid
> +    /// issues like circular locking dependencies.
> +    ///
> +    /// It returns the value returned by the closure.
> +    ///
> +    /// # Examples
> +    ///
> +    /// The following example shows how to use [`Guard::do_unlocked`] to temporarily release a lock,
> +    /// do some work, then re-lock it.

I would remove this sentence, as the example is pretty easy to follow
(at least at the moment).

> +    ///
> +    /// ```
> +    /// # use kernel::{new_spinlock, sync::lock::{Backend, Guard, Lock}};
> +    /// # use pin_init::stack_pin_init;
> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
> +    ///     // Address-equal means the same lock.
> +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
> +    /// }
> +    ///
> +    /// stack_pin_init! {
> +    ///     let l = new_spinlock!(42)
> +    /// }

I normally write

    stack_pin_init!(let l = new_spinlock!(42));

Or do you feel like that's less readable?

> +    ///
> +    /// let mut g = l.lock();
> +    /// let val = *g;
> +    ///
> +    /// // The lock will be released, but only temporarily
> +    /// g.do_unlocked(|| assert_eq!(val, 42));

I feel like this example doesn't show how `do_unlocked` is useful. How
about you add a function that locks the lock & you call it from inside
the `do_unlocked` call? Similar to the issue you described in the commit
message?

---
Cheers,
Benno

> +    ///
> +    /// // `g` originates from `l` and should be relocked now.
> +    /// assert_held(&g, &l);
> +    /// ```
> +    pub fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
>          // SAFETY: The caller owns the lock, so it is safe to unlock it.
>          unsafe { B::unlock(self.lock.state.get(), &self.state) };
>  
>
> base-commit: dff64b072708ffef23c117fa1ee1ea59eb417807