Add and use abstraction for synchronize_rcu()
v2 cover Add and use abstraction for synchronize_rcu()
Changes since v1:
- Vertically format imports. (Onur)
- Make the new function #[inline]. (Alice)
- Add Alice's R-b.
Philipp Stanner (3):
rust: sync: Add abstraction for synchronize_rcu()
rust: revocable: Use safe synchronize_rcu() abstraction
rust: sync: Use safe synchronize_rcu() abstraction in poll
rust/kernel/revocable.rs | 9 ++++++--- rust/kernel/sync/poll.rs | 10 ++++++---- rust/kernel/sync/rcu.rs | 9 +++++++++ 3 files changed, 21 insertions(+), 7 deletions(-)
base-commit: 43a393185e33e573a374c1d4f7ddf6481484ef8d
Signature · 1 line
2.54.0
PATCH v2 1/3 rust: sync: Add abstraction for synchronize_rcu()
synchronize_rcu() is a frequently used C function which is always safe to be called.
Add a safe abstraction for synchronize_rcu().
--- rust/kernel/sync/rcu.rs | 9 +++++++++ 1 file changed, 9 insertions(+)
rust/kernel/sync/rcu.rs +9 -0
@@ -50,3 +50,12 @@ fn drop(&mut self) {pub fn read_lock () ->Guard {Guard ::new () } + +/// Wait for one RCU grace period. +/// +/// You typically do this to wait for everyone holding a [`Guard`]. +#[inline] +pub fn synchronize_rcu () { +// SAFETY: `synchronize_rcu()` is always safe to be called. It just waits for a grace period. +unsafe { bindings::synchronize_rcu () }; +}
Signature · 1 line
2.54.0
Re: [PATCH v2 1/3] rust: sync: Add abstraction for synchronize_rcu()
Quoted 6 lines · Philipp Stanner
synchronize_rcu()is a frequently used C function which is always safe to be called.Add a safe abstraction for
synchronize_rcu().Signed-off-by Philipp Stanner <[email protected]>
Quoted 3 lines
--- rust/kernel/sync/rcu.rs | 9 +++++++++ 1 file changed, 9 insertions(+)
Danilo Krummrich
reviewerRe: [PATCH v2 1/3] rust: sync: Add abstraction for synchronize_rcu()
In reply to Philipp Stannerrust/kernel/sync/rcu.rs fn drop(&mut self) { · line 56
Quoted 3 lines · Philipp Stanner
rust/kernel/sync/rcu.rs +3
-0+/// Wait for one RCU grace period. +/// +/// You typically do this to wait for everyone holding a [`Guard`].
NIT: "typically" reads a bit as if there were other reasons to call synchronize_rcu() than to wait for all concurrent RCU read side critical sections.
Also, while it's implicit, it might still be worth to explicitly call out that this means concurrently held Guard objects (concurrent read side critical sections), i.e. subsequent read side critical sections may still run concurrently.
Quoted 7 lines
rust/kernel/sync/rcu.rs +5
-0+#[inline] +pub fn synchronize_rcu () { +// SAFETY: `synchronize_rcu()` is always safe to be called. It just waits for a grace period. +unsafe { bindings::synchronize_rcu () }; +}Signature · 1 line
2.54.0
Re: [PATCH v2 1/3] rust: sync: Add abstraction for synchronize_rcu()
Quoted 8 lines · Danilo Krummrich
rust/kernel/sync/rcu.rs fn drop(&mut self) { · line 56
Quoted 3 lines · Philipp Stanner
rust/kernel/sync/rcu.rs +3
-0+/// Wait for one RCU grace period. +/// +/// You typically do this to wait for everyone holding a [`Guard`]. NIT: "typically" reads a bit as if there were other reasons to call
synchronize_rcu()than to wait for all concurrent RCU read side critical sections.
The reason I wrote "typically" is because my mind had the potential future use-case of ours prefetched where we might have to do a synchronize_rcu() to wait for a C backend to be done with something, where no one really holds a Rust Guard (though of course the read lock).
Quoted 5 lines
Also, while it's implicit, it might still be worth to explicitly call out that this means concurrently held Guard objects (concurrent read side critical sections), i.e. subsequent read side critical sections may still run concurrently.
That's quite generic RCU knowledge IMO. I'm not sure to what degree one wants to document RCU in general at this new function here, vs just the Rust API.
Rewording the documentation is fine by me, but since we're in a nitty domain here I would then ask you to provide a few draft sentences that would satisfy your basic requirements.
P.
Quoted 8 lines
Quoted 8 lines
rust/kernel/sync/rcu.rs +5
-0+#[inline] +pub fn synchronize_rcu () { + // SAFETY: `synchronize_rcu()` is always safe to be called. It just waits for a grace period. + unsafe { bindings::synchronize_rcu() }; +}Signature · 1 line
2.54.0
Danilo Krummrich
reviewerRe: [PATCH v2 1/3] rust: sync: Add abstraction for synchronize_rcu()
Quoted 5 lines · Philipp Stanner
The reason I wrote "typically" is because my mind had the potential future use-case of ours prefetched where we might have to do a
synchronize_rcu()to wait for a C backend to be done with something, where no one really holds a RustGuard(though of course the read lock).
In that case I'd just say "read side critical section" and then mention the rcu::Guard type as one way to end up in such.
In comparison with other synchronization primitives, RCU is a global mechanism, so only talking about the rcu::Guard specifically might indeed be misleading.
Quoted 8 lines
Quoted 4 lines
Also, while it's implicit, it might still be worth to explicitly call out that this means concurrently held Guard objects (concurrent read side critical sections), i.e. subsequent read side critical sections may still run concurrently.
That's quite generic RCU knowledge IMO. I'm not sure to what degree one wants to document RCU in general at this new function here, vs just the Rust API.
One or two precise sentences should be enough; no need to document RCU in general.
Quoted 3 lines
Rewording the documentation is fine by me, but since we're in a nitty domain here I would then ask you to provide a few draft sentences that would satisfy your basic requirements.
Should be as simple as adding "in progress", e.g. considering the above:
This waits for all RCU read-side critical sections already in progress, including those established by an [rcu::Guard].
This does not prevent new read-side critical sections from starting; those may begin and run while this call blocks.
Re: [PATCH v2 1/3] rust: sync: Add abstraction for synchronize_rcu()
In reply to Philipp StannerQuoted 15 lines · Philipp Stanner
synchronize_rcu()is a frequently used C function which is always safe to be called.Add a safe abstraction for
synchronize_rcu().Signed-off-by Philipp Stanner <[email protected]>--- rust/kernel/sync/rcu.rs | 9 +++++++++ 1 file changed, 9 insertions(+)rust/kernel/sync/rcu.rs +0
-0
rust/kernel/sync/rcu.rs fn drop(&mut self) { · line 59
Quoted 10 lines
rust/kernel/sync/rcu.rs +7
-0@@ -50,3 +50,12 @@ fn drop(&mut self) {pub fn read_lock () ->Guard {Guard ::new () } + +/// Wait for one RCU grace period. +/// +/// You typically do this to wait for everyone holding a [`Guard`]. +#[inline] +pub fn synchronize_rcu () { +// SAFETY: `synchronize_rcu()` is always safe to be called. It just waits for a grace period.
Commething randomly here (I know as much rust as the next not-knowing-rust-guy, sadly), but synchronize_rcu() is not always safe to be called. You cannot call it if you have the rcu read lock, or cannot sleep for any reason. I don't know if you can encode these in the rust type system though.
Signature · 1 line
Pedro
Re: [PATCH v2 1/3] rust: sync: Add abstraction for synchronize_rcu()
Quoted 23 lines · Pedro Falcato
Quoted 22 lines · Philipp Stanner
synchronize_rcu()is a frequently used C function which is always safe to be called.Add a safe abstraction for
synchronize_rcu().Signed-off-by Philipp Stanner <[email protected]>---
rust/kernel/sync/rcu.rs | 9 +++++++++1 file changed, 9 insertions(+)rust/kernel/sync/rcu.rs +4
-0@@ -50,3 +50,12 @@ fn drop(&mut self) { pub fn read_lock() -> Guard { Guard::new() } + +/// Wait for one RCU grace period. +/// +/// You typically do this to wait for everyone holding a [`Guard`].
Quoted 8 lines
rust/kernel/sync/rcu.rs fn drop(&mut self) { · line 59
Quoted 3 lines
rust/kernel/sync/rcu.rs +3
-0+#[inline] +pub fn synchronize_rcu () { + // SAFETY: `synchronize_rcu()` is always safe to be called. It just waits for a grace period.Commething randomly here (I know as much rust as the next not-knowing-rust-guy, sadly), but
synchronize_rcu()is not always safe to be called. You cannot call it if you have the rcu read lock, or cannot sleep for any reason. I don't know if you can encode these in the rust type system though.
Preventing illegal actions in atomic context seems very difficult to achieve in Rust and AFAIK the only viable path forward currently is to have klint check for rule violations. Gary is working on that.
But it would be interesting to know more about how in general Rust's unsafe comments are related to problems beyond UAF issues, and to what degree we want to document context requirements.
C documents functions like synchronize_rcu() in much detail, but its documentation trusts on the readers general familiarity with RCU, assuming no one would come up with the idea of calling synchronize_rcu() in an atomic context.
P.
Re: [PATCH v2 1/3] rust: sync: Add abstraction for synchronize_rcu()
Quoted 4 lines · Philipp Stanner
But it would be interesting to know more about how in general Rust's unsafe comments are related to problems beyond UAF issues, and to what degree we want to document context requirements.
I am confused by the UAF there. Did you mean UB?
Rust's unsafe is about way more than just use-after-free -- it is about all potential undefined behavior.
At the same time, it is not about merely "dangerous" things.
If you cannot possibly cause UB, then it is not in scope. Otherwise, it is very much in scope and the safety preconditions/requirements need to be clearly documented (# Safety) or justified (// SAFETY:).
Now, sometimes it may not make a lot of sense to duplicate a ton of information, so sometimes we lift text to the Rust module docs and refer to it; and sometimes it may also make more sense to refer to external docs. One way or another, the goal is to document the requirements and what is going on as clearly as possible.
Cheers,
Miguel
Re: [PATCH v2 1/3] rust: sync: Add abstraction for synchronize_rcu()
Quoted 18 lines · Miguel Ojeda
Quoted 4 lines · Philipp Stanner
But it would be interesting to know more about how in general Rust's unsafe comments are related to problems beyond UAF issues, and to what degree we want to document context requirements.
I am confused by the UAF there. Did you mean UB?
Rust's
unsafeis about way more than just use-after-free -- it is about all potential undefined behavior.At the same time, it is not about merely "dangerous" things.
If you cannot possibly cause UB, then it is not in scope. Otherwise, it is very much in scope and the safety preconditions/requirements need to be clearly documented (
# Safety) or justified (// SAFETY:).
Quoted 5 lines
Now, sometimes it may not make a lot of sense to duplicate a ton of information, so sometimes we lift text to the Rust module docs and refer to it; and sometimes it may also make more sense to refer to external docs. One way or another, the goal is to document the requirements and what is going on as clearly as possible.
Well, commonly, deadlock is not regarded to be UB.
For RCU the question really is to what extend one wants to have it. The overall robustness requirement here is definitely for the Rust function rcu::synchronize_rcu(), since the API caller is the one in charge of the execution context.
If all potential failures one can cause by calling that function at the wrong place were regarded to be undefined behavior, then a synchronize_rcu() Rust function would have to be an unsafe function always, making a wrapper pointless.
Similarly, Rust's drop() implementations might be potentially "unsafe" with a hyper-strict definition (note that I'm unsure whether calling synchronize_rcu() in atomic context is actually even defined behavior; I think it is. I'm just brainstorming here)
I think briefly documenting the context requirement is fine, but from a consistency and pragmatism perspective I would not make that a formal safety requirement.
P.
Quoted 3 lines
Cheers,
Miguel
Danilo Krummrich
reviewerRe: [PATCH v2 1/3] rust: sync: Add abstraction for synchronize_rcu()
Quoted 3 lines · Philipp Stanner
with a hyper-strict definition (note that I'm unsure whether calling
synchronize_rcu()in atomic context is actually even defined behavior; I think it is. I'm just brainstorming here)
From the Rust side of things this is well defined; please see [1] and [2].
However, given the kernel context, things such as deadlocks have more consequences than in userspace. Which is why it is still desirable to catch them (e.g. with tools like klint).
[1] https://doc.rust-lang.org/reference/behavior-considered-undefined.html
[2] https://doc.rust-lang.org/reference/behavior-not-considered-unsafe.html
PATCH v2 2/3 rust: revocable: Use safe synchronize_rcu() abstraction
In reply to Philipp StannerWe now have a safe wrapper for the foreign function synchronize_rcu().
Use it in revocable.rs.
--- rust/kernel/revocable.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
rust/kernel/revocable.rs +6 -3
@@ -7,7 +7,11 @@use pin_init::Wrapper ; -use crate ::{bindings, prelude::*, sync::rcu, types::Opaque }; +use crate ::{ + prelude::*, + sync::rcu, + types::Opaque ,// +};use core::{ marker::PhantomData , ops::Deref ,
@@ -161,8 +165,7 @@ unsafe fn revoke_internal<const SYNC: bool>(&self) -> bool {if revoke {if SYNC { -// SAFETY: Just an FFI call, there are no further requirements. -unsafe { bindings::synchronize_rcu () }; + rcu::synchronize_rcu (); }// SAFETY: We know `self.data` is valid because only one CPU can succeed the
Signature · 1 line
2.54.0
Re: [PATCH v2 2/3] rust: revocable: Use safe synchronize_rcu() abstraction
Quoted 5 lines · Philipp Stanner
We now have a safe wrapper for the foreign function
synchronize_rcu().Use it in
revocable.rs.Signed-off-by Philipp Stanner <[email protected]>
Quoted 3 lines
--- rust/kernel/revocable.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
PATCH v2 3/3 rust: sync: Use safe synchronize_rcu() abstraction in poll
In reply to Philipp StannerWe now have a safe wrapper for the foreign function synchronize_rcu().
Use it in poll.rs.
--- rust/kernel/sync/poll.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
rust/kernel/sync/poll.rs +6 -4
@@ -8,7 +8,11 @@ bindings, fs::File, prelude::*, - sync::{CondVar, LockClassKey}, + sync::{ +CondVar , +LockClassKey , + rcu::synchronize_rcu, // + }, // };use core::{marker::PhantomData , ops::Deref };
@@ -99,8 +103,6 @@ fn drop(self: Pin<&mut Self>) {unsafe { bindings::__wake_up_pollfree (self.inner.wait_queue_head.get ()) };// Wait for epoll items to be properly removed. -// -// SAFETY: Just an FFI call. -unsafe { bindings::synchronize_rcu () }; +synchronize_rcu (); } }
Signature · 1 line
2.54.0
Re: [PATCH v2 3/3] rust: sync: Use safe synchronize_rcu() abstraction in poll
Quoted 6 lines · Philipp Stanner
We now have a safe wrapper for the foreign function
synchronize_rcu().Use it in
poll.rs.Signed-off-by Philipp Stanner <[email protected]>Reviewed-by Alice Ryhl <[email protected]>
Quoted 3 lines
--- rust/kernel/sync/poll.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Quoted 10 lines · On Mon, 22 Jun 2026 19:32:48 +0200 Philipp Stanner <[email protected]> wrote:
Changes since v1:
- Vertically format imports. (Onur)
- Make the new function #[inline]. (Alice)
- Add Alice's R-b.Philipp Stanner (3):
rust: sync: Add abstraction forsynchronize_rcu()
rust: revocable: Use safesynchronize_rcu()abstraction
rust: sync: Use safesynchronize_rcu()abstraction in poll
Quoted 10 lines
rust/kernel/revocable.rs | 9 ++++++--- rust/kernel/sync/poll.rs | 10 ++++++---- rust/kernel/sync/rcu.rs | 9 +++++++++ 3 files changed, 21 insertions(+), 7 deletions(-)base-commit:
43a393185e33e573a374c1d4f7ddf6481484ef8dSignature · 1 line
2.54.0
For the whole series:
Quoted 3 lines · Philipp Stanner
rust: sync: Add abstraction for
synchronize_rcu()
rust: revocable: Use safesynchronize_rcu()abstraction
rust: sync: Use safesynchronize_rcu()abstraction in poll