linux-kernel - Re: [PATCH v3 1/2] rust/revocable: add try_access

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: <D908W8AWVRHW.30H6WLUHQ7QZL@nvidia.com>
Date: Mon, 07 Apr 2025 16:57:50 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Benno Lossin" <benno.lossin@...ton.me>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>, "Danilo Krummrich"
 <dakr@...nel.org>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Bjorn Helgaas" <bhelgaas@...gle.com>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-pci@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] rust/revocable: add try_access_with()
 convenience method

On Mon Apr 7, 2025 at 6:20 AM JST, Benno Lossin wrote:
> On Sun Apr 6, 2025 at 3:58 PM CEST, Alexandre Courbot wrote:
>> Revocable::try_access() returns a guard through which the wrapped object
>> can be accessed. Code that can sleep is not allowed while the guard is
>> held; thus, it is common for the caller to explicitly drop it before
>> running sleepable code, e.g:
>>
>>     let b = bar.try_access()?;
>>     let reg = b.readl(...);
>>
>>     // Don't forget this or things could go wrong!
>>     drop(b);
>>
>>     something_that_might_sleep();
>>
>>     let b = bar.try_access()?;
>>     let reg2 = b.readl(...);
>>
>> This is arguably error-prone. try_access_with() provides an arguably
>> safer alternative, by taking a closure that is run while the guard is
>> held, and by dropping the guard automatically after the closure
>> completes. This way, code can be organized more clearly around the
>> critical sections and the risk of forgetting to release the guard when
>> needed is considerably reduced:
>>
>>     let reg = bar.try_access_with(|b| b.readl(...))?;
>>
>>     something_that_might_sleep();
>>
>>     let reg2 = bar.try_access_with(|b| b.readl(...))?;
>>
>> The closure can return nothing, or any value including a Result which is
>> then wrapped inside the Option returned by try_access_with. Error
>> management is driver-specific, so users are encouraged to create their
>> own macros that map and flatten the returned values to something
>> appropriate for the code they are working on.
>>
>> Acked-by: Danilo Krummrich <dakr@...nel.org>
>> Suggested-by: Danilo Krummrich <dakr@...nel.org>
>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>
> Reviewed-by: Benno Lossin <benno.lossin@...ton.me>
>
>> ---
>>  rust/kernel/revocable.rs | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> index 1e5a9d25c21b279b01f90b02997492aa4880d84f..b91e40e8160be0cc0ff8e0699e48e063c9dbce22 100644
>> --- a/rust/kernel/revocable.rs
>> +++ b/rust/kernel/revocable.rs
>> @@ -123,6 +123,22 @@ pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a
>>          }
>>      }
>>  
>> +    /// Tries to access the wrapped object and run a closure on it while the guard is held.
>> +    ///
>> +    /// This is a convenience method to run short non-sleepable code blocks while ensuring the
>> +    /// guard is dropped afterwards. [`Self::try_access`] carries the risk that the caller will
>> +    /// forget to explicitly drop that returned guard before calling sleepable code; this method
>> +    /// adds an extra safety to make sure it doesn't happen.
>> +    ///
>> +    /// Returns `None` if the object has been revoked and is therefore no longer accessible, or the
>> +    /// result of the closure wrapped in `Some`. If the closure returns a [`Result`] then the
>> +    /// return type becomes `Option<Result<>>`, which can be inconvenient. Users are encouraged to
>> +    /// define their own macro that turns the `Option` into a proper error code and flattens the
>> +    /// inner result into it if it makes sense within their subsystem.
>
> I personally wouldn't have mentioned this in the docs, since to me such
> a helper would be obvious, but I don't mind it either.

Using a helper did not immediately occur to me, which is why I came with
two different methods initially. I'm fine with removing this part of the
doc if it sounds like it is overstepping the purpose of documentation
tough.