[PATCH v1 0/2] hrtimer: Add hrtimer_forward_safe_from_callback()

[PATCH v1 0/2] hrtimer: Add hrtimer_forward_safe_from_callback()

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

Safe Rust code can arm a timer while its callback is running, so the
Rust abstraction cannot offer hrtimer_forward() from the callback as a
safe API.

Thomas suggested a helper that does the check and the update with the
base lock held [1]. Patch 1 adds it. Patch 2 makes the Rust callback
context use it.

The helper is not Rust specific. As Thomas noted, it can be used for
i915_pmu too. null_blk can use it as well. I can send those
conversions after the helper is merged.

[1] https://lore.kernel.org/all/877bk3ixp5.ffs@fw13/


FUJITA Tomonori (2):
  hrtimer: Add hrtimer_forward_safe_from_callback()
  rust: hrtimer: Use hrtimer_forward_safe_from_callback()

 include/linux/hrtimer.h     |  4 ++++
 kernel/time/hrtimer.c       | 30 ++++++++++++++++++++++++++++++
 rust/kernel/time/hrtimer.rs | 25 ++++++++++++++++++-------
 3 files changed, 52 insertions(+), 7 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0

[PATCH v1 1/2] hrtimer: Add hrtimer_forward_safe_from_callback()

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

A hrtimer can be started while its callback runs. hrtimer_forward()
writes the expiry without the base lock, so the caller has to serialize
it against a concurrent hrtimer_start(). Otherwise the write can change
the sort key of a queued timer.

Add a variant which does the check and the update with the base lock
held. If the timer is already queued, the expiry is left alone and the
expiry set by hrtimer_start() wins.

The first user is the Rust hrtimer abstraction. Safe Rust code can
start a timer while its callback runs, so forward() in the callback
can only be a safe API if the core handles a concurrent start.

Suggested-by: Thomas Gleixner <tglx@kernel.org>
Link: https://lore.kernel.org/all/877bk3ixp5.ffs@fw13/
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 include/linux/hrtimer.h |  4 ++++
 kernel/time/hrtimer.c   | 30 ++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index acfc5f93d967..f3c8542a2dbd 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -317,6 +317,10 @@ void hrtimer_update_function(struct hrtimer *timer,
 extern u64
 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval);
 
+extern u64
+hrtimer_forward_safe_from_callback(struct hrtimer *timer, ktime_t now,
+				   ktime_t interval);
+
 /**
  * hrtimer_forward_now() - forward the timer expiry so it expires after now
  * @timer:	hrtimer to forward
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index a9dc2d53aa92..9869827585af 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1121,6 +1121,36 @@ u64 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval)
 }
 EXPORT_SYMBOL_GPL(hrtimer_forward);
 
+/**
+ * hrtimer_forward_safe_from_callback() - forward the timer expiry under the base lock
+ * @timer:	hrtimer to forward
+ * @now:	forward past this time
+ * @interval:	the interval to forward
+ *
+ * Variant of hrtimer_forward() which is safe against a concurrent
+ * hrtimer_start(). The expiry is updated with the base lock held. If the
+ * timer is already queued, the expiry is not changed.
+ *
+ * Context: Only from the callback function of @timer.
+ *
+ * Return: The number of overruns, or 0 if the expiry was not changed.
+ */
+u64 hrtimer_forward_safe_from_callback(struct hrtimer *timer, ktime_t now,
+				       ktime_t interval)
+{
+	unsigned long flags;
+	u64 orun = 0;
+
+	lock_hrtimer_base(timer, &flags);
+
+	if (!timer->is_queued)
+		orun = hrtimer_forward(timer, now, interval);
+
+	unlock_hrtimer_base(timer, &flags);
+	return orun;
+}
+EXPORT_SYMBOL_GPL(hrtimer_forward_safe_from_callback);
+
 /*
  * enqueue_hrtimer - internal function to (re)start a timer
  *
-- 
2.43.0

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

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

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@gmail.com>
---
 rust/kernel/time/hrtimer.rs | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index a53f7f1341be..bf4094d84822 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -521,8 +521,7 @@ pub(crate) unsafe fn raw_cancel(this: *const Self) -> bool {
     /// # Safety
     ///
     /// - `self_ptr` must point to a valid `Self`.
-    /// - The caller must either have exclusive access to the data pointed at by `self_ptr`, or be
-    ///   within the context of the timer callback.
+    /// - The caller must have exclusive access to the data pointed at by `self_ptr`.
     #[inline]
     unsafe fn raw_forward(self_ptr: *mut Self, now: HrTimerInstant<T>, interval: Delta) -> u64
     where
@@ -1060,13 +1059,25 @@ pub(crate) unsafe fn from_raw(timer: *mut HrTimer<T>) -> Self {
 
     /// Conditionally forward the timer.
     ///
-    /// This function is identical to [`HrTimer::forward()`] except that it may only be used from
-    /// within the context of a [`HrTimer`] callback.
+    /// This function does the same as [`HrTimer::forward()`], but it may only be used from within
+    /// the context of a [`HrTimer`] callback, and it is safe against a concurrent start operation.
+    /// If the timer was started by a concurrent start operation, the expiry is not changed, this
+    /// function returns 0, and the expiry set by the start operation is used.
     pub fn forward(&mut self, now: HrTimerInstant<T>, interval: Delta) -> u64 {
+        // SAFETY: By our type invariants, `self.0` always points to a valid `HrTimer<T>`.
+        let c_timer_ptr = unsafe { HrTimer::<T>::raw_get(self.0.as_ptr()) };
+
         // SAFETY:
-        // - We are guaranteed to be within the context of a timer callback by our type invariants
-        // - By our type invariants, `self.0` always points to a valid `HrTimer<T>`
-        unsafe { HrTimer::<T>::raw_forward(self.0.as_ptr(), now, interval) }
+        // - `c_timer_ptr` is initialized and valid.
+        // - By our type invariants, we are within the context of the timer callback, as
+        //   `hrtimer_forward_safe_from_callback()` requires.
+        unsafe {
+            bindings::hrtimer_forward_safe_from_callback(
+                c_timer_ptr,
+                now.as_nanos(),
+                interval.as_nanos(),
+            )
+        }
     }
 
     /// Conditionally forward the timer.
-- 
2.43.0

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

[Andreas Hindborg]

"FUJITA Tomonori" <tomo@flapping.org> writes:

> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>
> 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@gmail.com>

This looks good to me.

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

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