linux-kernel - Re: [PATCH v1 2/2] rust: Add read_poll

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: <875xeugncc.fsf@t14s.mail-host-address-is-not-set>
Date: Mon, 11 Aug 2025 12:32:51 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: Danilo Krummrich <dakr@...nel.org>, FUJITA Tomonori
 <fujita.tomonori@...il.com>
Cc: alex.gaynor@...il.com, ojeda@...nel.org, aliceryhl@...gle.com,
 anna-maria@...utronix.de, bjorn3_gh@...tonmail.com, boqun.feng@...il.com,
 frederic@...nel.org, gary@...yguo.net, jstultz@...gle.com,
 linux-kernel@...r.kernel.org, lossin@...nel.org, lyude@...hat.com,
 rust-for-linux@...r.kernel.org, sboyd@...nel.org, tglx@...utronix.de,
 tmgross@...ch.edu, acourbot@...dia.com, daniel.almeida@...labora.com,
 Fiona Behrens <me@...enk.dev>
Subject: Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

"Danilo Krummrich" <dakr@...nel.org> writes:

> On Mon Aug 11, 2025 at 6:10 AM CEST, FUJITA Tomonori wrote:
>> Add read_poll_timeout functions which poll periodically until a
>> condition is met or a timeout is reached.
>>
>> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
>> macro and a simple wrapper for Rust doesn't work. So this implements
>> the same functionality in Rust.
>>
>> The C version uses usleep_range() while the Rust version uses
>> fsleep(), which uses the best sleep method so it works with spans that
>> usleep_range() doesn't work nicely with.
>>
>> The sleep_before_read argument isn't supported since there is no user
>> for now. It's rarely used in the C version.
>>
>> read_poll_timeout() can only be used in a nonatomic context. This
>> requirement is not checked by these abstractions, but it is intended
>> that klint [1] or a similar tool will be used to check it in the
>> future.
>>
>> Link: https://rust-for-linux.com/klint [1]
>> Reviewed-by: Fiona Behrens <me@...enk.dev>
>> Tested-by: Daniel Almeida <daniel.almeida@...labora.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
>> ---
>>  rust/kernel/time.rs      |   1 +
>>  rust/kernel/time/poll.rs | 104 +++++++++++++++++++++++++++++++++++++++
>
> Hm, are we should this should go in the time module? I does use timekeeping
> stuff, but not every user of timekeeping stuff should go under the time module.
>
> This is rather I/O stuff and I'd expect it in rust/kernel/io/poll.rs instead.
>
>> +/// Polls periodically until a condition is met or a timeout is reached.
>> +///
>> +/// The function repeatedly executes the given operation `op` closure and
>> +/// checks its result using the condition closure `cond`.
>> +///
>> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
>> +/// Otherwise, it waits for a duration specified by `sleep_delta`
>> +/// before executing `op` again.
>> +///
>> +/// This process continues until either `cond` returns `true` or the timeout,
>> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>> +///
>> +/// This function can only be used in a nonatomic context.
>> +///
>> +/// # Examples
>> +///
>> +/// ```no_run
>> +/// use kernel::io::Io;
>> +/// use kernel::time::{poll::read_poll_timeout, Delta};
>> +///
>> +/// const HW_READY: u16 = 0x01;
>> +///
>> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
>> +///     // The `op` closure reads the value of a specific status register.
>> +///     let op = || -> Result<u16> { io.try_read16(0x1000) };
>> +///
>> +///     // The `cond` closure takes a reference to the value returned by `op`
>> +///     // and checks whether the hardware is ready.
>> +///     let cond = |val: &u16| *val == HW_READY;
>> +///
>> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>> +///         Ok(_) => {
>> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
>> +///             Ok(())
>> +///         }
>> +///         Err(e) => Err(e),
>> +///     }
>> +/// }
>> +/// ```
>
> This is exactly what I had in mind, thanks!
>
>> +/// ```rust
>> +/// use kernel::sync::{SpinLock, new_spinlock};
>> +/// use kernel::time::Delta;
>> +/// use kernel::time::poll::read_poll_timeout;
>> +///
>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>> +/// let g = lock.lock();
>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>
> I assume you want to demonstrate misuse from atomic contex here? I'd rather not
> do so. But if we really want that, there should be a *very* obvious comment
> about this being wrong somewhere.

I think we should just drop this example.


Best regards,
Andreas Hindborg