linux-kernel - Re: [PATCH] rust: sync: lock: Add an example for Guard::lock

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87r03n1tmd.fsf@kernel.org>
Date: Mon, 24 Feb 2025 12:15:22 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <benno.lossin@...ton.me>
Cc: "Boqun Feng" <boqun.feng@...il.com>,  <linux-kernel@...r.kernel.org>,
  <rust-for-linux@...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>,  "Alice
 Ryhl" <aliceryhl@...gle.com>,  "Trevor Gross" <tmgross@...ch.edu>
Subject: Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

"Benno Lossin" <benno.lossin@...ton.me> writes:

> On 24.02.25 09:08, Andreas Hindborg wrote:
>> Boqun Feng <boqun.feng@...il.com> writes:
>>
>>> To provide examples on usage of `Guard::lock_ref()` along with the unit
>>> test, an "assert a lock is held by a guard" example is added.
>>>
>>> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
>>> ---
>>> This depends on Alice's patch:
>>>
>>> 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
>>>
>>> I'm also OK to fold this in if Alice thinks it's fine.
>>>
>>>  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
>>> index 3701fac6ebf6..6d868e35b0a3 100644
>>> --- a/rust/kernel/sync/lock.rs
>>> +++ b/rust/kernel/sync/lock.rs
>>> @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>>>
>>>  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
>>>      /// Returns the lock that this guard originates from.
>>> +    ///
>>> +    /// # Examples
>>> +    ///
>>> +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
>>> +    /// lock is held.
>>> +    ///
>>> +    /// ```
>>> +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
>>> +    ///
>>> +    /// 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));
>>> +    /// }
>>
>> This seems super useful. Perhaps add this method as part of the lock api
>> instead of just having it in the example?
>
> I don't think it should be an assert. Instead make it return a
> `Result<(), ()>`. (or create better named unit error types)

No, this should not be part of usual control flow, and developers should
not make control flow decisions based on this. It would always be an
assertion. But you are right that `assert!` is probably not what we
want. `debug_assert!` might be fine though.


Best regards,
Andreas Hindborg