linux-kernel - Re: [PATCH] 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: <aIJzXV8XyIo5O3F6@tardis-2.local>
Date: Thu, 24 Jul 2025 10:54:37 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Lyude Paul <lyude@...hat.com>
Cc: rust-for-linux@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	linux-kernel@...r.kernel.org, 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>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH] rust: lock: Export Guard::do_unlocked()

On Thu, Jul 24, 2025 at 01:25:00PM -0400, 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.
> 
> 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>

Seems reasonable.

> ---
> You can find an example usage of this here:
> 
>   https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-slim/drivers/gpu/drm/rvkms/crtc.rs
> 
>  rust/kernel/sync/lock.rs | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index e82fa5be289c1..60eb98805a489 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -228,7 +228,13 @@ pub fn lock_ref(&self) -> &'a Lock<T, B> {
>          self.lock
>      }
>  
> -    pub(crate) fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
> +    /// Unlock this [`Guard`]'s lock temporarily, execute `cb`, and then re-lock it.

I would like to change this line as the following to be consistent with
other comments in the file.

/// Releases this [`Guard`]'s lock temporary, 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.
> +    ///

Could you add an "# Examples" section here with a simple example?
Thanks!

Otherwise it looks good to me, thanks!

Regards
Boqun

> +    /// If the closure returns a value, it will be returned by this function.
> +    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
> -- 
> 2.50.0
>