Re: [PATCH v1 2/2] rust: hrtimer: Use hrtimer_forward_safe_from_callback()

[Andreas Hindborg]

"FUJITA Tomonori" <tomo@xxxxxxxxxxxx> writes:

> From: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
>
> Safe Rust code can start a timer while its callback runs. A timer
> started through Arc<T> or Pin<&T> can be started again from another
> clone or copy of the pointer. HrTimerCallbackContext::forward() calls
> hrtimer_forward(), which writes the expiry without the base lock, so
> the write can race with the concurrent hrtimer_start().
>
> Call hrtimer_forward_safe_from_callback() instead. It does the update
> with the base lock held. If the timer is already queued by a concurrent
> start, the expiry is left alone and 0 is returned.
>
> HrTimer::forward() keeps calling hrtimer_forward(). It takes
> Pin<&mut Self>, so no other reference to the timer exists and no
> concurrent start is possible. Narrow the safety requirement of
> raw_forward() to exclusive access, since the callback context no longer
> uses it.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>

This looks good to me.

Reviewed-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>

While reading this, I thought we should expand the safety requirement
for the handle trait, because even though handles keep the callback
target alive, they do not own the target. The armed timer does. And so a
(future) &mut method on the handle is not allowed to mutably access the
target. This implicit today, but I thought maybe we should make it
explicit.

What do you think:

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index ab7c568b8855..84f45a7a3a17 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -771,6 +771,14 @@ fn run(
 /// is running. If the timer handler is running when the handle is dropped, the
 /// drop method must wait for the handler to return before returning.
 ///
+/// While the timer is armed, the callback target is borrowed by the timer: the
+/// timer callback can create a reference to the target at any time, and for
+/// some pointer types that reference is exclusive. Implementers must not create
+/// a reference to the callback target that would conflict with the reference
+/// that [`RawHrTimerCallback::run`] creates for the pointer type in question.
+/// Deriving raw pointers from the target, as the [`Self::cancel`]
+/// implementations do, always satisfies this.
+///
 /// Note: One way to satisfy the safety requirement is to call `Self::cancel` in
 /// the drop implementation for `Self.`
 pub unsafe trait HrTimerHandle {
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
index 46ccff9e0024..eef4900cea7a 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -20,8 +20,12 @@ pub struct ArcHrTimerHandle<T>
     pub(crate) inner: Arc<T>,
 }
 
-// SAFETY: We implement drop below, and we cancel the timer in the drop
-// implementation.
+// SAFETY:
+//  - We implement drop below, and we cancel the timer in the drop
+//    implementation.
+//  - The callback for this pointer type receives an `ArcBorrow<T>`, so the
+//    timer only ever takes shared access to the callback target. References
+//    derived from `self.inner` are shared as well, so they cannot conflict.
 unsafe impl<T> HrTimerHandle for ArcHrTimerHandle<T>
 where
     T: HasHrTimer<T>,
diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
index 5fd374fdc480..3498c78f99f4 100644
--- a/rust/kernel/time/hrtimer/pin.rs
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -19,8 +19,13 @@ pub struct PinHrTimerHandle<'a, T>
     pub(crate) inner: Pin<&'a T>,
 }
 
-// SAFETY: We cancel the timer when the handle is dropped. The implementation of
-// the `cancel` method will block if the timer handler is running.
+// SAFETY:
+//  - We cancel the timer when the handle is dropped. The implementation of the
+//    `cancel` method will block if the timer handler is running.
+//  - The callback for this pointer type receives a `Pin<&T>`, so the timer only
+//    ever takes shared access to the callback target. The `Pin<&T>` held by the
+//    handle and references derived from it are shared as well, so they cannot
+//    conflict.
 unsafe impl<'a, T> HrTimerHandle for PinHrTimerHandle<'a, T>
 where
     T: HasHrTimer<T>,
diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
index 2bba3c41d6e9..7a54889173ba 100644
--- a/rust/kernel/time/hrtimer/pin_mut.rs
+++ b/rust/kernel/time/hrtimer/pin_mut.rs
@@ -17,8 +17,12 @@ pub struct PinMutHrTimerHandle<'a, T>
     _p: PhantomData<&'a mut T>,
 }
 
-// SAFETY: We cancel the timer when the handle is dropped. The implementation of
-// the `cancel` method will block if the timer handler is running.
+// SAFETY:
+//  - We cancel the timer when the handle is dropped. The implementation of the
+//    `cancel` method will block if the timer handler is running.
+//  - `cancel` only derives raw pointers from `self.inner`, so it does not
+//    create a reference that could conflict with the `Pin<&mut T>` that the
+//    callback creates.
 unsafe impl<'a, T> HrTimerHandle for PinMutHrTimerHandle<'a, T>
 where
     T: HasHrTimer<T>,
diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
index 399ad7677043..c13a24e19550 100644
--- a/rust/kernel/time/hrtimer/tbox.rs
+++ b/rust/kernel/time/hrtimer/tbox.rs
@@ -26,8 +26,14 @@ pub struct BoxHrTimerHandle<T, A>
     _p: core::marker::PhantomData<A>,
 }
 
-// SAFETY: We implement drop below, and we cancel the timer in the drop
-// implementation.
+// SAFETY:
+//  - We implement drop below, and we cancel the timer in the drop
+//    implementation.
+//  - `cancel` only derives raw pointers from `self.inner`, so it does not
+//    create a reference that could conflict with the `Pin<&mut T>` that the
+//    callback creates. The drop implementation reconstructs the `Box` only
+//    after `cancel` has returned, and as it owns the `T`, no other pointer
+//    exists that could arm the timer again.
 unsafe impl<T, A> HrTimerHandle for BoxHrTimerHandle<T, A>
 where
     T: HasHrTimer<T>,
---

Best regards,
Andreas Hindborg