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

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(+)

We now have a safe wrapper for the foreign function synchronize_rcu().

Use it in revocable.rs.

Signed-off-by Philipp Stanner <[email protected]>

---
 rust/kernel/revocable.rs | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

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

---
 rust/kernel/sync/poll.rs | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

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

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

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

index a32bef6e490b..0d438ef31766 100644

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

index a32bef6e490b..0d438ef31766 100644
@@ -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`].

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.

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.

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

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.

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

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)