linux-kernel - Re: [PATCH v10 7/8] 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: <20250215.184801.161111735013966961.fujita.tomonori@gmail.com>
Date: Sat, 15 Feb 2025 18:48:01 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: gary@...yguo.net
Cc: fujita.tomonori@...il.com, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org, netdev@...r.kernel.org, andrew@...n.ch,
 hkallweit1@...il.com, tmgross@...ch.edu, ojeda@...nel.org,
 alex.gaynor@...il.com, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
 a.hindborg@...sung.com, aliceryhl@...gle.com, anna-maria@...utronix.de,
 frederic@...nel.org, tglx@...utronix.de, arnd@...db.de,
 jstultz@...gle.com, sboyd@...nel.org, mingo@...hat.com,
 peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org,
 dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
 mgorman@...e.de, vschneid@...hat.com, tgunders@...hat.com, me@...enk.dev
Subject: Re: [PATCH v10 7/8] rust: Add read_poll_timeout functions

On Fri, 14 Feb 2025 11:37:40 +0000
Gary Guo <gary@...yguo.net> wrote:

> On Fri, 14 Feb 2025 13:05:30 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@...il.com> wrote:
> 
>> On Sun, 9 Feb 2025 16:20:48 +0000
>> Gary Guo <gary@...yguo.net> wrote:
>> 
>> >> +fn might_sleep(loc: &Location<'_>) {
>> >> +    // SAFETY: FFI call.
>> >> +    unsafe {
>> >> +        crate::bindings::__might_sleep_precision(
>> >> +            loc.file().as_ptr().cast(),
>> >> +            loc.file().len() as i32,
>> >> +            loc.line() as i32,
>> >> +        )
>> >> +    }
>> >> +}  
>> > 
>> > One last Q: why isn't `might_sleep` marked as `track_caller` and then
>> > have `Location::caller` be called internally?
>> >
>> > It would make the API same as the C macro.  
>> 
>> Equivalent to the C side __might_sleep(), not might_sleep(). To avoid
>> confusion, it might be better to change the name of this function.
>> 
>> The reason why __might_sleep() is used instead of might_sleep() is
>> might_sleep() can't always be called. It was discussed in v2:
>> 
>> https://lore.kernel.org/all/ZwPT7HZvG1aYONkQ@boqun-archlinux/
> 
> I don't follow. `__might_sleep` or `might_sleep` wouldn't make a
> difference here, given that this function may actually sleep.

Yeah, it doesn't matter here. If I understand correctly, the
discussion is about whether might_sleep() itself should be unsafe
considering the case where it is called from other functions. I simply
chose uncontroversial __might_sleep().

After reviewing the code again, I realized that I made a mistake;
__might_sleep() should only be executed when CONFIG_DEBUG_ATOMIC_SLEEP
is enabled. I also think that it is confusing that might_sleep() calls
C's __might_sleep().

How about implementing the equivalent to might_sleep()?

/// Annotation for functions that can sleep.
///
/// Equivalent to the C side [`might_sleep()`], this function serves as
/// a debugging aid and a potential scheduling point.
///
/// This function can only be used in a nonatomic context.
#[track_caller]
fn might_sleep() {
    #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
    {
        let loc = core::panic::Location::caller();
	// SAFETY: FFI call.
	unsafe {
	    crate::bindings::__might_sleep_precision(
	        loc.file().as_ptr().cast(),
	        loc.file().len() as i32,
                loc.line() as i32,
	    )
	}
    }
    // SAFETY: FFI call.
    unsafe { crate::bindings::might_resched() }
}


>> > Also -- perhaps this function can be public (though I guess you'd need
>> > to put it in a new module).  
>> 
>> Wouldn't it be better to keep it private until actual users appear?

I'll make the above public if you think that is the better approach.

C's might_sleep() is defined in linux/kernel.h but kernel/kernel.rs
isn't a good choice, I guess. kernel/sched.rs or other options?