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: <20250814.152936.963617059163777662.fujita.tomonori@gmail.com>
Date: Thu, 14 Aug 2025 15:29:36 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: dakr@...nel.org
Cc: fujita.tomonori@...il.com, a.hindborg@...nel.org,
 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,
 me@...enk.dev
Subject: Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

On Mon, 11 Aug 2025 11:55:56 +0200
"Danilo Krummrich" <dakr@...nel.org> wrote:

> 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.

Either is fine by me.

If there are no other opinions, I’ll go with io/poll.rs in the next
version.


>> +/// ```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 was discussing with Andreas, and I’ll remove this example.

>> +/// drop(g);
>> +///
>> +/// # Ok::<(), Error>(())
>> +/// ```
>> +#[track_caller]
>> +pub fn read_poll_timeout<Op, Cond, T>(
>> +    mut op: Op,
>> +    mut cond: Cond,
>> +    sleep_delta: Delta,
>> +    timeout_delta: Option<Delta>,
>> +) -> Result<T>
>> +where
>> +    Op: FnMut() -> Result<T>,
>> +    Cond: FnMut(&T) -> bool,
>> +{
>> +    let start: Instant<Monotonic> = Instant::now();
>> +    let sleep = !sleep_delta.is_zero();
>> +
>> +    // Unlike the C version, we always call `might_sleep()`.
> 
> I think we should explain why, i.e. the argument about being error prone, clear
> separation of read_poll_timeout() and read_poll_timeout_atomic() for klint, etc.
> (I also think the C version should not have done this conditionally to begin
> with.)

// Unlike the C version, we always call `might_sleep()`, because
// conditional calls are error-prone. We clearly separate the
// `read_poll_timeout()` and `read_poll_timeout_atomic()` functions for
// tools like klint.

Looks reasonable?