[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