| Message ID | 20241005122531.20298-6-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rust: Add IO polling | expand |
> might_sleep() is called via a wrapper so the __FILE__ and __LINE__ > debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we > expect; the wrapper instead of the caller. So not very useful. All we know is that somewhere in Rust something is sleeping in atomic context. Is it possible to do better? Does __FILE__ and __LINE__ exist in Rust? > + if sleep { > + // SAFETY: FFI call. > + unsafe { bindings::might_sleep() } > + } What is actually unsafe about might_sleep()? It is a void foo(void) function, so takes no parameters, returns no results. It cannot affect anything which Rust is managing. > + // SAFETY: FFI call. > + unsafe { bindings::cpu_relax() } Same here. Andrew
On Sat, Oct 05, 2024 at 08:32:01PM +0200, Andrew Lunn wrote: > > might_sleep() is called via a wrapper so the __FILE__ and __LINE__ > > debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we > > expect; the wrapper instead of the caller. > > So not very useful. All we know is that somewhere in Rust something is > sleeping in atomic context. Is it possible to do better? Does __FILE__ > and __LINE__ exist in Rust? > Sure, you can use: https://doc.rust-lang.org/core/macro.line.html > > + if sleep { > > + // SAFETY: FFI call. > > + unsafe { bindings::might_sleep() } > > + } > > What is actually unsafe about might_sleep()? It is a void foo(void) Every extern "C" function is by default unsafe, because C doesn't have the concept of safe/unsafe. If you want to avoid unsafe, you could introduce a Rust's might_sleep() which calls into `bindings::might_sleep()`: pub fn might_sleep() { // SAFETY: ?? unsafe { bindings::might_sleep() } } however, if you call a might_sleep() in a preemption disabled context when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means an unexpected RCU quiescent state, which results an early RCU grace period, and that may mean a use-after-free. So it's not that safe as you may expected. Regards, Boqun > function, so takes no parameters, returns no results. It cannot affect > anything which Rust is managing. > > > + // SAFETY: FFI call. > > + unsafe { bindings::cpu_relax() } > > Same here. > > Andrew >
On Sat, Oct 05, 2024 at 03:22:23PM -0700, Boqun Feng wrote: > On Sat, Oct 05, 2024 at 08:32:01PM +0200, Andrew Lunn wrote: > > > might_sleep() is called via a wrapper so the __FILE__ and __LINE__ > > > debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we > > > expect; the wrapper instead of the caller. > > > > So not very useful. All we know is that somewhere in Rust something is > > sleeping in atomic context. Is it possible to do better? Does __FILE__ > > and __LINE__ exist in Rust? > > > > Sure, you can use: > > https://doc.rust-lang.org/core/macro.line.html So i guess might_sleep() needs turning into some sort of macro, calling __might_sleep(__FILE__, __LINE__); might_resched(); > > > + if sleep { > > > + // SAFETY: FFI call. > > > + unsafe { bindings::might_sleep() } > > > + } > > > > What is actually unsafe about might_sleep()? It is a void foo(void) > > Every extern "C" function is by default unsafe, because C doesn't have > the concept of safe/unsafe. If you want to avoid unsafe, you could > introduce a Rust's might_sleep() which calls into > `bindings::might_sleep()`: > > pub fn might_sleep() { > // SAFETY: ?? > unsafe { bindings::might_sleep() } > } > > however, if you call a might_sleep() in a preemption disabled context > when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means > an unexpected RCU quiescent state, which results an early RCU grace > period, and that may mean a use-after-free. So it's not that safe as you > may expected. If you call might_sleep() in a preemption disabled context you code is already unsafe, since that is the whole point of it, to find bugs where you use a sleeping function in atomic context. Depending on why you are in atomic context, it might appear to work, until it does not actually work, and bad things happen. So it is not might_sleep() which is unsafe, it is the Rust code calling it. Andrew
On Sun, 6 Oct 2024 16:45:21 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Sat, Oct 05, 2024 at 03:22:23PM -0700, Boqun Feng wrote: >> On Sat, Oct 05, 2024 at 08:32:01PM +0200, Andrew Lunn wrote: >> > > might_sleep() is called via a wrapper so the __FILE__ and __LINE__ >> > > debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we >> > > expect; the wrapper instead of the caller. >> > >> > So not very useful. All we know is that somewhere in Rust something is >> > sleeping in atomic context. Is it possible to do better? Does __FILE__ >> > and __LINE__ exist in Rust? >> > >> >> Sure, you can use: >> >> https://doc.rust-lang.org/core/macro.line.html > > So i guess might_sleep() needs turning into some sort of macro, calling > __might_sleep(__FILE__, __LINE__); might_resched(); Yeah, I think we could do such. Or we could drop the might_sleep call here? We might be able to expect the improvement C support in Rust in the future.
On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote: [...] > > > > + if sleep { > > > > + // SAFETY: FFI call. > > > > + unsafe { bindings::might_sleep() } > > > > + } > > > > > > What is actually unsafe about might_sleep()? It is a void foo(void) > > > > Every extern "C" function is by default unsafe, because C doesn't have > > the concept of safe/unsafe. If you want to avoid unsafe, you could > > introduce a Rust's might_sleep() which calls into > > `bindings::might_sleep()`: > > > > pub fn might_sleep() { > > // SAFETY: ?? > > unsafe { bindings::might_sleep() } > > } > > > > however, if you call a might_sleep() in a preemption disabled context > > when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means > > an unexpected RCU quiescent state, which results an early RCU grace > > period, and that may mean a use-after-free. So it's not that safe as you > > may expected. > > If you call might_sleep() in a preemption disabled context you code is > already unsafe, since that is the whole point of it, to find bugs Well, in Rust, the rule is: any type-checked (compiled successfully) code that only calls safe Rust functions cannot be unsafe. So the fact that calling might_sleep() in a preemption disabled context is unsafe means that something has to be unsafe. This eventually can turn into a "blaming game" in the design space: we can either design the preemption disable function as unsafe or the might_sleep() function as unsafe. But one of them has to be unsafe function, otherwise we are breaking the safe code guarantee. However, this is actually a special case: currently we want to use klint [1] to detect all context mis-matches at compile time. So the above rule extends for kernel: any type-checked *and klint-checked* code that only calls safe Rust functions cannot be unsafe. I.e. we add additional compile time checking for unsafe code. So if might_sleep() has the proper klint annotation, and we actually enable klint for kernel code, then we can make it safe (along with preemption disable functions being safe). > where you use a sleeping function in atomic context. Depending on why > you are in atomic context, it might appear to work, until it does not > actually work, and bad things happen. So it is not might_sleep() which > is unsafe, it is the Rust code calling it. The whole point of unsafe functions is that calling it may result into unsafe code, so that's why all extern "C" functions are unsafe, so are might_sleep() (without klint in the picture). [1]: https://lwn.net/Articles/951550/ Regards, Boqun > > Andrew > > > >
On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote: > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote: > [...] > > > > > + if sleep { > > > > > + // SAFETY: FFI call. > > > > > + unsafe { bindings::might_sleep() } > > > > > + } > > > > > > > > What is actually unsafe about might_sleep()? It is a void foo(void) > > > > > > Every extern "C" function is by default unsafe, because C doesn't have > > > the concept of safe/unsafe. If you want to avoid unsafe, you could > > > introduce a Rust's might_sleep() which calls into > > > `bindings::might_sleep()`: > > > > > > pub fn might_sleep() { > > > // SAFETY: ?? > > > unsafe { bindings::might_sleep() } > > > } > > > > > > however, if you call a might_sleep() in a preemption disabled context > > > when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means > > > an unexpected RCU quiescent state, which results an early RCU grace > > > period, and that may mean a use-after-free. So it's not that safe as you > > > may expected. > > > > If you call might_sleep() in a preemption disabled context you code is > > already unsafe, since that is the whole point of it, to find bugs > > Well, in Rust, the rule is: any type-checked (compiled successfully) > code that only calls safe Rust functions cannot be unsafe. So the fact > that calling might_sleep() in a preemption disabled context is unsafe > means that something has to be unsafe. > > This eventually can turn into a "blaming game" in the design space: we > can either design the preemption disable function as unsafe or the > might_sleep() function as unsafe. But one of them has to be unsafe > function, otherwise we are breaking the safe code guarantee. Just keep in mind, it could of been C which put you into atomic context before calling into Rust. An interrupt handler would be a good example, and i'm sure there are others. > However, this is actually a special case: currently we want to use klint > [1] to detect all context mis-matches at compile time. So the above rule > extends for kernel: any type-checked *and klint-checked* code that only > calls safe Rust functions cannot be unsafe. I.e. we add additional > compile time checking for unsafe code. So if might_sleep() has the > proper klint annotation, and we actually enable klint for kernel code, > then we can make it safe (along with preemption disable functions being > safe). > > > where you use a sleeping function in atomic context. Depending on why > > you are in atomic context, it might appear to work, until it does not > > actually work, and bad things happen. So it is not might_sleep() which > > is unsafe, it is the Rust code calling it. > > The whole point of unsafe functions is that calling it may result into > unsafe code, so that's why all extern "C" functions are unsafe, so are > might_sleep() (without klint in the picture). There is a psychological part to this. might_sleep() is a good debug tool, which costs very little in normal builds, but finds logic bugs when enabled in debug builds. What we don't want is Rust developers not scattering it though their code because it adds unsafe code, and the aim is not to have any unsafe code. Andrew
On Mon, Oct 07, 2024 at 03:48:09PM +0200, Andrew Lunn wrote: > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote: > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote: > > [...] > > > > > > + if sleep { > > > > > > + // SAFETY: FFI call. > > > > > > + unsafe { bindings::might_sleep() } > > > > > > + } > > > > > > > > > > What is actually unsafe about might_sleep()? It is a void foo(void) > > > > > > > > Every extern "C" function is by default unsafe, because C doesn't have > > > > the concept of safe/unsafe. If you want to avoid unsafe, you could > > > > introduce a Rust's might_sleep() which calls into > > > > `bindings::might_sleep()`: > > > > > > > > pub fn might_sleep() { > > > > // SAFETY: ?? > > > > unsafe { bindings::might_sleep() } > > > > } > > > > > > > > however, if you call a might_sleep() in a preemption disabled context > > > > when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means > > > > an unexpected RCU quiescent state, which results an early RCU grace > > > > period, and that may mean a use-after-free. So it's not that safe as you > > > > may expected. > > > > > > If you call might_sleep() in a preemption disabled context you code is > > > already unsafe, since that is the whole point of it, to find bugs > > > > Well, in Rust, the rule is: any type-checked (compiled successfully) > > code that only calls safe Rust functions cannot be unsafe. So the fact > > that calling might_sleep() in a preemption disabled context is unsafe > > means that something has to be unsafe. > > > > This eventually can turn into a "blaming game" in the design space: we > > can either design the preemption disable function as unsafe or the > > might_sleep() function as unsafe. But one of them has to be unsafe > > function, otherwise we are breaking the safe code guarantee. > > Just keep in mind, it could of been C which put you into atomic > context before calling into Rust. An interrupt handler would be a good > example, and i'm sure there are others. > That's why the klint approach is preferred right now. Without klint, and if we don't want to mark might_sleep() as unsafe, we probably need to mark the registration of an interrupt handler unsafe, and the safety requirement would be "making sure the handler doesn't call schedule()". > > However, this is actually a special case: currently we want to use klint > > [1] to detect all context mis-matches at compile time. So the above rule > > extends for kernel: any type-checked *and klint-checked* code that only > > calls safe Rust functions cannot be unsafe. I.e. we add additional > > compile time checking for unsafe code. So if might_sleep() has the > > proper klint annotation, and we actually enable klint for kernel code, > > then we can make it safe (along with preemption disable functions being > > safe). > > > > > where you use a sleeping function in atomic context. Depending on why > > > you are in atomic context, it might appear to work, until it does not > > > actually work, and bad things happen. So it is not might_sleep() which > > > is unsafe, it is the Rust code calling it. > > > > The whole point of unsafe functions is that calling it may result into > > unsafe code, so that's why all extern "C" functions are unsafe, so are > > might_sleep() (without klint in the picture). > > There is a psychological part to this. might_sleep() is a good debug > tool, which costs very little in normal builds, but finds logic bugs > when enabled in debug builds. What we don't want is Rust developers > not scattering it though their code because it adds unsafe code, and > the aim is not to have any unsafe code. > Sure, but my point is these need to be put together into a proper design. For example, spin_lock() is currently exposed into Rust as a safe API lock(), so the following code is unsafe: let g = lock1.lock(); // lock1 is a spinlock might_sleep(); drop(g); without the klint rule, if we want to mark might_sleep() as safe, then we need to mark lock() as unsafe, otherwise, it's an unsafe code block constructed by pure safe functions. However, compared to might_sleep(), I think we would like keep lock() as safe since it is used more widely. Regards, Boqun > Andrew
On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote: > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote: > > However, this is actually a special case: currently we want to use klint > > [1] to detect all context mis-matches at compile time. So the above rule > > extends for kernel: any type-checked *and klint-checked* code that only > > calls safe Rust functions cannot be unsafe. I.e. we add additional > > compile time checking for unsafe code. So if might_sleep() has the > > proper klint annotation, and we actually enable klint for kernel code, > > then we can make it safe (along with preemption disable functions being > > safe). > > > > > where you use a sleeping function in atomic context. Depending on why > > > you are in atomic context, it might appear to work, until it does not > > > actually work, and bad things happen. So it is not might_sleep() which > > > is unsafe, it is the Rust code calling it. > > > > The whole point of unsafe functions is that calling it may result into > > unsafe code, so that's why all extern "C" functions are unsafe, so are > > might_sleep() (without klint in the picture). > > There is a psychological part to this. might_sleep() is a good debug > tool, which costs very little in normal builds, but finds logic bugs > when enabled in debug builds. What we don't want is Rust developers > not scattering it though their code because it adds unsafe code, and > the aim is not to have any unsafe code. We can add a safe wrapper for it: pub fn might_sleep() { // SAFETY: Always safe to call. unsafe { bindings::might_sleep() }; } Alice
On Mon, Oct 07, 2024 at 04:08:48PM +0200, Alice Ryhl wrote: > On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote: > > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote: > > > However, this is actually a special case: currently we want to use klint > > > [1] to detect all context mis-matches at compile time. So the above rule > > > extends for kernel: any type-checked *and klint-checked* code that only > > > calls safe Rust functions cannot be unsafe. I.e. we add additional > > > compile time checking for unsafe code. So if might_sleep() has the > > > proper klint annotation, and we actually enable klint for kernel code, > > > then we can make it safe (along with preemption disable functions being > > > safe). > > > > > > > where you use a sleeping function in atomic context. Depending on why > > > > you are in atomic context, it might appear to work, until it does not > > > > actually work, and bad things happen. So it is not might_sleep() which > > > > is unsafe, it is the Rust code calling it. > > > > > > The whole point of unsafe functions is that calling it may result into > > > unsafe code, so that's why all extern "C" functions are unsafe, so are > > > might_sleep() (without klint in the picture). > > > > There is a psychological part to this. might_sleep() is a good debug > > tool, which costs very little in normal builds, but finds logic bugs > > when enabled in debug builds. What we don't want is Rust developers > > not scattering it though their code because it adds unsafe code, and > > the aim is not to have any unsafe code. > > We can add a safe wrapper for it: > > pub fn might_sleep() { > // SAFETY: Always safe to call. > unsafe { bindings::might_sleep() }; It's not always safe to call, because might_sleep() has a might_resched() and in preempt=voluntary kernel, that's a cond_resched(), which may eventually call __schedule() and report a quiescent state of RCU. This could means an unexpected early grace period, and that means a potential use-afer-free. Regards, Boqun > } > > Alice
On Mon, Oct 7, 2024 at 4:14 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Mon, Oct 07, 2024 at 04:08:48PM +0200, Alice Ryhl wrote: > > On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote: > > > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote: > > > > However, this is actually a special case: currently we want to use klint > > > > [1] to detect all context mis-matches at compile time. So the above rule > > > > extends for kernel: any type-checked *and klint-checked* code that only > > > > calls safe Rust functions cannot be unsafe. I.e. we add additional > > > > compile time checking for unsafe code. So if might_sleep() has the > > > > proper klint annotation, and we actually enable klint for kernel code, > > > > then we can make it safe (along with preemption disable functions being > > > > safe). > > > > > > > > > where you use a sleeping function in atomic context. Depending on why > > > > > you are in atomic context, it might appear to work, until it does not > > > > > actually work, and bad things happen. So it is not might_sleep() which > > > > > is unsafe, it is the Rust code calling it. > > > > > > > > The whole point of unsafe functions is that calling it may result into > > > > unsafe code, so that's why all extern "C" functions are unsafe, so are > > > > might_sleep() (without klint in the picture). > > > > > > There is a psychological part to this. might_sleep() is a good debug > > > tool, which costs very little in normal builds, but finds logic bugs > > > when enabled in debug builds. What we don't want is Rust developers > > > not scattering it though their code because it adds unsafe code, and > > > the aim is not to have any unsafe code. > > > > We can add a safe wrapper for it: > > > > pub fn might_sleep() { > > // SAFETY: Always safe to call. > > unsafe { bindings::might_sleep() }; > > It's not always safe to call, because might_sleep() has a > might_resched() and in preempt=voluntary kernel, that's a > cond_resched(), which may eventually call __schedule() and report a > quiescent state of RCU. This could means an unexpected early grace > period, and that means a potential use-afer-free. Atomicity violations are intended to be caught by klint. If you want to change that decision, you'll have to add unsafe to all functions that sleep including Mutex::lock, CondVar::wait, and many others. Alice
On Mon, Oct 07, 2024 at 04:16:46PM +0200, Alice Ryhl wrote: > On Mon, Oct 7, 2024 at 4:14 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > On Mon, Oct 07, 2024 at 04:08:48PM +0200, Alice Ryhl wrote: > > > On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote: > > > > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote: > > > > > However, this is actually a special case: currently we want to use klint > > > > > [1] to detect all context mis-matches at compile time. So the above rule > > > > > extends for kernel: any type-checked *and klint-checked* code that only > > > > > calls safe Rust functions cannot be unsafe. I.e. we add additional > > > > > compile time checking for unsafe code. So if might_sleep() has the > > > > > proper klint annotation, and we actually enable klint for kernel code, > > > > > then we can make it safe (along with preemption disable functions being > > > > > safe). > > > > > > > > > > > where you use a sleeping function in atomic context. Depending on why > > > > > > you are in atomic context, it might appear to work, until it does not > > > > > > actually work, and bad things happen. So it is not might_sleep() which > > > > > > is unsafe, it is the Rust code calling it. > > > > > > > > > > The whole point of unsafe functions is that calling it may result into > > > > > unsafe code, so that's why all extern "C" functions are unsafe, so are > > > > > might_sleep() (without klint in the picture). > > > > > > > > There is a psychological part to this. might_sleep() is a good debug > > > > tool, which costs very little in normal builds, but finds logic bugs > > > > when enabled in debug builds. What we don't want is Rust developers > > > > not scattering it though their code because it adds unsafe code, and > > > > the aim is not to have any unsafe code. > > > > > > We can add a safe wrapper for it: > > > > > > pub fn might_sleep() { > > > // SAFETY: Always safe to call. > > > unsafe { bindings::might_sleep() }; > > > > It's not always safe to call, because might_sleep() has a > > might_resched() and in preempt=voluntary kernel, that's a > > cond_resched(), which may eventually call __schedule() and report a > > quiescent state of RCU. This could means an unexpected early grace > > period, and that means a potential use-afer-free. > > Atomicity violations are intended to be caught by klint. If you want Yes, I already mentioned this to Andrew previously. > to change that decision, you'll have to add unsafe to all functions > that sleep including Mutex::lock, CondVar::wait, and many others. No, I'm not trying to change that decision, just to make it clear that we can mark might_sleep() as safe because of the decision, not because it's really safe even without klint... Regards, Boqun > > Alice
On Mon, Oct 07, 2024 at 07:19:56AM -0700, Boqun Feng wrote: > On Mon, Oct 07, 2024 at 04:16:46PM +0200, Alice Ryhl wrote: > > On Mon, Oct 7, 2024 at 4:14 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > On Mon, Oct 07, 2024 at 04:08:48PM +0200, Alice Ryhl wrote: > > > > On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > > > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote: > > > > > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote: > > > > > > However, this is actually a special case: currently we want to use klint > > > > > > [1] to detect all context mis-matches at compile time. So the above rule > > > > > > extends for kernel: any type-checked *and klint-checked* code that only > > > > > > calls safe Rust functions cannot be unsafe. I.e. we add additional > > > > > > compile time checking for unsafe code. So if might_sleep() has the > > > > > > proper klint annotation, and we actually enable klint for kernel code, > > > > > > then we can make it safe (along with preemption disable functions being > > > > > > safe). > > > > > > > > > > > > > where you use a sleeping function in atomic context. Depending on why > > > > > > > you are in atomic context, it might appear to work, until it does not > > > > > > > actually work, and bad things happen. So it is not might_sleep() which > > > > > > > is unsafe, it is the Rust code calling it. > > > > > > > > > > > > The whole point of unsafe functions is that calling it may result into > > > > > > unsafe code, so that's why all extern "C" functions are unsafe, so are > > > > > > might_sleep() (without klint in the picture). > > > > > > > > > > There is a psychological part to this. might_sleep() is a good debug > > > > > tool, which costs very little in normal builds, but finds logic bugs > > > > > when enabled in debug builds. What we don't want is Rust developers > > > > > not scattering it though their code because it adds unsafe code, and > > > > > the aim is not to have any unsafe code. > > > > > > > > We can add a safe wrapper for it: > > > > > > > > pub fn might_sleep() { > > > > // SAFETY: Always safe to call. > > > > unsafe { bindings::might_sleep() }; > > > > > > It's not always safe to call, because might_sleep() has a > > > might_resched() and in preempt=voluntary kernel, that's a > > > cond_resched(), which may eventually call __schedule() and report a > > > quiescent state of RCU. This could means an unexpected early grace > > > period, and that means a potential use-afer-free. > > > > Atomicity violations are intended to be caught by klint. If you want > > Yes, I already mentioned this to Andrew previously. > > > to change that decision, you'll have to add unsafe to all functions > > that sleep including Mutex::lock, CondVar::wait, and many others. > > No, I'm not trying to change that decision, just to make it clear that > we can mark might_sleep() as safe because of the decision, not because > it's really safe even without klint... > Anyway, I think Tomo needs to call __might_sleep() instead of might_sleep(), and __might_sleep() seems a pure debug function (not involved with schedule at all). So the wrapper of __might_sleep() can be perfectly safe. Regards, Boqun > Regards, > Boqun > > > > > Alice
> > pub fn might_sleep() { > > // SAFETY: Always safe to call. > > unsafe { bindings::might_sleep() }; > > It's not always safe to call, because might_sleep() has a > might_resched() and in preempt=voluntary kernel, that's a > cond_resched(), which may eventually call __schedule() and report a > quiescent state of RCU. This could means an unexpected early grace > period, and that means a potential use-afer-free. How does C handle this? I'm not an RCU person... But if you have called might_sleep() you are about to do something which could sleep. If it does sleep, the scheduler is going to be called, the grace period has ended, and RCU is going to do its thing. If that results in a use-after-free, your code is broken. might_sleep makes no difference here, the code is still broken, it just happens to light the fuse for the explosion a bit earlier. Or, i'm missing something, not being an RCU person. Andrew
On Mon, Oct 07, 2024 at 07:13:40PM +0200, Andrew Lunn wrote: > > > pub fn might_sleep() { > > > // SAFETY: Always safe to call. > > > unsafe { bindings::might_sleep() }; > > > > It's not always safe to call, because might_sleep() has a > > might_resched() and in preempt=voluntary kernel, that's a > > cond_resched(), which may eventually call __schedule() and report a > > quiescent state of RCU. This could means an unexpected early grace > > period, and that means a potential use-afer-free. > > How does C handle this? > > I'm not an RCU person... > > But if you have called might_sleep() you are about to do something > which could sleep. If it does sleep, the scheduler is going to be > called, the grace period has ended, and RCU is going to do its > thing. If that results in a use-after-free, your code is > broken. might_sleep makes no difference here, the code is still > broken, it just happens to light the fuse for the explosion a bit > earlier. > Because of the might_resched() in might_sleep(), it will report the quiescent state of the current CPU, and RCU will pass a grace period if all CPUs have passed a quiescent state. So for example if someone writes the following: <reader> <updater> rcu_read_lock(); p = rcu_dereference(gp); might_sleep(): might_resched(): todo = gp; rcu_assign_pointer(gp, NULL); synchronize_rcu(); rcu_all_qs(); // report a quiescent state inside RCU read-side // critical section, which may make a grace period // pass even there is an active RCU reader kfree(todo); a = READ_ONCE(p->a); // UAF rcu_read_unlock(); We probably call the reader side code a "wrong annotation", however, it's still unsafe code because of the UAF. Also you seems to assume that might_sleep() is always attached to a sleepable function, which is not an invalid assumption, but we couldn't use it for reasoning the safe/unsafe property of Rust functions unless we can encode this in the type system. For Rust code, without klint rule, might_sleep() needs to be unsafe. So we have two options for might_sleep(). * Since we rely on klint for atomic context detection, we can mark the trivial wrapper (as what Alice presented in the other email) as safe, but we need to begin to add klint annotation for that function, unless Gary finds a smart way to auto-annotate functions. * Instead of might_sleep(), we provide the wrapper of __might_sleep(), since it doesn't have might_resched() in it, it should be safe. And all we care about here is the debugging rather than voluntary context switch. (Besides I think preempt=volunatry is eventually going to be gone because of PREEMPT_AUTO [1], if that happens I think the might_resched() might be dropped entirely). Does this make sense? [1]: https://lore.kernel.org/lkml/20240528003521.979836-1-ankur.a.arora@oracle.com/ Regards, Boqun > Or, i'm missing something, not being an RCU person. > > Andrew
> Because of the might_resched() in might_sleep(), it will report the > quiescent state of the current CPU, and RCU will pass a grace period if > all CPUs have passed a quiescent state. So for example if someone writes > the following: > > <reader> <updater> > rcu_read_lock(); > p = rcu_dereference(gp); > might_sleep(): > might_resched(): > todo = gp; > rcu_assign_pointer(gp, NULL); > synchronize_rcu(); > > rcu_all_qs(); // report a quiescent state inside RCU read-side > // critical section, which may make a grace period > // pass even there is an active RCU reader > > kfree(todo); > You are obviously missing something here. The call that actually sleeps mutex_lock(&lock) > a = READ_ONCE(p->a); // UAF > rcu_read_unlock(); A might_sleep() should be paired with something which does actually sleep, under some condition. At least, that is how it is used in C. The iopoll being re-implemented here is an example of that. So take the might_sleep out above, just leaving the mutex_lock. If the mutex is uncontested, the code does not sleep and everything is O.K? If it needs to wait for the mutex, it triggers a UAF. The might_sleep() will also trigger a stack trace, if its is enabled, because you are not allowed to sleep inside rcu_read_lock(), it is an example of atomic context. As far as i see, might_sleep() will cause UAF where there is going to be a UAF anyway. If you are using it correctly, it does not cause UAF. > We probably call the reader side code a "wrong annotation", however, > it's still unsafe code because of the UAF. Also you seems to assume that > might_sleep() is always attached to a sleepable function, which is not > an invalid assumption, but we couldn't use it for reasoning the > safe/unsafe property of Rust functions unless we can encode this in the > type system. How are any of the sleeping call encoded in the type system? I assume any use of a mutex lock, sleep, wait for completion, etc are not all marked as unsafe? There is some sort of wrapper around them? Why not just extend that wrapper to might_sleep(). > For Rust code, without klint rule, might_sleep() needs to > be unsafe. So we have two options for might_sleep(). > > * Since we rely on klint for atomic context detection, we can mark the > trivial wrapper (as what Alice presented in the other email) as safe, > but we need to begin to add klint annotation for that function, unless > Gary finds a smart way to auto-annotate functions. Are there klint annotations for all sleeping functions? > * Instead of might_sleep(), we provide the wrapper of __might_sleep(), > since it doesn't have might_resched() in it, it should be safe. And > all we care about here is the debugging rather than voluntary context > switch. (Besides I think preempt=volunatry is eventually going to be > gone because of PREEMPT_AUTO [1], if that happens I think the > might_resched() might be dropped entirely). __might_sleep() might be safe, but your code is still broken and going to UAF at some point. Don't you want that UAF to happen more reliably and faster so you can find the issue? That would be the advantage of might_sleep() over __might_sleep(). Andrew
On Tue, Oct 08, 2024 at 02:12:51PM +0200, Andrew Lunn wrote: > > Because of the might_resched() in might_sleep(), it will report the > > quiescent state of the current CPU, and RCU will pass a grace period if > > all CPUs have passed a quiescent state. So for example if someone writes > > the following: > > > > <reader> <updater> > > rcu_read_lock(); > > p = rcu_dereference(gp); > > might_sleep(): > > might_resched(): > > todo = gp; > > rcu_assign_pointer(gp, NULL); > > synchronize_rcu(); > > > > rcu_all_qs(); // report a quiescent state inside RCU read-side > > // critical section, which may make a grace period > > // pass even there is an active RCU reader > > > > kfree(todo); > > > > You are obviously missing something here. The call that actually sleeps > > mutex_lock(&lock) > > > a = READ_ONCE(p->a); // UAF > > rcu_read_unlock(); > > A might_sleep() should be paired with something which does actually > sleep, under some condition. At least, that is how it is used in C. How do you guarantee the "should" part? How can a compiler detect a might_sleep() that doesn't have a paired "something which does actually sleep"? I feel like we are just talking through each other, what I was trying to say is might_sleep() is unsafe because the rule of Rust safe code (if we don't consider klint) and I'm using an example here to explain why. And when we are talking about the safe/unsafe attribute of a function, we cannot use the reasoning "this function should be always used with another function". > The iopoll being re-implemented here is an example of that. > > So take the might_sleep out above, just leaving the mutex_lock. If the > mutex is uncontested, the code does not sleep and everything is O.K? > If it needs to wait for the mutex, it triggers a UAF. > > The might_sleep() will also trigger a stack trace, if its is enabled, > because you are not allowed to sleep inside rcu_read_lock(), it is an > example of atomic context. These functionalities you mentioned above are also provided by __might_sleep(), no? > > As far as i see, might_sleep() will cause UAF where there is going to > be a UAF anyway. If you are using it correctly, it does not cause UAF. > Again, I agree with your assumption that might_sleep() will always be paired with a sleep function, but we cannot mark might_sleep() as safe because of that. We can, however, mark might_sleep() as safe because klint is supposed to cover the detection of atomic context violations. But we have a better option: __might_sleep(). > > We probably call the reader side code a "wrong annotation", however, > > it's still unsafe code because of the UAF. Also you seems to assume that > > might_sleep() is always attached to a sleepable function, which is not > > an invalid assumption, but we couldn't use it for reasoning the > > safe/unsafe property of Rust functions unless we can encode this in the > > type system. > > How are any of the sleeping call encoded in the type system? I assume There's no easy way, something might work is introducing effect system [1] into Rust, but that's very complicated and may take years. When there's no easy way to encode something in the type system, it's usually the time that unsafe comes to happen, an unsafe function can have a requirement that cannot be easily detected by compilers, and via unsafe block and safety comments, programmers provide the reasons why these requirements are fulfilled. > any use of a mutex lock, sleep, wait for completion, etc are not all > marked as unsafe? There is some sort of wrapper around them? Why not They are marked as safe because of the klint extension of safe Rust rule I mentioned. > just extend that wrapper to might_sleep(). > > > For Rust code, without klint rule, might_sleep() needs to > > be unsafe. So we have two options for might_sleep(). > > > > * Since we rely on klint for atomic context detection, we can mark the > > trivial wrapper (as what Alice presented in the other email) as safe, > > but we need to begin to add klint annotation for that function, unless > > Gary finds a smart way to auto-annotate functions. > > Are there klint annotations for all sleeping functions? > Not yet, klint is still WIP. But we generally agree that atomic context violations should be detected by klint (instead of making sleep functions unsafe or using type system to encode sleep functions). > > * Instead of might_sleep(), we provide the wrapper of __might_sleep(), > > since it doesn't have might_resched() in it, it should be safe. And > > all we care about here is the debugging rather than voluntary context > > switch. (Besides I think preempt=volunatry is eventually going to be > > gone because of PREEMPT_AUTO [1], if that happens I think the > > might_resched() might be dropped entirely). > > __might_sleep() might be safe, but your code is still broken and going > to UAF at some point. Don't you want that UAF to happen more reliably > and faster so you can find the issue? That would be the advantage of > might_sleep() over __might_sleep(). > Could you give me an example that might_sleep() can detect a bug while __might_sleep() cannot? IIUC, __might_sleep() is the core of atomic context detection in might_sleep(), so when CONFIG_DEBUG_ATOMIC_SLEEP=y, __might_sleep() should detect all bugs that might_sleep() would detect. Or you are talking about detecting even when CONFIG_DEBUG_ATOMIC_SLEEP=n? [1]: https://en.wikipedia.org/wiki/Effect_system Regards, Boqun > Andrew
On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > As far as i see, might_sleep() will cause UAF where there is going to > be a UAF anyway. If you are using it correctly, it does not cause UAF. This already implies that it is an unsafe function (in general, i.e. modulo klint, or a way to force the user to have to write `unsafe` somewhere else, or what I call ASHes -- "acknowledged soundness holes"). If we consider as safe functions that, if used correctly, do not cause UB, then all functions would be safe. Cheers, Miguel
On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote: > On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > As far as i see, might_sleep() will cause UAF where there is going to > > be a UAF anyway. If you are using it correctly, it does not cause UAF. > > This already implies that it is an unsafe function (in general, i.e. > modulo klint, or a way to force the user to have to write `unsafe` > somewhere else, or what I call ASHes -- "acknowledged soundness > holes"). > > If we consider as safe functions that, if used correctly, do not cause > UB, then all functions would be safe. From what i hear, klint is still WIP. So we have to accept there will be bad code out there, which will UAF. We want to find such bad code, and the easiest way to find it at the moment is to make it UAF as fast as possible. might_sleep() does that, __might_sleep() does not, and using neither is the slowest way. Andrew
On Tue, Oct 08, 2024 at 07:16:42PM +0200, Andrew Lunn wrote: > On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote: > > On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > As far as i see, might_sleep() will cause UAF where there is going to > > > be a UAF anyway. If you are using it correctly, it does not cause UAF. > > > > This already implies that it is an unsafe function (in general, i.e. > > modulo klint, or a way to force the user to have to write `unsafe` > > somewhere else, or what I call ASHes -- "acknowledged soundness > > holes"). > > > > If we consider as safe functions that, if used correctly, do not cause > > UB, then all functions would be safe. > > From what i hear, klint is still WIP. So we have to accept there will > be bad code out there, which will UAF. We want to find such bad code, If you don't believe in klint, then we need to mark might_sleep() as unsafe, as I already explain a million times, might_sleep() is unsafe without the klint compile time check. You have to accept that an unsafe function should really be marked as unsafe. And yes, in this way, all sleep functions would be marked as unsafe as well (or we could mark all preemption disable function as unsafe), but still an unsafe function is unsafe. Again, as Miguel mentioned, we can only mark might_sleep() because sleep in atomic context is an ASH, not because it's really safe. > and the easiest way to find it at the moment is to make it UAF as > fast as possible. might_sleep() does that, __might_sleep() does not, > and using neither is the slowest way. > might_sleep() is useful because it checks preemption count and task state, which is provided by __might_sleep() as well. I don't think causing UAF helps we detect atomic context violation faster than what __might_sleep() already have. Again, could you provide an example that help me understand your reasoning here? Regards, Boqun > Andrew
On Tue, Oct 08, 2024 at 02:53:56PM -0700, Boqun Feng wrote: > On Tue, Oct 08, 2024 at 07:16:42PM +0200, Andrew Lunn wrote: > > On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote: > > > On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > As far as i see, might_sleep() will cause UAF where there is going to > > > > be a UAF anyway. If you are using it correctly, it does not cause UAF. > > > > > > This already implies that it is an unsafe function (in general, i.e. > > > modulo klint, or a way to force the user to have to write `unsafe` > > > somewhere else, or what I call ASHes -- "acknowledged soundness > > > holes"). > > > > > > If we consider as safe functions that, if used correctly, do not cause > > > UB, then all functions would be safe. > > > > From what i hear, klint is still WIP. So we have to accept there will > > be bad code out there, which will UAF. We want to find such bad code, > > If you don't believe in klint, then we need to mark might_sleep() as > unsafe, as I already explain a million times, might_sleep() is unsafe > without the klint compile time check. You have to accept that an unsafe > function should really be marked as unsafe. And yes, in this way, all > sleep functions would be marked as unsafe as well (or we could mark all > preemption disable function as unsafe), but still an unsafe function is > unsafe. > > Again, as Miguel mentioned, we can only mark might_sleep() because sleep > in atomic context is an ASH, not because it's really safe. > > > and the easiest way to find it at the moment is to make it UAF as > > fast as possible. might_sleep() does that, __might_sleep() does not, > > and using neither is the slowest way. > > > > might_sleep() is useful because it checks preemption count and task > state, which is provided by __might_sleep() as well. I don't think > causing UAF helps we detect atomic context violation faster than what > __might_sleep() already have. Again, could you provide an example that > help me understand your reasoning here? > Another advantage of __might_sleep() is that it's already an exported symbol, so we don't need to introduce a rust helper. Regards, Boqun > Regards, > Boqun > > > Andrew
On Tue, Oct 08, 2024 at 02:53:56PM -0700, Boqun Feng wrote: > On Tue, Oct 08, 2024 at 07:16:42PM +0200, Andrew Lunn wrote: > > On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote: > > > On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > As far as i see, might_sleep() will cause UAF where there is going to > > > > be a UAF anyway. If you are using it correctly, it does not cause UAF. > > > > > > This already implies that it is an unsafe function (in general, i.e. > > > modulo klint, or a way to force the user to have to write `unsafe` > > > somewhere else, or what I call ASHes -- "acknowledged soundness > > > holes"). > > > > > > If we consider as safe functions that, if used correctly, do not cause > > > UB, then all functions would be safe. > > > > From what i hear, klint is still WIP. So we have to accept there will > > be bad code out there, which will UAF. We want to find such bad code, > > If you don't believe in klint I did not say that. It is WIP, and without it i assume nothing is detecting at compile time that the code is broken. Hence we need to find the problem at runtime, which is what might_sleep() is all about. > might_sleep() is useful because it checks preemption count and task > state, which is provided by __might_sleep() as well. I don't think > causing UAF helps we detect atomic context violation faster than what > __might_sleep() already have. Again, could you provide an example that > help me understand your reasoning here? > > while (1) { > > <reader> <updater> > > rcu_read_lock(); > > p = rcu_dereference(gp); > > mutex_lock(&lock) > > a = READ_ONCE(p->a); > > mutex_unlock(&lock) > > rcu_read_unlock(); > > } The mutex lock is likely to be uncontested, so you don't sleep, and so don't trigger the UAF. The code is clearly broken, but you survive. Until the lock is contested, you do sleep, RCU falls apart, resulting in a UAF. Now if you used might_sleep(), every time you go around that loop you do some of the same processing as actually sleeping, so are much more likely to trigger the UAF. might_sleep() as you pointed out, is also active when CONFIG_DEBUG_ATOMIC_SLEEP is false. Thus it is also going to trigger the broken code to UAF faster. And i expect a lot of testing is done without CONFIG_DEBUG_ATOMIC_SLEEP and CONFIG_PROVE_LOCKING. Once klint is completed, and detects all these problems at compile time, we can then discard all this might_sleep stuff. But until then, the faster code explodes, the more likely it is going to be quickly and cheaply fixed. Andrew
On Wed, Oct 09, 2024 at 12:26:00AM +0200, Andrew Lunn wrote: > On Tue, Oct 08, 2024 at 02:53:56PM -0700, Boqun Feng wrote: > > On Tue, Oct 08, 2024 at 07:16:42PM +0200, Andrew Lunn wrote: > > > On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote: > > > > On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > > > As far as i see, might_sleep() will cause UAF where there is going to > > > > > be a UAF anyway. If you are using it correctly, it does not cause UAF. > > > > > > > > This already implies that it is an unsafe function (in general, i.e. > > > > modulo klint, or a way to force the user to have to write `unsafe` > > > > somewhere else, or what I call ASHes -- "acknowledged soundness > > > > holes"). > > > > > > > > If we consider as safe functions that, if used correctly, do not cause > > > > UB, then all functions would be safe. > > > > > > From what i hear, klint is still WIP. So we have to accept there will > > > be bad code out there, which will UAF. We want to find such bad code, > > > > If you don't believe in klint > > I did not say that. It is WIP, and without it i assume nothing is > detecting at compile time that the code is broken. Hence we need to > find the problem at runtime, which is what might_sleep() is all about. > > > might_sleep() is useful because it checks preemption count and task > > state, which is provided by __might_sleep() as well. I don't think > > causing UAF helps we detect atomic context violation faster than what > > __might_sleep() already have. Again, could you provide an example that > > help me understand your reasoning here? > > > > while (1) { > > > <reader> <updater> > > > rcu_read_lock(); > > > p = rcu_dereference(gp); > > > mutex_lock(&lock) > > > a = READ_ONCE(p->a); > > > mutex_unlock(&lock) > > > rcu_read_unlock(); > > > } > > The mutex lock is likely to be uncontested, so you don't sleep, and so > don't trigger the UAF. The code is clearly broken, but you survive. > Until the lock is contested, you do sleep, RCU falls apart, resulting > in a UAF. > > Now if you used might_sleep(), every time you go around that loop you > do some of the same processing as actually sleeping, so are much more > likely to trigger the UAF. > > might_sleep() as you pointed out, is also active when > CONFIG_DEBUG_ATOMIC_SLEEP is false. Thus it is also going to trigger > the broken code to UAF faster. And i expect a lot of testing is done > without CONFIG_DEBUG_ATOMIC_SLEEP and CONFIG_PROVE_LOCKING. > Hmm.. but that means we need to quickly detect UAF and track down to the source, right? In a build without CONFIG_DEBUG_ATOMIC_SLEEP and CONFIG_PROVE_LOCKING, may I assume memory sanitizer is also not available? Then how do we detect UAF relatively quickly? Or memory sanitizer is in fact relatively cheap, so it can still be enabled, what's the configuration of netdev CI/testing? Regards, Boqun > Once klint is completed, and detects all these problems at compile > time, we can then discard all this might_sleep stuff. But until then, > the faster code explodes, the more likely it is going to be quickly > and cheaply fixed. > > Andrew
On Sat, 5 Oct 2024 15:22:23 -0700 Boqun Feng <boqun.feng@gmail.com> wrote: > On Sat, Oct 05, 2024 at 08:32:01PM +0200, Andrew Lunn wrote: >> > might_sleep() is called via a wrapper so the __FILE__ and __LINE__ >> > debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we >> > expect; the wrapper instead of the caller. >> >> So not very useful. All we know is that somewhere in Rust something is >> sleeping in atomic context. Is it possible to do better? Does __FILE__ >> and __LINE__ exist in Rust? >> > > Sure, you can use: > > https://doc.rust-lang.org/core/macro.line.html To get the proper file name and line number with those macros, we need to use __might_sleep() instead of might_sleep(). We could also call to might_resched() there to emulate might_sleep() but fsleep() works without it so I call only __might_sleep() in the next version.
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c index c274546bcf78..f9569ff1717e 100644 --- a/rust/helpers/helpers.c +++ b/rust/helpers/helpers.c @@ -12,6 +12,7 @@ #include "build_assert.c" #include "build_bug.c" #include "err.c" +#include "kernel.c" #include "kunit.c" #include "mutex.c" #include "page.c" diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c new file mode 100644 index 000000000000..5b9614974c76 --- /dev/null +++ b/rust/helpers/kernel.c @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/kernel.h> + +void rust_helper_cpu_relax(void) +{ + cpu_relax(); +} + +void rust_helper_might_sleep(void) +{ + might_sleep(); +} diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 6f1587a2524e..d571b9587ed6 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -58,6 +58,7 @@ macro_rules! declare_err { declare_err!(EPIPE, "Broken pipe."); declare_err!(EDOM, "Math argument out of domain of func."); declare_err!(ERANGE, "Math result not representable."); + declare_err!(ETIMEDOUT, "Connection timed out."); declare_err!(ERESTARTSYS, "Restart the system call."); declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted."); declare_err!(ERESTARTNOHAND, "Restart if no handler."); diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs new file mode 100644 index 000000000000..033f3c4e4adf --- /dev/null +++ b/rust/kernel/io.rs @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Input and Output. + +pub mod poll; diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs new file mode 100644 index 000000000000..d248a16a7158 --- /dev/null +++ b/rust/kernel/io/poll.rs @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! IO polling. +//! +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h). + +use crate::{ + bindings, + error::{code::*, Result}, + time::{fsleep, Delta, Ktime}, +}; + +/// Polls periodically until a condition is met or a timeout is reached. +/// +/// `op` is called repeatedly until `cond` returns `true` or the timeout is +/// reached. The return value of `op` is passed to `cond`. +/// +/// `sleep_delta` is the duration to sleep between calls to `op`. +/// If `sleep_delta` is less than one microsecond, the function will busy-wait. +/// +/// `timeout_delta` is the maximum time to wait for `cond` to return `true`. +/// +/// If `sleep_before_read` is `true`, the function will sleep for `sleep_delta` +/// first. +/// +/// This function can only be used in a nonatomic context. +pub fn read_poll_timeout<Op, Cond, T: Copy>( + mut op: Op, + cond: Cond, + sleep_delta: Delta, + timeout_delta: Delta, + sleep_before_read: bool, +) -> Result<T> +where + Op: FnMut() -> Result<T>, + Cond: Fn(T) -> bool, +{ + let timeout = Ktime::ktime_get() + timeout_delta; + let sleep = sleep_delta.as_micros() != 0; + + if sleep { + // SAFETY: FFI call. + unsafe { bindings::might_sleep() } + } + + if sleep_before_read && sleep { + fsleep(sleep_delta); + } + + let val = loop { + let val = op()?; + if cond(val) { + break val; + } + if !timeout_delta.is_zero() && Ktime::ktime_get() > timeout { + break op()?; + } + if sleep { + fsleep(sleep_delta); + } + // SAFETY: FFI call. + unsafe { bindings::cpu_relax() } + }; + + if cond(val) { + Ok(val) + } else { + Err(ETIMEDOUT) + } +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 22a3bfa5a9e9..7b6888723fc4 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -35,6 +35,7 @@ #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)] pub mod firmware; pub mod init; +pub mod io; pub mod ioctl; #[cfg(CONFIG_KUNIT)] pub mod kunit;
Add read_poll_timeout function which polls periodically until a condition is met or a timeout is reached. 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. might_sleep() is called via a wrapper so the __FILE__ and __LINE__ debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we expect; the wrapper instead of the caller. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/helpers/helpers.c | 1 + rust/helpers/kernel.c | 13 ++++++++ rust/kernel/error.rs | 1 + rust/kernel/io.rs | 5 +++ rust/kernel/io/poll.rs | 70 ++++++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 6 files changed, 91 insertions(+) create mode 100644 rust/helpers/kernel.c create mode 100644 rust/kernel/io.rs create mode 100644 rust/kernel/io/poll.rs