[PATCH 0/6] hrtimer: add an expiry injecting callback variant

[PATCH 0/6] hrtimer: add an expiry injecting callback variant

[Andreas Hindborg]

An hrtimer callback runs after __run_hrtimer() has dropped the
cpu_base->lock, and a timer can be armed from any CPU at any time,
including while its callback is running. A callback that adjusts its
own expiry with hrtimer_forward() therefore races with a concurrent
hrtimer_start_range_ns():

- The read-modify-write of node.expires in hrtimer_forward() is
  unsynchronized against the write in the start path, which happens
  under the base lock.

- The is_queued check in hrtimer_forward() is a
  time-of-check-to-time-of-use bug. A concurrent start can enqueue
  the timer right after the check, and forwarding a queued timer
  changes the expiry of a node inside the timerqueue without
  re-sorting it, leaving the tree unordered.

Users that both forward from the callback and arm the timer from
other contexts have to serialize the two themselves, as perf does
with cpc->hrtimer_lock and the hrtimer_active flag, see 4cfafd3082af
("sched,perf: Fix periodic timers"). The requirement is subtle. i915_pmu
and taprio do not honor it today.

For the Rust hrtimer abstraction this is a soundness problem rather than
a documentation problem: safe Rust code can arm a timer whose callback
is running, because Arc<T> is Clone and Pin<&T> is Copy, so a callback
context forward() cannot be offered as safe API at all. Making arming
exclusive in the Rust type system was tried [1] and abandoned. It adds
complexity to the Arc based API, and it leaves the C interface as the
same trap for C users.

Gary suggested [2] removing the race structurally instead: snapshot
the expiry under the base lock, hand it to the callback by value, and
have the callback request the forward and the requeue instead of
performing them itself. This series implements that suggestion.

Patch 1 adds the expiry injecting callback variant to the hrtimer core.
Such a callback is installed with hrtimer_setup_ext() and receives
the expiry snapshotted under the base lock. To restart the timer it
fills a struct hrtimer_forward_args and returns HRTIMER_RESTART, and
__run_hrtimer() then applies the forward and the enqueue with the base
lock held. The callback never touches live timer state. If a concurrent
start enqueued the timer while the callback ran, the restart request
is discarded and the start wins, which matches how we already treat a
restart of a timer that was requeued behind the callback's back. The
new callback pointer shares storage with the classic one in an anonymous
union and is discriminated by a flag placed in existing padding, so
struct hrtimer does not grow and the classic callback path is untouched.

Patch 2 converts i915_pmu, which forwards from its sampling callback
while gt park/unpark can start the timer from another CPU. The
conversion closes that window without adding locking to the sampling
path, and demonstrates that the new variant is not Rust-only
plumbing.

Patches 3 to 6 are the Rust side. Patch 3 moves the abstraction to
the new callback variant: HrTimerCallback::run() receives the expiry
snapshot and returns HrTimerRestart::Forward { now, interval }, and
HrTimerCallbackContext with its forward()/forward_now() methods is
removed. Patch 4 is Tomonori's expires() fix rebased on top, now
justified by exclusive access rather than by callback context. Patch
5 documents the pre-existing hazard that starting a timer from within
its own handler self-deadlocks when the returned handle is dropped
there.

The i915 patch is compile tested only, I have no hardware for it.

[1]: https://lore.kernel.org/rust-for-linux/20260813134834.1562995-1-tomo@flapping.org/
[2]: https://lore.kernel.org/rust-for-linux/DKNVOU9JC15P.3DEBNZ56QK20E@garyguo.net/

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Andreas Hindborg (4):
      hrtimer: add expiry injecting callback variant
      drm/i915/pmu: use the expiry injecting hrtimer callback
      rust: hrtimer: use the expiry injecting callback variant
      rust: hrtimer: document deadlock when starting a timer in its handler

FUJITA Tomonori (2):
      rust: hrtimer: restrict expires() to exclusive access
      rust: hrtimer: Make HrTimer repr(transparent)

 drivers/gpu/drm/i915/i915_pmu.c     |   8 +-
 include/linux/hrtimer.h             |   7 +
 include/linux/hrtimer_types.h       |  34 ++++-
 kernel/time/hrtimer.c               | 112 +++++++++++++++-
 rust/helpers/time.c                 |   6 +
 rust/kernel/time/hrtimer.rs         | 257 +++++++++++++++++++-----------------
 rust/kernel/time/hrtimer/arc.rs     |  23 ++--
 rust/kernel/time/hrtimer/pin.rs     |  23 ++--
 rust/kernel/time/hrtimer/pin_mut.rs |  26 ++--
 rust/kernel/time/hrtimer/tbox.rs    |  23 ++--
 10 files changed, 352 insertions(+), 167 deletions(-)
---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260825-expires-v2-0764adf4c466

Best regards,
--  
Andreas Hindborg <a.hindborg@kernel.org>

[PATCH 1/6] hrtimer: add expiry injecting callback variant

[Andreas Hindborg]

A hrtimer callback may modify its own expiry with hrtimer_forward()
and read it with hrtimer_get_expires(). Both run after
__run_hrtimer() has dropped the cpu_base->lock, so they race with a
concurrent hrtimer_start_range_ns() on another CPU, which rewrites
node.expires under the base lock and requeues the timer:

- The unlocked read-modify-write of node.expires in
  hrtimer_forward() is a data race against the locked write in the
  start path.

- The is_queued check in hrtimer_forward() is racy with
  a time-of-check-time-of-use bug as well: a concurrent
  start can enqueue the timer between the check and the expiry
  update, and forwarding an already queued timer changes the expiry
  of a node inside the timerqueue without re-sorting, leaving the
  tree unordered.

Timer users which both forward in the callback and arm from other contexts
must provide their own serialization, e.g. perf's cpc->hrtimer_lock plus
hrtimer_active flag, see commit 4cfafd3082af ("sched,perf: Fix periodic
timers"). The requirement is subtle and not enforced; i915_pmu and taprio
currently get it wrong. For the Rust hrtimer abstraction it is a soundness
problem: safe code can arm a timer whose callback is running, so callback
context forward and expiry reads cannot be offered as safe API.

Add an alternative callback variant that removes the race
structurally instead of requiring serialization. An expiry injecting
callback receives the expiry snapshotted under the base lock by
value and, to restart the timer, fills a struct hrtimer_forward_args
and returns HRTIMER_RESTART. __run_hrtimer() then applies the
forward and the enqueue with the base lock held. The callback never
accesses live timer state.

If a concurrent start enqueued the timer while the callback ran, the
restart request is discarded and the start wins, matching the
existing "restart == HRTIMER_RESTART && !timer->is_queued" handling
for classic callbacks. The is_queued check is reliable here: while
base->running == timer, hrtimer_try_to_cancel() bails out before
remove_hrtimer() and the timer cannot switch bases, so only a
concurrent start can enqueue it, and the start path writes the
expiry and is_queued in the same critical section. Thus !is_queued
at requeue time guarantees the expiry still equals the snapshot
handed to the callback: the deferred hrtimer_forward() cannot hit
its concurrent start check, and an overrun count the callback
derived from the snapshot is consistent with the forward that is
applied.

The new callback pointer shares storage with the classic one in an
anonymous union, discriminated by a new is_ext flag placed in
existing padding; sizeof(struct hrtimer) is unchanged and the
classic callback path is unaffected. hrtimer_update_function()
rejects timers with an expiry injecting callback.

Link: https://lore.kernel.org/r/87h5kp88uy.fsf@kernel.org
Suggested-by: Gary Guo <gary@garyguo.net>
Assisted-by: claude-code:claude-fable-5
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 include/linux/hrtimer.h       |   7 +++
 include/linux/hrtimer_types.h |  34 ++++++++++++-
 kernel/time/hrtimer.c         | 112 +++++++++++++++++++++++++++++++++++++++---
 3 files changed, 145 insertions(+), 8 deletions(-)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 6862dea0acc52..a45a0677238c9 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -190,6 +190,10 @@ static inline enum hrtimer_restart hrtimer_dummy_timeout(struct hrtimer *unused)
 /* Initialize timers: */
 extern void hrtimer_setup(struct hrtimer *timer, enum hrtimer_restart (*function)(struct hrtimer *),
 			  clockid_t clock_id, enum hrtimer_mode mode);
+
+extern void hrtimer_setup_ext(struct hrtimer *timer, hrtimer_ext_func_t function_ext,
+			      clockid_t clock_id, enum hrtimer_mode mode);
+
 extern void hrtimer_setup_on_stack(struct hrtimer *timer,
 				   enum hrtimer_restart (*function)(struct hrtimer *),
 				   clockid_t clock_id, enum hrtimer_mode mode);
@@ -307,6 +311,9 @@ static inline int hrtimer_callback_running(struct hrtimer *timer)
 static inline void hrtimer_update_function(struct hrtimer *timer,
 					   enum hrtimer_restart (*function)(struct hrtimer *))
 {
+	if (WARN_ON_ONCE(timer->is_ext))
+		return;
+
 #ifdef CONFIG_PROVE_LOCKING
 	guard(raw_spinlock_irqsave)(&timer->base->cpu_base->lock);
 
diff --git a/include/linux/hrtimer_types.h b/include/linux/hrtimer_types.h
index b5dacc8271a4a..7c7e03dffcae7 100644
--- a/include/linux/hrtimer_types.h
+++ b/include/linux/hrtimer_types.h
@@ -5,6 +5,7 @@
 #include <linux/types.h>
 #include <linux/timerqueue_types.h>
 
+struct hrtimer;
 struct hrtimer_clock_base;
 
 /*
@@ -15,6 +16,26 @@ enum hrtimer_restart {
 	HRTIMER_RESTART,	/* Timer must be restarted */
 };
 
+/**
+ * struct hrtimer_forward_args - deferred forward request of an expiry
+ *				 injecting callback
+ * @now:	forward past this time
+ * @interval:	the interval to forward by
+ *
+ * Filled in by an expiry injecting callback (see hrtimer_setup_ext())
+ * which returns HRTIMER_RESTART. The hrtimer core applies the forward
+ * with the timer base lock held before requeueing the timer, i.e.
+ * hrtimer_forward(timer, now, interval).
+ */
+struct hrtimer_forward_args {
+	ktime_t				now;
+	ktime_t				interval;
+};
+
+/* Callback function of an expiry injecting timer, see hrtimer_setup_ext() */
+typedef enum hrtimer_restart (*hrtimer_ext_func_t)(struct hrtimer *timer, ktime_t expires,
+						   struct hrtimer_forward_args *fwd);
+
 /**
  * struct hrtimer - the basic hrtimer structure
  * @node:	Linked timerqueue node, which also manages node.expires,
@@ -27,6 +48,9 @@ enum hrtimer_restart {
  *		The time which was given as expiry time when the timer
  *		was armed.
  * @function:	timer expiry callback function
+ * @function_ext: expiry injecting timer callback function, receives the
+ *		expiry snapshot and returns a forward request instead of
+ *		modifying the expiry itself. Valid if @is_ext is set.
  * @base:	pointer to the timer base (per cpu and per clock)
  * @is_queued:	Indicates whether a timer is enqueued or not
  * @is_rel:	Set if the timer was armed relative
@@ -35,8 +59,10 @@ enum hrtimer_restart {
  *		even on RT.
  * @is_lazy:	Set if the timer is frequently rearmed to avoid updates
  *		of the clock event device
+ * @is_ext:	Set if @function_ext is valid instead of @function
  *
- * The hrtimer structure must be initialized by hrtimer_setup()
+ * The hrtimer structure must be initialized by hrtimer_setup() or
+ * hrtimer_setup_ext()
  */
 struct hrtimer {
 	struct timerqueue_linked_node	node;
@@ -46,8 +72,12 @@ struct hrtimer {
 	bool				is_soft;
 	bool				is_hard;
 	bool				is_lazy;
+	bool				is_ext;
 	ktime_t				_softexpires;
-	enum hrtimer_restart		(*__private function)(struct hrtimer *);
+	union {
+		enum hrtimer_restart	(*__private function)(struct hrtimer *);
+		hrtimer_ext_func_t	__private function_ext;
+	};
 };
 
 #endif /* _LINUX_HRTIMER_TYPES_H */
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 313dcea127fe4..e718dd0e05195 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1863,8 +1863,7 @@ ktime_t hrtimer_cb_get_time(const struct hrtimer *timer)
 }
 EXPORT_SYMBOL_GPL(hrtimer_cb_get_time);
 
-static void __hrtimer_setup(struct hrtimer *timer, enum hrtimer_restart (*fn)(struct hrtimer *),
-			    clockid_t clock_id, enum hrtimer_mode mode)
+static void __hrtimer_init(struct hrtimer *timer, clockid_t clock_id, enum hrtimer_mode mode)
 {
 	bool softtimer = !!(mode & HRTIMER_MODE_SOFT);
 	struct hrtimer_cpu_base *cpu_base;
@@ -1898,6 +1897,12 @@ static void __hrtimer_setup(struct hrtimer *timer, enum hrtimer_restart (*fn)(st
 	timer->is_lazy = !!(mode & HRTIMER_MODE_LAZY_REARM);
 	timer->base = &cpu_base->clock_base[base];
 	timerqueue_linked_init(&timer->node);
+}
+
+static void __hrtimer_setup(struct hrtimer *timer, enum hrtimer_restart (*fn)(struct hrtimer *),
+			    clockid_t clock_id, enum hrtimer_mode mode)
+{
+	__hrtimer_init(timer, clock_id, mode);
 
 	if (WARN_ON_ONCE(!fn))
 		ACCESS_PRIVATE(timer, function) = hrtimer_dummy_timeout;
@@ -1905,6 +1910,20 @@ static void __hrtimer_setup(struct hrtimer *timer, enum hrtimer_restart (*fn)(st
 		ACCESS_PRIVATE(timer, function) = fn;
 }
 
+static void __hrtimer_setup_ext(struct hrtimer *timer, hrtimer_ext_func_t fn,
+				clockid_t clock_id, enum hrtimer_mode mode)
+{
+	__hrtimer_init(timer, clock_id, mode);
+
+	if (WARN_ON_ONCE(!fn)) {
+		ACCESS_PRIVATE(timer, function) = hrtimer_dummy_timeout;
+		return;
+	}
+
+	ACCESS_PRIVATE(timer, function_ext) = fn;
+	timer->is_ext = true;
+}
+
 /**
  * hrtimer_setup - initialize a timer to the given clock
  * @timer:	the timer to be initialized
@@ -1926,6 +1945,45 @@ void hrtimer_setup(struct hrtimer *timer, enum hrtimer_restart (*function)(struc
 }
 EXPORT_SYMBOL_GPL(hrtimer_setup);
 
+/**
+ * hrtimer_setup_ext - initialize a timer with an expiry injecting callback
+ * @timer:	the timer to be initialized
+ * @function_ext: the expiry injecting callback function
+ * @clock_id:	the clock to be used
+ * @mode:       The modes which are relevant for initialization:
+ *              HRTIMER_MODE_ABS, HRTIMER_MODE_REL, HRTIMER_MODE_ABS_SOFT,
+ *              HRTIMER_MODE_REL_SOFT
+ *
+ *              The PINNED variants of the above can be handed in,
+ *              but the PINNED bit is ignored as pinning happens
+ *              when the hrtimer is started
+ *
+ * In contrast to a callback installed by hrtimer_setup(), an expiry
+ * injecting callback does not access the expiry of the timer itself.
+ * The expiry is snapshotted under the timer base lock and handed into
+ * the callback by value. To restart the timer, the callback fills @fwd
+ * and returns HRTIMER_RESTART; the core then applies
+ * hrtimer_forward(timer, fwd->now, fwd->interval) and requeues the
+ * timer, both under the timer base lock.
+ *
+ * This closes the race between hrtimer_forward()/expiry reads in
+ * callback context and a concurrent hrtimer_start() on another CPU,
+ * without requiring the timer user to provide serialization: if a
+ * concurrent start requeued the timer while the callback ran, the
+ * restart request is discarded and the concurrent start wins.
+ *
+ * The callback must not call hrtimer_forward() or modify the expiry
+ * itself, and a callback returning HRTIMER_RESTART must fill @fwd with
+ * a non zero interval.
+ */
+void hrtimer_setup_ext(struct hrtimer *timer, hrtimer_ext_func_t function_ext,
+		       clockid_t clock_id, enum hrtimer_mode mode)
+{
+	debug_setup(timer, clock_id, mode);
+	__hrtimer_setup_ext(timer, function_ext, clock_id, mode);
+}
+EXPORT_SYMBOL_GPL(hrtimer_setup_ext);
+
 /**
  * hrtimer_setup_on_stack - initialize a timer on stack memory
  * @timer:	The timer to be initialized
@@ -1991,8 +2049,11 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base, struct hrtimer_cloc
 			  struct hrtimer *timer, ktime_t now, unsigned long flags)
 	__must_hold(&cpu_base->lock)
 {
-	enum hrtimer_restart (*fn)(struct hrtimer *);
+	enum hrtimer_restart (*fn)(struct hrtimer *) = NULL;
+	struct hrtimer_forward_args fwd = { };
+	hrtimer_ext_func_t fn_ext = NULL;
 	bool expires_in_hardirq;
+	ktime_t expires = 0;
 	int restart;
 
 	lockdep_assert_held(&cpu_base->lock);
@@ -2010,7 +2071,20 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base, struct hrtimer_cloc
 	raw_write_seqcount_barrier(&base->seq);
 
 	__remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, false);
-	fn = ACCESS_PRIVATE(timer, function);
+
+	/*
+	 * Snapshot the expiry for an expiry injecting callback while the
+	 * base lock is still held. The callback gets the snapshot by
+	 * value and must not access timer->node.expires, which a
+	 * concurrent hrtimer_start_range_ns() can modify once the lock is
+	 * dropped.
+	 */
+	if (timer->is_ext) {
+		fn_ext = ACCESS_PRIVATE(timer, function_ext);
+		expires = hrtimer_get_expires(timer);
+	} else {
+		fn = ACCESS_PRIVATE(timer, function);
+	}
 
 	/*
 	 * Clear the 'is relative' flag for the TIME_LOW_RES case. If the
@@ -2029,12 +2103,19 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base, struct hrtimer_cloc
 	trace_hrtimer_expire_entry(timer, now);
 	expires_in_hardirq = lockdep_hrtimer_enter(timer);
 
-	restart = fn(timer);
+	if (fn_ext)
+		restart = fn_ext(timer, expires, &fwd);
+	else
+		restart = fn(timer);
 
 	lockdep_hrtimer_exit(expires_in_hardirq);
 	trace_hrtimer_expire_exit(timer);
 	raw_spin_lock_irq(&cpu_base->lock);
 
+	/* An expiry injecting callback requesting a restart must forward. */
+	if (fn_ext && restart == HRTIMER_RESTART && WARN_ON_ONCE(!fwd.interval))
+		restart = HRTIMER_NORESTART;
+
 	/*
 	 * Note: We clear the running state after enqueue_hrtimer and
 	 * we do not reprogram the event hardware. Happens either in
@@ -2044,8 +2125,27 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base, struct hrtimer_cloc
 	 * hrtimer_start_range_ns() can have popped in and enqueued the timer
 	 * for us already.
 	 */
-	if (restart == HRTIMER_RESTART && !timer->is_queued)
+	if (restart == HRTIMER_RESTART && !timer->is_queued) {
+		/*
+		 * Apply the deferred forward of an expiry injecting
+		 * callback with the base lock held.
+		 *
+		 * While base->running == timer, hrtimer_try_to_cancel()
+		 * bails out before remove_hrtimer() and the timer cannot
+		 * switch bases, so only a concurrent start can have
+		 * enqueued the timer and it writes the expiry and
+		 * is_queued in the same critical section. Thus !is_queued
+		 * here guarantees that the expiry is still equal to the
+		 * snapshot handed to the callback, and the concurrent
+		 * start check in hrtimer_forward() cannot trigger. If a
+		 * concurrent start enqueued the timer, is_queued is set
+		 * and the restart request is discarded - the concurrent
+		 * start expressed newer intent and wins.
+		 */
+		if (fn_ext)
+			hrtimer_forward(timer, fwd.now, fwd.interval);
 		enqueue_hrtimer(timer, base, HRTIMER_MODE_ABS, false);
+	}
 
 	/*
 	 * Separate the ->running assignment from the ->is_queued assignment.

-- 
2.51.2

[PATCH 2/6] drm/i915/pmu: use the expiry injecting hrtimer callback

[Andreas Hindborg]

The sampling timer forwards itself from its callback:

  i915_sample()
    hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));

hrtimer callbacks run with the timer base lock dropped, so this
read-modify-write of the expiry races with a concurrent restart of
the timer. Such a restart is possible here: i915_pmu_gt_parked()
clears pmu->timer_enabled, and a subsequent
__i915_pmu_maybe_start_timer() from i915_pmu_gt_unparked() or event
enable on another CPU sees the timer disabled and calls
hrtimer_start_range_ns() - also while the callback is running, since
i915_sample() checks timer_enabled only once at entry and takes no
lock. hrtimer_forward() then operates on an already requeued timer:
it warns and, in the worst case, rewrites the expiry of an enqueued
timer without the base lock.

Convert the timer to the expiry injecting callback variant. The
callback returns the forward request instead of applying it, and the
hrtimer core applies it under the timer base lock. If a concurrent
start requeued the timer while the callback ran, the core discards
the callback's restart request and the start wins, which closes the
park/unpark race without adding any locking to the sampling path.

No functional change in the common case: the timer still forwards by
PERIOD past the sampling timestamp.

Assisted-by: claude-code:claude-fable-5
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 drivers/gpu/drm/i915/i915_pmu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 1c3bafda9c708..c18587e200c9f 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -502,7 +502,8 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
 	intel_gt_pm_put_async(gt, wakeref);
 }
 
-static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
+static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer, ktime_t expires,
+					struct hrtimer_forward_args *fwd)
 {
 	struct i915_pmu *pmu = container_of(hrtimer, struct i915_pmu, timer);
 	struct drm_i915_private *i915 = pmu_to_i915(pmu);
@@ -533,7 +534,8 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
 		frequency_sample(gt, period_ns);
 	}
 
-	hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
+	fwd->now = now;
+	fwd->interval = ns_to_ktime(PERIOD);
 
 	return HRTIMER_RESTART;
 }
@@ -1157,7 +1159,7 @@ void i915_pmu_register(struct drm_i915_private *i915)
 	int ret = -ENOMEM;
 
 	spin_lock_init(&pmu->lock);
-	hrtimer_setup(&pmu->timer, i915_sample, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_setup_ext(&pmu->timer, i915_sample, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	init_rc6(pmu);
 
 	if (IS_DGFX(i915)) {

-- 
2.51.2

[PATCH 3/6] rust: hrtimer: use the expiry injecting callback variant

[Andreas Hindborg]

A timer callback could modify the expiry of its timer with
HrTimerCallbackContext::forward(). The C side runs callbacks with the
timer base lock dropped, and a concurrent start operation - reachable
from safe code, since Arc<T> is Clone and Pin<&T> is Copy - rewrites
the expiry under the base lock and requeues the timer. The unlocked
read-modify-write of the expiry in hrtimer_forward() is a data race
with such a concurrent start, and forwarding a requeued timer changes
the expiry of a node inside the timerqueue without re-sorting it,
leaving the tree unordered.

Switch the abstraction to the expiry injecting callback variant
provided by hrtimer_setup_ext(). The callback receives the expiry by
value, snapshotted under the timer base lock, and requests a restart
by returning HrTimerRestart::Forward { now, interval }. The timer
core applies the forward and requeues the timer under the timer base
lock after the callback has returned. If a concurrent start operation
requeued the timer while the callback ran, the request is discarded
and the concurrent start operation wins.

The callback never accesses live timer state, so the race is removed
structurally while concurrent start operations remain allowed:

- HrTimerCallback::run() receives the expiry snapshot instead of a
  HrTimerCallbackContext and returns HrTimerRestart<T>, which now
  carries the forward request.

- HrTimerCallbackContext is removed together with its forward() and
  forward_now() methods. HrTimer::forward() on an exclusive reference
  remains available.

- The callback trampolines of the four pointer types translate the
  expiry snapshot and the forward request across the FFI boundary.

Link: https://lore.kernel.org/r/87h5kp88uy.fsf@kernel.org
Suggested-by: Gary Guo <gary@garyguo.net>
Assisted-by: claude-code:claude-fable-5
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs         | 212 ++++++++++++++++++------------------
 rust/kernel/time/hrtimer/arc.rs     |  21 ++--
 rust/kernel/time/hrtimer/pin.rs     |  21 ++--
 rust/kernel/time/hrtimer/pin_mut.rs |  24 ++--
 rust/kernel/time/hrtimer/tbox.rs    |  21 ++--
 5 files changed, 162 insertions(+), 137 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 2d7f1131a8131..e6570a6162035 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -60,16 +60,23 @@
 //! by the `cancel` operation. A timer that is cancelled enters the **stopped**
 //! state.
 //!
-//! A `cancel` or `restart` operation on a timer in the **running** state takes
-//! effect after the handler has returned and the timer has transitioned
-//! out of the **running** state.
+//! The handler receives the expiry time that was in effect when the timer fired. To restart the
+//! timer, the handler returns a forward request; the timer core applies the request and requeues
+//! the timer. A concurrent `start` operation overrides the restart request of the timer handler.
+//!
+//! A `cancel` operation on a timer in the **running** state takes effect after
+//! the handler has returned and the timer has transitioned out of the
+//! **running** state.
 //!
 //! A `restart` operation on a timer in the **stopped** state is equivalent to a
 //! `start` operation.
 //!
-//! When a type implements both `HrTimerPointer` and `Clone`, it is possible to
-//! issue the `start` operation while the timer is in the **started** state. In
-//! this case the `start` operation is equivalent to the `restart` operation.
+//! When a type implements both `HrTimerPointer` and `Clone`, it is possible to issue the `start`
+//! operation while the timer is in the **started** or **running** state. In this case the `start`
+//! operation is equivalent to the `restart` operation. A `restart` operation on a timer in the
+//! **running** state takes effect immediately: the timer re-enters the **started** state before the
+//! handler returns, and a restart requested by the return value of the handler is discarded in
+//! favor of the `restart` operation.
 //!
 //! # Examples
 //!
@@ -87,8 +94,8 @@
 //! #     },
 //! #     time::{
 //! #         hrtimer::{
-//! #             RelativeMode, HrTimer, HrTimerCallback, HrTimerPointer,
-//! #             HrTimerRestart, HrTimerCallbackContext
+//! #             RelativeMode, HrTimer, HrTimerCallback, HrTimerInstant,
+//! #             HrTimerPointer, HrTimerRestart
 //! #         },
 //! #         Delta, Monotonic,
 //! #     },
@@ -130,7 +137,7 @@
 //! impl HrTimerCallback for BoxIntrusiveHrTimer {
 //!     type Pointer<'a> = Pin<KBox<Self>>;
 //!
-//!     fn run(this: Pin<&mut Self>, _ctx: HrTimerCallbackContext<'_, Self>) -> HrTimerRestart {
+//!     fn run(this: Pin<&mut Self>, _expires: HrTimerInstant<Self>) -> HrTimerRestart<Self> {
 //!         pr_info!("Timer called\n");
 //!
 //!         let flag = this.shared.flag.fetch_add(1, ordering::Full);
@@ -139,7 +146,7 @@
 //!         if flag == 4 {
 //!             HrTimerRestart::NoRestart
 //!         } else {
-//!             HrTimerRestart::Restart
+//!             HrTimerRestart::forward_now(Delta::from_micros(200))
 //!         }
 //!     }
 //! }
@@ -176,8 +183,8 @@
 //! #     },
 //! #     time::{
 //! #         hrtimer::{
-//! #             RelativeMode, HrTimer, HrTimerCallback, HrTimerPointer, HrTimerRestart,
-//! #             HasHrTimer, HrTimerCallbackContext
+//! #             RelativeMode, HrTimer, HrTimerCallback, HrTimerInstant, HrTimerPointer,
+//! #             HrTimerRestart, HasHrTimer
 //! #         },
 //! #         Delta, Monotonic,
 //! #     },
@@ -208,8 +215,8 @@
 //!
 //!     fn run(
 //!         this: ArcBorrow<'_, Self>,
-//!         _ctx: HrTimerCallbackContext<'_, Self>,
-//!     ) -> HrTimerRestart {
+//!         _expires: HrTimerInstant<Self>,
+//!     ) -> HrTimerRestart<Self> {
 //!         pr_info!("Timer called\n");
 //!
 //!         let flag = this.flag.fetch_add(1, ordering::Full);
@@ -218,7 +225,7 @@
 //!         if flag == 4 {
 //!             HrTimerRestart::NoRestart
 //!         } else {
-//!             HrTimerRestart::Restart
+//!             HrTimerRestart::forward_now(Delta::from_micros(200))
 //!         }
 //!     }
 //! }
@@ -252,8 +259,8 @@
 //! #     },
 //! #     time::{
 //! #         hrtimer::{
-//! #             ScopedHrTimerPointer, HrTimer, HrTimerCallback, HrTimerPointer, HrTimerRestart,
-//! #             HasHrTimer, RelativeMode, HrTimerCallbackContext
+//! #             ScopedHrTimerPointer, HrTimer, HrTimerCallback, HrTimerInstant,
+//! #             HrTimerPointer, HrTimerRestart, HasHrTimer, RelativeMode
 //! #         },
 //! #         Delta, Monotonic,
 //! #     },
@@ -283,7 +290,7 @@
 //! impl HrTimerCallback for IntrusiveHrTimer {
 //!     type Pointer<'a> = Pin<&'a Self>;
 //!
-//!     fn run(this: Pin<&Self>, _ctx: HrTimerCallbackContext<'_, Self>) -> HrTimerRestart {
+//!     fn run(this: Pin<&Self>, _expires: HrTimerInstant<Self>) -> HrTimerRestart<Self> {
 //!         pr_info!("Timer called\n");
 //!
 //!         this.flag.store(1, ordering::Release);
@@ -324,8 +331,8 @@
 //! #     },
 //! #     time::{
 //! #         hrtimer::{
-//! #             ScopedHrTimerPointer, HrTimer, HrTimerCallback, HrTimerPointer, HrTimerRestart,
-//! #             HasHrTimer, RelativeMode, HrTimerCallbackContext
+//! #             ScopedHrTimerPointer, HrTimer, HrTimerCallback, HrTimerInstant,
+//! #             HrTimerPointer, HrTimerRestart, HasHrTimer, RelativeMode
 //! #         },
 //! #         Delta, Monotonic,
 //! #     },
@@ -368,7 +375,7 @@
 //! impl HrTimerCallback for IntrusiveHrTimer {
 //!     type Pointer<'a> = Pin<&'a mut Self>;
 //!
-//!     fn run(this: Pin<&mut Self>, _ctx: HrTimerCallbackContext<'_, Self>) -> HrTimerRestart {
+//!     fn run(this: Pin<&mut Self>, _expires: HrTimerInstant<Self>) -> HrTimerRestart<Self> {
 //!         pr_info!("Timer called\n");
 //!
 //!         let flag = this.shared.flag.fetch_add(1, ordering::Full);
@@ -377,7 +384,7 @@
 //!         if flag == 4 {
 //!             HrTimerRestart::NoRestart
 //!         } else {
-//!             HrTimerRestart::Restart
+//!             HrTimerRestart::forward_now(Delta::from_micros(200))
 //!         }
 //!     }
 //! }
@@ -405,7 +412,7 @@
 
 use super::{ClockSource, Delta, Instant};
 use crate::{prelude::*, types::Opaque};
-use core::{marker::PhantomData, ptr::NonNull};
+use core::marker::PhantomData;
 use pin_init::PinInit;
 
 /// A type-alias to refer to the [`Instant<C>`] for a given `T` from [`HrTimer<T>`].
@@ -417,7 +424,7 @@
 ///
 /// # Invariants
 ///
-/// * `self.timer` is initialized by `bindings::hrtimer_setup`.
+/// * `self.timer` is initialized by `bindings::hrtimer_setup_ext`.
 #[pin_data]
 #[repr(C)]
 pub struct HrTimer<T> {
@@ -442,13 +449,14 @@ pub fn new() -> impl PinInit<Self>
         T: HasHrTimer<T>,
     {
         pin_init!(Self {
-            // INVARIANT: We initialize `timer` with `hrtimer_setup` below.
+            // INVARIANT: We initialize `timer` with `hrtimer_setup_ext` below.
             timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
                 // SAFETY: By design of `pin_init!`, `place` is a pointer to a
-                // live allocation. hrtimer_setup will initialize `place` and
-                // does not require `place` to be initialized prior to the call.
+                // live allocation. hrtimer_setup_ext will initialize `place`
+                // and does not require `place` to be initialized prior to the
+                // call.
                 unsafe {
-                    bindings::hrtimer_setup(
+                    bindings::hrtimer_setup_ext(
                         place,
                         Some(T::Pointer::run),
                         <<T as HasHrTimer<T>>::TimerMode as HrTimerMode>::Clock::ID,
@@ -510,8 +518,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
@@ -533,8 +540,8 @@ unsafe fn raw_forward(self_ptr: *mut Self, now: HrTimerInstant<T>, interval: Del
     /// `interval`.
     ///
     /// This function is mainly useful for timer types which can provide exclusive access to the
-    /// timer when the timer is not running. For forwarding the timer from within the timer callback
-    /// context, see [`HrTimerCallbackContext::forward()`].
+    /// timer when the timer is not running. To forward the timer from within the timer callback,
+    /// return [`HrTimerRestart::Forward`] from the callback instead.
     ///
     /// Returns the number of overruns that occurred as a result of the timer expiry change.
     pub fn forward(self: Pin<&mut Self>, now: HrTimerInstant<T>, interval: Delta) -> u64
@@ -707,9 +714,16 @@ pub trait RawHrTimerCallback {
     ///
     /// # Safety
     ///
-    /// Only to be called by C code in the `hrtimer` subsystem. `this` must point
-    /// to the `bindings::hrtimer` structure that was used to start the timer.
-    unsafe extern "C" fn run(this: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
+    /// Only to be called by C code in the `hrtimer` subsystem. `this` must
+    /// point to the `bindings::hrtimer` structure that was used to start the
+    /// timer, `expires` must be the expiry of the timer snapshotted under the
+    /// timer base lock, and `fwd` must be valid for writing a
+    /// `bindings::hrtimer_forward_args`.
+    unsafe extern "C" fn run(
+        this: *mut bindings::hrtimer,
+        expires: bindings::ktime_t,
+        fwd: *mut bindings::hrtimer_forward_args,
+    ) -> bindings::hrtimer_restart;
 }
 
 /// Implemented by structs that can be the target of a timer callback.
@@ -719,10 +733,14 @@ pub trait HrTimerCallback {
     type Pointer<'a>: RawHrTimerCallback;
 
     /// Called by the timer logic when the timer fires.
+    ///
+    /// `expires` is the expiry time of the timer, read under the timer base
+    /// lock when the timer fired. A concurrent restart of the timer is not
+    /// reflected in `expires`.
     fn run(
         this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>,
-        ctx: HrTimerCallbackContext<'_, Self>,
-    ) -> HrTimerRestart
+        expires: HrTimerInstant<Self>,
+    ) -> HrTimerRestart<Self>
     where
         Self: Sized,
         Self: HasHrTimer<Self>;
@@ -829,19 +847,62 @@ unsafe fn start(this: *const Self, expires: <Self::TimerMode as HrTimerMode>::Ex
     }
 }
 
-/// Restart policy for timers.
-#[derive(Copy, Clone, PartialEq, Eq, Debug)]
-#[repr(u32)]
-pub enum HrTimerRestart {
+/// Restart policy for timers, as returned by [`HrTimerCallback::run`].
+///
+/// A timer callback requests a restart of its timer by returning
+/// [`HrTimerRestart::Forward`]. The forward is not applied by the callback
+/// itself: the timer core applies it and requeues the timer under the timer
+/// base lock after the callback has returned. If the timer was restarted by a
+/// concurrent start operation while the callback was running, the request is
+/// discarded and the concurrent start operation wins.
+pub enum HrTimerRestart<T: HasHrTimer<T>> {
     /// Timer should not be restarted.
-    NoRestart = bindings::hrtimer_restart_HRTIMER_NORESTART,
-    /// Timer should be restarted.
-    Restart = bindings::hrtimer_restart_HRTIMER_RESTART,
+    NoRestart,
+    /// Forward the timer expiry to lie past `now` in increments of `interval`
+    /// and restart the timer.
+    ///
+    /// `interval` must be a positive time delta.
+    Forward {
+        /// The point in time to forward the expiry past.
+        now: HrTimerInstant<T>,
+        /// The time interval to forward the expiry by.
+        interval: Delta,
+    },
 }
 
-impl HrTimerRestart {
-    fn into_c(self) -> bindings::hrtimer_restart {
-        self as bindings::hrtimer_restart
+impl<T: HasHrTimer<T>> HrTimerRestart<T> {
+    /// Request that the timer be forwarded past the current time by `interval`
+    /// and restarted.
+    pub fn forward_now(interval: Delta) -> Self {
+        Self::Forward {
+            now: HrTimerInstant::<T>::now(),
+            interval,
+        }
+    }
+
+    /// Convert to the C representation, filling `fwd` with the forward
+    /// request.
+    ///
+    /// # Safety
+    ///
+    /// `fwd` must be valid for writing a `bindings::hrtimer_forward_args`.
+    pub(crate) unsafe fn into_c(
+        self,
+        fwd: *mut bindings::hrtimer_forward_args,
+    ) -> bindings::hrtimer_restart {
+        match self {
+            Self::NoRestart => bindings::hrtimer_restart_HRTIMER_NORESTART,
+            Self::Forward { now, interval } => {
+                // SAFETY: By our safety contract, `fwd` is valid for writing.
+                unsafe {
+                    *fwd = bindings::hrtimer_forward_args {
+                        now: now.as_nanos(),
+                        interval: interval.as_nanos(),
+                    }
+                };
+                bindings::hrtimer_restart_HRTIMER_RESTART
+            }
+        }
     }
 }
 
@@ -1010,63 +1071,6 @@ impl<C: ClockSource> HrTimerMode for RelativePinnedHardMode<C> {
     type Expires = Delta;
 }
 
-/// Privileged smart-pointer for a [`HrTimer`] callback context.
-///
-/// Many [`HrTimer`] methods can only be called in two situations:
-///
-/// * When the caller has exclusive access to the `HrTimer` and the `HrTimer` is guaranteed not to
-///   be running.
-/// * From within the context of an `HrTimer`'s callback method.
-///
-/// This type provides access to said methods from within a timer callback context.
-///
-/// # Invariants
-///
-/// * The existence of this type means the caller is currently within the callback for an
-///   [`HrTimer`].
-/// * `self.0` always points to a live instance of [`HrTimer<T>`].
-pub struct HrTimerCallbackContext<'a, T: HasHrTimer<T>>(NonNull<HrTimer<T>>, PhantomData<&'a ()>);
-
-impl<'a, T: HasHrTimer<T>> HrTimerCallbackContext<'a, T> {
-    /// Create a new [`HrTimerCallbackContext`].
-    ///
-    /// # Safety
-    ///
-    /// This function relies on the caller being within the context of a timer callback, so it must
-    /// not be used anywhere except for within implementations of [`RawHrTimerCallback::run`]. The
-    /// caller promises that `timer` points to a valid initialized instance of
-    /// [`bindings::hrtimer`].
-    ///
-    /// The returned `Self` must not outlive the function context of [`RawHrTimerCallback::run`]
-    /// where this function is called.
-    pub(crate) unsafe fn from_raw(timer: *mut HrTimer<T>) -> Self {
-        // SAFETY: The caller guarantees `timer` is a valid pointer to an initialized
-        // `bindings::hrtimer`
-        // INVARIANT: Our safety contract ensures that we're within the context of a timer callback
-        // and that `timer` points to a live instance of `HrTimer<T>`.
-        Self(unsafe { NonNull::new_unchecked(timer) }, PhantomData)
-    }
-
-    /// 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.
-    pub fn forward(&mut self, now: HrTimerInstant<T>, interval: Delta) -> u64 {
-        // 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) }
-    }
-
-    /// Conditionally forward the timer.
-    ///
-    /// This is a variant of [`HrTimerCallbackContext::forward()`] that uses an interval after the
-    /// current time of the base clock for the [`HrTimer`].
-    pub fn forward_now(&mut self, duration: Delta) -> u64 {
-        self.forward(HrTimerInstant::<T>::now(), duration)
-    }
-}
-
 /// Use to implement the [`HasHrTimer<T>`] trait.
 ///
 /// See [`module`] documentation for an example.
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
index 7be82bcb352ac..8a9fcb5c69e64 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -3,13 +3,13 @@
 use super::HasHrTimer;
 use super::HrTimer;
 use super::HrTimerCallback;
-use super::HrTimerCallbackContext;
 use super::HrTimerHandle;
 use super::HrTimerMode;
 use super::HrTimerPointer;
 use super::RawHrTimerCallback;
 use crate::sync::Arc;
 use crate::sync::ArcBorrow;
+use crate::time::Instant;
 
 /// A handle for an `Arc<HasHrTimer<T>>` returned by a call to
 /// [`HrTimerPointer::start`].
@@ -79,7 +79,11 @@ impl<T> RawHrTimerCallback for Arc<T>
 {
     type CallbackTarget<'a> = ArcBorrow<'a, T>;
 
-    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+    unsafe extern "C" fn run(
+        ptr: *mut bindings::hrtimer,
+        expires: bindings::ktime_t,
+        fwd: *mut bindings::hrtimer_forward_args,
+    ) -> bindings::hrtimer_restart {
         // `HrTimer` is `repr(C)`
         let timer_ptr = ptr.cast::<super::HrTimer<T>>();
 
@@ -100,12 +104,13 @@ impl<T> RawHrTimerCallback for Arc<T>
         //    allocation from other `Arc` clones.
         let receiver = unsafe { ArcBorrow::from_raw(data_ptr) };
 
-        // SAFETY:
-        // - By C API contract `timer_ptr` is the pointer that we passed when queuing the timer, so
-        //   it is a valid pointer to a `HrTimer<T>` embedded in a `T`.
-        // - We are within `RawHrTimerCallback::run`
-        let context = unsafe { HrTimerCallbackContext::from_raw(timer_ptr) };
+        // SAFETY: By C API contract, `expires` is the expiry of the timer
+        // snapshotted under the timer base lock, and timers cannot have
+        // negative expiry times.
+        let expires = unsafe { Instant::from_ktime(expires) };
 
-        T::run(receiver, context).into_c()
+        // SAFETY: By C API contract, `fwd` is valid for writing a
+        // `bindings::hrtimer_forward_args`.
+        unsafe { T::run(receiver, expires).into_c(fwd) }
     }
 }
diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
index 4d39ef7816971..d1143f278f312 100644
--- a/rust/kernel/time/hrtimer/pin.rs
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -3,11 +3,11 @@
 use super::HasHrTimer;
 use super::HrTimer;
 use super::HrTimerCallback;
-use super::HrTimerCallbackContext;
 use super::HrTimerHandle;
 use super::HrTimerMode;
 use super::RawHrTimerCallback;
 use super::UnsafeHrTimerPointer;
+use crate::time::Instant;
 use core::pin::Pin;
 
 /// A handle for a `Pin<&HasHrTimer>`. When the handle exists, the timer might be
@@ -82,7 +82,11 @@ impl<'a, T> RawHrTimerCallback for Pin<&'a T>
 {
     type CallbackTarget<'b> = Self;
 
-    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+    unsafe extern "C" fn run(
+        ptr: *mut bindings::hrtimer,
+        expires: bindings::ktime_t,
+        fwd: *mut bindings::hrtimer_forward_args,
+    ) -> bindings::hrtimer_restart {
         // `HrTimer` is `repr(C)`
         let timer_ptr = ptr.cast::<HrTimer<T>>();
 
@@ -104,12 +108,13 @@ impl<'a, T> RawHrTimerCallback for Pin<&'a T>
         // here.
         let receiver_pin = unsafe { Pin::new_unchecked(receiver_ref) };
 
-        // SAFETY:
-        // - By C API contract `timer_ptr` is the pointer that we passed when queuing the timer, so
-        //   it is a valid pointer to a `HrTimer<T>` embedded in a `T`.
-        // - We are within `RawHrTimerCallback::run`
-        let context = unsafe { HrTimerCallbackContext::from_raw(timer_ptr) };
+        // SAFETY: By C API contract, `expires` is the expiry of the timer
+        // snapshotted under the timer base lock, and timers cannot have
+        // negative expiry times.
+        let expires = unsafe { Instant::from_ktime(expires) };
 
-        T::run(receiver_pin, context).into_c()
+        // SAFETY: By C API contract, `fwd` is valid for writing a
+        // `bindings::hrtimer_forward_args`.
+        unsafe { T::run(receiver_pin, expires).into_c(fwd) }
     }
 }
diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
index 9d9447d4d57e8..04f9d8cbddcd2 100644
--- a/rust/kernel/time/hrtimer/pin_mut.rs
+++ b/rust/kernel/time/hrtimer/pin_mut.rs
@@ -1,9 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 
 use super::{
-    HasHrTimer, HrTimer, HrTimerCallback, HrTimerCallbackContext, HrTimerHandle, HrTimerMode,
-    RawHrTimerCallback, UnsafeHrTimerPointer,
+    HasHrTimer, HrTimer, HrTimerCallback, HrTimerHandle, HrTimerMode, RawHrTimerCallback,
+    UnsafeHrTimerPointer,
 };
+use crate::time::Instant;
 use core::{marker::PhantomData, pin::Pin, ptr::NonNull};
 
 /// A handle for a `Pin<&mut HasHrTimer>`. When the handle exists, the timer might
@@ -85,7 +86,11 @@ impl<'a, T> RawHrTimerCallback for Pin<&'a mut T>
 {
     type CallbackTarget<'b> = Self;
 
-    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+    unsafe extern "C" fn run(
+        ptr: *mut bindings::hrtimer,
+        expires: bindings::ktime_t,
+        fwd: *mut bindings::hrtimer_forward_args,
+    ) -> bindings::hrtimer_restart {
         // `HrTimer` is `repr(C)`
         let timer_ptr = ptr.cast::<HrTimer<T>>();
 
@@ -107,12 +112,13 @@ impl<'a, T> RawHrTimerCallback for Pin<&'a mut T>
         // here.
         let receiver_pin = unsafe { Pin::new_unchecked(receiver_ref) };
 
-        // SAFETY:
-        // - By C API contract `timer_ptr` is the pointer that we passed when queuing the timer, so
-        //   it is a valid pointer to a `HrTimer<T>` embedded in a `T`.
-        // - We are within `RawHrTimerCallback::run`
-        let context = unsafe { HrTimerCallbackContext::from_raw(timer_ptr) };
+        // SAFETY: By C API contract, `expires` is the expiry of the timer
+        // snapshotted under the timer base lock, and timers cannot have
+        // negative expiry times.
+        let expires = unsafe { Instant::from_ktime(expires) };
 
-        T::run(receiver_pin, context).into_c()
+        // SAFETY: By C API contract, `fwd` is valid for writing a
+        // `bindings::hrtimer_forward_args`.
+        unsafe { T::run(receiver_pin, expires).into_c(fwd) }
     }
 }
diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
index aa1ee31a71953..c7f86909e21b5 100644
--- a/rust/kernel/time/hrtimer/tbox.rs
+++ b/rust/kernel/time/hrtimer/tbox.rs
@@ -3,12 +3,12 @@
 use super::HasHrTimer;
 use super::HrTimer;
 use super::HrTimerCallback;
-use super::HrTimerCallbackContext;
 use super::HrTimerHandle;
 use super::HrTimerMode;
 use super::HrTimerPointer;
 use super::RawHrTimerCallback;
 use crate::prelude::*;
+use crate::time::Instant;
 use core::ptr::NonNull;
 
 /// A handle for a [`Box<HasHrTimer<T>>`] returned by a call to
@@ -102,7 +102,11 @@ impl<T, A> RawHrTimerCallback for Pin<Box<T, A>>
 {
     type CallbackTarget<'a> = Pin<&'a mut T>;
 
-    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+    unsafe extern "C" fn run(
+        ptr: *mut bindings::hrtimer,
+        expires: bindings::ktime_t,
+        fwd: *mut bindings::hrtimer_forward_args,
+    ) -> bindings::hrtimer_restart {
         // `HrTimer` is `repr(C)`
         let timer_ptr = ptr.cast::<super::HrTimer<T>>();
 
@@ -120,12 +124,13 @@ impl<T, A> RawHrTimerCallback for Pin<Box<T, A>>
         //   `data_ptr` exist.
         let data_mut_ref = unsafe { Pin::new_unchecked(&mut *data_ptr) };
 
-        // SAFETY:
-        // - By C API contract `timer_ptr` is the pointer that we passed when queuing the timer, so
-        //   it is a valid pointer to a `HrTimer<T>` embedded in a `T`.
-        // - We are within `RawHrTimerCallback::run`
-        let context = unsafe { HrTimerCallbackContext::from_raw(timer_ptr) };
+        // SAFETY: By C API contract, `expires` is the expiry of the timer
+        // snapshotted under the timer base lock, and timers cannot have
+        // negative expiry times.
+        let expires = unsafe { Instant::from_ktime(expires) };
 
-        T::run(data_mut_ref, context).into_c()
+        // SAFETY: By C API contract, `fwd` is valid for writing a
+        // `bindings::hrtimer_forward_args`.
+        unsafe { T::run(data_mut_ref, expires).into_c(fwd) }
     }
 }

-- 
2.51.2

[PATCH 4/6] rust: hrtimer: restrict expires() to exclusive access

[Andreas Hindborg]

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

HrTimer::expires() read node.expires through a volatile load on a
shared reference. The read is unsynchronized: a concurrent start
operation rewrites the expiry under the timer base lock, and the
64-bit load can tear on 32-bit architectures. The volatile idiom
narrows the race but does not remove it.

Change expires() to take Pin<&mut Self>. Wherever an exclusive
reference to the timer is reachable, no start operation can run
concurrently: the timer handles own or borrow the containing object
exclusively for the box and pinned pointer types, and no exclusive
reference is reachable through an Arc. Route the read through
hrtimer_get_expires() via a helper instead of duplicating the field
access on the Rust side, and provide the unsafe expires_unchecked()
for contexts that can guarantee exclusive access by other means.

Reading the expiry from within the timer callback is served by the
expiry snapshot passed to HrTimerCallback::run(), so no callback
context accessor is needed.

Fixes: 4b0147494275 ("rust: hrtimer: Add HrTimer::expires()")
Closes: https://lore.kernel.org/rust-for-linux/87ldi7f4o1.fsf@t14s.mail-host-address-is-not-set/
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Link: https://lore.kernel.org/r/20260813134834.1562995-4-tomo@flapping.org
[ Andreas - Reword commit message and rebase on expiry injection patches. ]
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/helpers/time.c         |  6 ++++++
 rust/kernel/time/hrtimer.rs | 37 +++++++++++++++++++++++--------------
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/rust/helpers/time.c b/rust/helpers/time.c
index 32f4959704939..205a38839532a 100644
--- a/rust/helpers/time.c
+++ b/rust/helpers/time.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/delay.h>
+#include <linux/hrtimer.h>
 #include <linux/ktime.h>
 #include <linux/timekeeping.h>
 
@@ -38,3 +39,8 @@ __rust_helper void rust_helper_udelay(unsigned long usec)
 {
 	udelay(usec);
 }
+
+__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
+{
+	return hrtimer_get_expires(timer);
+}
diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index e6570a6162035..bdb6aaa228396 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -567,27 +567,36 @@ pub fn forward_now(self: Pin<&mut Self>, interval: Delta) -> u64
         self.forward(HrTimerInstant::<T>::now(), interval)
     }
 
+    /// Return the time expiry for this [`HrTimer`].
+    ///
+    /// # Safety
+    ///
+    /// The caller must have exclusive access to `self`.
+    #[inline]
+    unsafe fn expires_unchecked(&self) -> HrTimerInstant<T>
+    where
+        T: HasHrTimer<T>,
+    {
+        // SAFETY:
+        // - The C API requirements for this function are fulfilled by our safety contract.
+        // - Timers cannot have negative `ktime_t` values as their expiration time.
+        unsafe { Instant::from_ktime(bindings::hrtimer_get_expires(Self::raw_get(self))) }
+    }
+
     /// Return the time expiry for this [`HrTimer`].
     ///
     /// This value should only be used as a snapshot, as the actual expiry time could change after
-    /// this function is called.
-    pub fn expires(&self) -> HrTimerInstant<T>
+    /// this function is called. To read the expiry from within the timer callback, use the value
+    /// passed to [`HrTimerCallback::run`] instead.
+    pub fn expires(self: Pin<&mut Self>) -> HrTimerInstant<T>
     where
         T: HasHrTimer<T>,
     {
-        // SAFETY: `self` is an immutable reference and thus always points to a valid `HrTimer`.
-        let c_timer_ptr = unsafe { HrTimer::raw_get(self) };
+        // SAFETY: `expires_unchecked` does not move `Self`.
+        let this = unsafe { self.get_unchecked_mut() };
 
-        // SAFETY:
-        // - Timers cannot have negative ktime_t values as their expiration time.
-        // - There's no actual locking here, a racy read is fine and expected
-        unsafe {
-            Instant::from_ktime(
-                // This `read_volatile` is intended to correspond to a READ_ONCE call.
-                // FIXME(read_once): Replace with `read_once` when available on the Rust side.
-                core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
-            )
-        }
+        // SAFETY: By existence of `Pin<&mut Self>`, we have exclusive access to `Self`.
+        unsafe { this.expires_unchecked() }
     }
 }
 

-- 
2.51.2

[PATCH 5/6] rust: hrtimer: document deadlock when starting a timer in its handler

[Andreas Hindborg]

The state machine documentation states that for pointer types that
implement `Clone`, the `start` operation may be issued while the timer
is in the **running** state, and that it is then equivalent to the
`restart` operation. That only holds when the operation is issued from
outside the timer handler.

The `start` operation returns a `HrTimerHandle`, and dropping the
handle cancels the timer with `hrtimer_cancel()`, which blocks until a
running handler has returned. When `start` is issued from within the
handler, the handle is also dropped within the handler, so the cancel
waits for the very handler that issues it, and the handler deadlocks.

Note this in the state machine documentation.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index bdb6aaa22839..2a9abc9f5d8c 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -78,6 +78,8 @@
 //! handler returns, and a restart requested by the return value of the handler is discarded in
 //! favor of the `restart` operation.
 //!
+//! ⚠️ Issuing the `start` operation from within the timer handler will lead to deadlock.
+//!
 //! # Examples
 //!
 //! ## Using an intrusive timer living in a [`Box`]

-- 
2.51.2

[PATCH 6/6] rust: hrtimer: Make HrTimer repr(transparent)

[Andreas Hindborg]

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

HrTimerCallbackContext acquires a &HrTimer<T> from a
NonNull<HrTimer<T>> while a &mut HrTimer<T> can exist at the same
time. This is sound only because HrTimer's sole field is
Opaque<bindings::hrtimer>, which puts every byte behind an UnsafeCell.
Adding a field to HrTimer that is not Opaque would make acquiring that
shared reference unsound.

Make HrTimer repr(transparent), which prevents multiple fields, so that
such a refactor fails to compile instead of silently introducing
unsoundness. This does not guarantee the remaining field stays behind
Opaque, but it rules out the likely way of getting there.

repr(transparent) cannot be combined with repr(C), so drop the latter.

Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Link: https://msgid.link/20260813134834.1562995-5-tomo@flapping.org
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs         | 6 +++++-
 rust/kernel/time/hrtimer/arc.rs     | 2 +-
 rust/kernel/time/hrtimer/pin.rs     | 2 +-
 rust/kernel/time/hrtimer/pin_mut.rs | 2 +-
 rust/kernel/time/hrtimer/tbox.rs    | 2 +-
 5 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 2a9abc9f5d8c..ab7c568b8855 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -427,8 +427,12 @@
 /// # Invariants
 ///
 /// * `self.timer` is initialized by `bindings::hrtimer_setup_ext`.
+// `repr(transparent)` is not merely about layout. `HrTimerCallbackContext` acquires a
+// `&HrTimer<T>` while a `&mut HrTimer<T>` may exist, which is sound only because every byte of
+// this type sits inside `Opaque`. Being transparent rejects a second field at compile time,
+// but it does not enforce that the remaining field stays `Opaque`.
 #[pin_data]
-#[repr(C)]
+#[repr(transparent)]
 pub struct HrTimer<T> {
     #[pin]
     timer: Opaque<bindings::hrtimer>,
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
index 8a9fcb5c69e6..46ccff9e0024 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -84,7 +84,7 @@ impl<T> RawHrTimerCallback for Arc<T>
         expires: bindings::ktime_t,
         fwd: *mut bindings::hrtimer_forward_args,
     ) -> bindings::hrtimer_restart {
-        // `HrTimer` is `repr(C)`
+        // `HrTimer` is `repr(transparent)`
         let timer_ptr = ptr.cast::<super::HrTimer<T>>();
 
         // SAFETY: By C API contract `ptr` is the pointer we passed when
diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
index d1143f278f31..5fd374fdc480 100644
--- a/rust/kernel/time/hrtimer/pin.rs
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -87,7 +87,7 @@ impl<'a, T> RawHrTimerCallback for Pin<&'a T>
         expires: bindings::ktime_t,
         fwd: *mut bindings::hrtimer_forward_args,
     ) -> bindings::hrtimer_restart {
-        // `HrTimer` is `repr(C)`
+        // `HrTimer` is `repr(transparent)`
         let timer_ptr = ptr.cast::<HrTimer<T>>();
 
         // SAFETY: By the safety requirement of this function, `timer_ptr`
diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
index 04f9d8cbddcd..2bba3c41d6e9 100644
--- a/rust/kernel/time/hrtimer/pin_mut.rs
+++ b/rust/kernel/time/hrtimer/pin_mut.rs
@@ -91,7 +91,7 @@ impl<'a, T> RawHrTimerCallback for Pin<&'a mut T>
         expires: bindings::ktime_t,
         fwd: *mut bindings::hrtimer_forward_args,
     ) -> bindings::hrtimer_restart {
-        // `HrTimer` is `repr(C)`
+        // `HrTimer` is `repr(transparent)`
         let timer_ptr = ptr.cast::<HrTimer<T>>();
 
         // SAFETY: By the safety requirement of this function, `timer_ptr`
diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
index c7f86909e21b..399ad7677043 100644
--- a/rust/kernel/time/hrtimer/tbox.rs
+++ b/rust/kernel/time/hrtimer/tbox.rs
@@ -107,7 +107,7 @@ impl<T, A> RawHrTimerCallback for Pin<Box<T, A>>
         expires: bindings::ktime_t,
         fwd: *mut bindings::hrtimer_forward_args,
     ) -> bindings::hrtimer_restart {
-        // `HrTimer` is `repr(C)`
+        // `HrTimer` is `repr(transparent)`
         let timer_ptr = ptr.cast::<super::HrTimer<T>>();
 
         // SAFETY: By C API contract `ptr` is the pointer we passed when

-- 
2.51.2

Re: [PATCH 5/6] rust: hrtimer: document deadlock when starting a timer in its handler

[Gary Guo]

On Tue Aug 25, 2026 at 1:16 PM BST, Andreas Hindborg wrote:
> The state machine documentation states that for pointer types that
> implement `Clone`, the `start` operation may be issued while the timer
> is in the **running** state, and that it is then equivalent to the
> `restart` operation. That only holds when the operation is issued from
> outside the timer handler.
>
> The `start` operation returns a `HrTimerHandle`, and dropping the
> handle cancels the timer with `hrtimer_cancel()`, which blocks until a
> running handler has returned. When `start` is issued from within the
> handler, the handle is also dropped within the handler, so the cancel
> waits for the very handler that issues it, and the handler deadlocks.

That's not always true, you can start timer and store its handle elsewhere.

I think the proper wording is that "cancelling a timer from within the timer
handler will lead to deadlock".

Best,
Gary

>
> Note this in the state machine documentation.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time/hrtimer.rs | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index bdb6aaa22839..2a9abc9f5d8c 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -78,6 +78,8 @@
>  //! handler returns, and a restart requested by the return value of the handler is discarded in
>  //! favor of the `restart` operation.
>  //!
> +//! ⚠️ Issuing the `start` operation from within the timer handler will lead to deadlock.
> +//!
>  //! # Examples
>  //!
>  //! ## Using an intrusive timer living in a [`Box`]

Re: [PATCH 6/6] rust: hrtimer: Make HrTimer repr(transparent)

[Gary Guo]

On Tue Aug 25, 2026 at 1:16 PM BST, Andreas Hindborg wrote:
> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>
> HrTimerCallbackContext acquires a &HrTimer<T> from a
> NonNull<HrTimer<T>> while a &mut HrTimer<T> can exist at the same
> time. This is sound only because HrTimer's sole field is
> Opaque<bindings::hrtimer>, which puts every byte behind an UnsafeCell.
> Adding a field to HrTimer that is not Opaque would make acquiring that
> shared reference unsound.

HrTimerCallbackContext is removed in patch 4, though?

Still worh preferring `#[repr(transparent)]` over `#[repr(C)]`, but the
motivation should be reworded and the comment on `HrTimer` should be removed.

Best,
Gary

>
> Make HrTimer repr(transparent), which prevents multiple fields, so that
> such a refactor fails to compile instead of silently introducing
> unsoundness. This does not guarantee the remaining field stays behind
> Opaque, but it rules out the likely way of getting there.
>
> repr(transparent) cannot be combined with repr(C), so drop the latter.
>
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Link: https://msgid.link/20260813134834.1562995-5-tomo@flapping.org
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time/hrtimer.rs         | 6 +++++-
>  rust/kernel/time/hrtimer/arc.rs     | 2 +-
>  rust/kernel/time/hrtimer/pin.rs     | 2 +-
>  rust/kernel/time/hrtimer/pin_mut.rs | 2 +-
>  rust/kernel/time/hrtimer/tbox.rs    | 2 +-
>  5 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index 2a9abc9f5d8c..ab7c568b8855 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -427,8 +427,12 @@
>  /// # Invariants
>  ///
>  /// * `self.timer` is initialized by `bindings::hrtimer_setup_ext`.
> +// `repr(transparent)` is not merely about layout. `HrTimerCallbackContext` acquires a
> +// `&HrTimer<T>` while a `&mut HrTimer<T>` may exist, which is sound only because every byte of
> +// this type sits inside `Opaque`. Being transparent rejects a second field at compile time,
> +// but it does not enforce that the remaining field stays `Opaque`.
>  #[pin_data]
> -#[repr(C)]
> +#[repr(transparent)]
>  pub struct HrTimer<T> {
>      #[pin]
>      timer: Opaque<bindings::hrtimer>,

Re: [PATCH 6/6] rust: hrtimer: Make HrTimer repr(transparent)

[Andreas Hindborg]

"Gary Guo" <gary@garyguo.net> writes:

> On Tue Aug 25, 2026 at 1:16 PM BST, Andreas Hindborg wrote:
>> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>
>> HrTimerCallbackContext acquires a &HrTimer<T> from a
>> NonNull<HrTimer<T>> while a &mut HrTimer<T> can exist at the same
>> time. This is sound only because HrTimer's sole field is
>> Opaque<bindings::hrtimer>, which puts every byte behind an UnsafeCell.
>> Adding a field to HrTimer that is not Opaque would make acquiring that
>> shared reference unsound.
>
> HrTimerCallbackContext is removed in patch 4, though?
>
> Still worh preferring `#[repr(transparent)]` over `#[repr(C)]`, but the
> motivation should be reworded and the comment on `HrTimer` should be removed.

Yes, I was a little too fast when I added this patch. I'd like to keep
it as well, but it needs some edits.

Best regards,
Andreas Hindborg

Re: [PATCH 5/6] rust: hrtimer: document deadlock when starting a timer in its handler

[Andreas Hindborg]

"Gary Guo" <gary@garyguo.net> writes:

> On Tue Aug 25, 2026 at 1:16 PM BST, Andreas Hindborg wrote:
>> The state machine documentation states that for pointer types that
>> implement `Clone`, the `start` operation may be issued while the timer
>> is in the **running** state, and that it is then equivalent to the
>> `restart` operation. That only holds when the operation is issued from
>> outside the timer handler.
>>
>> The `start` operation returns a `HrTimerHandle`, and dropping the
>> handle cancels the timer with `hrtimer_cancel()`, which blocks until a
>> running handler has returned. When `start` is issued from within the
>> handler, the handle is also dropped within the handler, so the cancel
>> waits for the very handler that issues it, and the handler deadlocks.
>
> That's not always true, you can start timer and store its handle elsewhere.
>
> I think the proper wording is that "cancelling a timer from within the timer
> handler will lead to deadlock".

Right, I'll update the wording and comment.


Best regards,
Andreas Hindborg

Re: [PATCH 1/6] hrtimer: add expiry injecting callback variant

[Thomas Gleixner]

On Tue, Aug 25 2026 at 14:16, Andreas Hindborg wrote:
> A hrtimer callback may modify its own expiry with hrtimer_forward()
> and read it with hrtimer_get_expires(). Both run after
> __run_hrtimer() has dropped the cpu_base->lock, so they race with a
> concurrent hrtimer_start_range_ns() on another CPU, which rewrites
> node.expires under the base lock and requeues the timer:
>
> - The unlocked read-modify-write of node.expires in
>   hrtimer_forward() is a data race against the locked write in the
>   start path.
>
> - The is_queued check in hrtimer_forward() is racy with
>   a time-of-check-time-of-use bug as well: a concurrent
>   start can enqueue the timer between the check and the expiry
>   update, and forwarding an already queued timer changes the expiry
>   of a node inside the timerqueue without re-sorting, leaving the
>   tree unordered.
>
> Timer users which both forward in the callback and arm from other contexts
> must provide their own serialization, e.g. perf's cpc->hrtimer_lock plus
> hrtimer_active flag, see commit 4cfafd3082af ("sched,perf: Fix periodic
> timers"). The requirement is subtle and not enforced; i915_pmu and taprio
> currently get it wrong. For the Rust hrtimer abstraction it is a soundness
> problem: safe code can arm a timer whose callback is running, so callback
> context forward and expiry reads cannot be offered as safe API.
>
> Add an alternative callback variant that removes the race
> structurally instead of requiring serialization. An expiry injecting
> callback receives the expiry snapshotted under the base lock by
> value and, to restart the timer, fills a struct hrtimer_forward_args
> and returns HRTIMER_RESTART. __run_hrtimer() then applies the
> forward and the enqueue with the base lock held. The callback never
> accesses live timer state.
>
> If a concurrent start enqueued the timer while the callback ran, the
> restart request is discarded and the start wins, matching the
> existing "restart == HRTIMER_RESTART && !timer->is_queued" handling
> for classic callbacks. The is_queued check is reliable here: while
> base->running == timer, hrtimer_try_to_cancel() bails out before
> remove_hrtimer() and the timer cannot switch bases, so only a
> concurrent start can enqueue it, and the start path writes the
> expiry and is_queued in the same critical section. Thus !is_queued
> at requeue time guarantees the expiry still equals the snapshot
> handed to the callback: the deferred hrtimer_forward() cannot hit
> its concurrent start check, and an overrun count the callback
> derived from the snapshot is consistent with the forward that is
> applied.
>
> The new callback pointer shares storage with the classic one in an
> anonymous union, discriminated by a new is_ext flag placed in
> existing padding; sizeof(struct hrtimer) is unchanged and the
> classic callback path is unaffected. hrtimer_update_function()
> rejects timers with an expiry injecting callback.

Aside of the horrible name (what is "ext"?) the whole mechanism is
creating a false sense of "safety" as the same problem exists when the
timer callback simply sets the new expiry time without invoking
hrtimer_forward().

Also why is Rust special and wants to delegate the external
serialization requirement to the hrtimer core code and thereby adding a
boatload of extra conditionals into the hotpath to distinguish between
the two callback variants?

Yes, you found an example for the downside of this design in the i915
code. That's not surprising because i915 is generally known for ignoring
documentation and making up their own rules just because they can. So
I'm not accepting this as an argument at all.

I agree with you that the lack of enforcement of that external
serialization rule is suboptimal. External serialization is hard to
validate/enforce in general though it's not impossible.

But let's first take a step back and look at the larger picture.

The obvious question is:

   Why is the base lock dropped when invoking the callback?

The answer is simply that the callback might and in many cases will
acquire a lock which is used in the reverse lock order for the purpose
of external serialization.

If you think about that then it's pretty obvious that you can introduce
a safe variant of hrtimer_forward() which can be invoked from the
callback:

hrtimer_forward_safe_from_callback(timer)
{
        base = lock_running_timer_base(timer);
        if (!hrtimer_is_queued(timer))
        	hrtimer_forward(timer);
        unlock_timer_base(base);
}

That prevents the scenario you described in a completely safe way, no?

Coming back to validation/enforcement of external serialization. That's
a problem which has been solved in other places already. Look at the
seqlock code for inspiration.

Thanks,

        tglx

Re: [PATCH 1/6] hrtimer: add expiry injecting callback variant

[Gary Guo]

On Tue Sep 29, 2026 at 9:56 PM BST, Thomas Gleixner wrote:
> On Tue, Aug 25 2026 at 14:16, Andreas Hindborg wrote:
>> A hrtimer callback may modify its own expiry with hrtimer_forward()
>> and read it with hrtimer_get_expires(). Both run after
>> __run_hrtimer() has dropped the cpu_base->lock, so they race with a
>> concurrent hrtimer_start_range_ns() on another CPU, which rewrites
>> node.expires under the base lock and requeues the timer:
>>
>> - The unlocked read-modify-write of node.expires in
>>   hrtimer_forward() is a data race against the locked write in the
>>   start path.
>>
>> - The is_queued check in hrtimer_forward() is racy with
>>   a time-of-check-time-of-use bug as well: a concurrent
>>   start can enqueue the timer between the check and the expiry
>>   update, and forwarding an already queued timer changes the expiry
>>   of a node inside the timerqueue without re-sorting, leaving the
>>   tree unordered.
>>
>> Timer users which both forward in the callback and arm from other contexts
>> must provide their own serialization, e.g. perf's cpc->hrtimer_lock plus
>> hrtimer_active flag, see commit 4cfafd3082af ("sched,perf: Fix periodic
>> timers"). The requirement is subtle and not enforced; i915_pmu and taprio
>> currently get it wrong. For the Rust hrtimer abstraction it is a soundness
>> problem: safe code can arm a timer whose callback is running, so callback
>> context forward and expiry reads cannot be offered as safe API.
>>
>> Add an alternative callback variant that removes the race
>> structurally instead of requiring serialization. An expiry injecting
>> callback receives the expiry snapshotted under the base lock by
>> value and, to restart the timer, fills a struct hrtimer_forward_args
>> and returns HRTIMER_RESTART. __run_hrtimer() then applies the
>> forward and the enqueue with the base lock held. The callback never
>> accesses live timer state.
>>
>> If a concurrent start enqueued the timer while the callback ran, the
>> restart request is discarded and the start wins, matching the
>> existing "restart == HRTIMER_RESTART && !timer->is_queued" handling
>> for classic callbacks. The is_queued check is reliable here: while
>> base->running == timer, hrtimer_try_to_cancel() bails out before
>> remove_hrtimer() and the timer cannot switch bases, so only a
>> concurrent start can enqueue it, and the start path writes the
>> expiry and is_queued in the same critical section. Thus !is_queued
>> at requeue time guarantees the expiry still equals the snapshot
>> handed to the callback: the deferred hrtimer_forward() cannot hit
>> its concurrent start check, and an overrun count the callback
>> derived from the snapshot is consistent with the forward that is
>> applied.
>>
>> The new callback pointer shares storage with the classic one in an
>> anonymous union, discriminated by a new is_ext flag placed in
>> existing padding; sizeof(struct hrtimer) is unchanged and the
>> classic callback path is unaffected. hrtimer_update_function()
>> rejects timers with an expiry injecting callback.
>
> Aside of the horrible name (what is "ext"?) the whole mechanism is
> creating a false sense of "safety" as the same problem exists when the
> timer callback simply sets the new expiry time without invoking
> hrtimer_forward().
>
> Also why is Rust special and wants to delegate the external
> serialization requirement to the hrtimer core code and thereby adding a
> boatload of extra conditionals into the hotpath to distinguish between
> the two callback variants?
>
> Yes, you found an example for the downside of this design in the i915
> code. That's not surprising because i915 is generally known for ignoring
> documentation and making up their own rules just because they can. So
> I'm not accepting this as an argument at all.
>
> I agree with you that the lack of enforcement of that external
> serialization rule is suboptimal. External serialization is hard to
> validate/enforce in general though it's not impossible.
>
> But let's first take a step back and look at the larger picture.
>
> The obvious question is:
>
>    Why is the base lock dropped when invoking the callback?
>
> The answer is simply that the callback might and in many cases will
> acquire a lock which is used in the reverse lock order for the purpose
> of external serialization.
>
> If you think about that then it's pretty obvious that you can introduce
> a safe variant of hrtimer_forward() which can be invoked from the
> callback:
>
> hrtimer_forward_safe_from_callback(timer)
> {
>         base = lock_running_timer_base(timer);
>         if (!hrtimer_is_queued(timer))
>         	hrtimer_forward(timer);
>         unlock_timer_base(base);
> }

FWIW we did discuss about this option. Here's the list of all options that we
come up with:

1. Take base lock before calling forward and release it afterward (the one you
   mentioned above). This one requires exposing the base lock (at least to Rust
   abstraction).

2. Prevent timer operation from within the callback and have hrtimer core doing
   it (Andreas's patch).

3. Force users to stop the timer before re-arming. This was dismissed because
   stopping the timer will require waiting for the callback, and this is prone to
   deadlock condition.

4. Add a lock to Rust side that all users must take to forward / start. We don't
   want to add a new lock just for this, and taking base lock would be more
   favourable.

We consider (2) the best option because it avoids several pitfalls with (1):

- It is impossible to forget to forward and return restart, or forward but
  return norestart.

- The base lock is not unlocked before relocked, so the forward and restarting
  is atomic. So it's impossible to have the condition where the forward occurs,
  but before returning RESTART, the hrtimer is concurrently queued.

- Unlock/relock have an overhead (we didn't quantify how much impact this will
  be, though).

I supposed another alternative is to add a mode in hrtimer core where the base
lock is not unlocked before calling the callback, and expose base lock APIs
(like option 1) so that the Rust hrtimer abstraction can unlock it before
calling the callbacks.

Best,
Gary

>
> That prevents the scenario you described in a completely safe way, no?
>
> Coming back to validation/enforcement of external serialization. That's
> a problem which has been solved in other places already. Look at the
> seqlock code for inspiration.

Re: [PATCH 1/6] hrtimer: add expiry injecting callback variant

[Thomas Gleixner]

Gary!

On Wed, Sep 30 2026 at 14:14, Gary Guo wrote:
> On Tue Sep 29, 2026 at 9:56 PM BST, Thomas Gleixner wrote:
>> hrtimer_forward_safe_from_callback(timer)
>> {
>>         base = lock_running_timer_base(timer);
>>         if (!hrtimer_is_queued(timer))
>>         	hrtimer_forward(timer);
>>         unlock_timer_base(base);
>> }
>
> FWIW we did discuss about this option. Here's the list of all options that we
> come up with:
>
> 1. Take base lock before calling forward and release it afterward (the one you
>    mentioned above). This one requires exposing the base lock (at least to Rust
>    abstraction).

Why?

With the above C core function Rust does not even know that the
base lock exists. Rust invokes the function and relies on the guarantees
it provides.

And the function is not Rust specific at all. You can use it to paper
over the i915 bugs too, no?

> 2. Prevent timer operation from within the callback and have hrtimer core doing
>    it (Andreas's patch).

Which adds overhead into the hotpath and creates yet another weird
"scratch my itch" API.

> 3. Force users to stop the timer before re-arming. This was dismissed because
>    stopping the timer will require waiting for the callback, and this is prone to
>    deadlock condition.

Obviously and that's one of the reasons that the code is implemented the
way it is, which puts some reasonable responsibility to the users for
the sake of simplicity and performance.

Now you want to reverse that and add overhead and complexity to the core
to cater for the potential stupidity of users without even solving _all_
related problems:

   You again forgot that simply setting the new expiry time from the
   callback without invoking forward() has exactly the same issue.

Maybe you can prevent that on the Rust side, but the C side still allows
that so your magic new callback is just providing a false sense of safety.

> 4. Add a lock to Rust side that all users must take to forward / start. We don't
>    want to add a new lock just for this, and taking base lock would be more
>    favourable.

There is a reason why it is documented that this needs external
serialization.

Neither this magic inject callback variant nor what I proposed solve the
underlying problem of two competing contexts which try to rearm the
timer to a context dependent expiry time.

All they can do is prevent inconsistent state, but the price to pay in
terms of overhead and complexity are very different. And as I said above
neither one of them solves the 'set expiry directly from the callback'
problem.

> We consider (2) the best option because it avoids several pitfalls with (1):
>
> - It is impossible to forget to forward and return restart, or forward but
>   return norestart.

Forward and forget to return RESTART is harmless. All what happens is that the
timer won't fire so some device won't work as expected. There are a
gazillion of other ways to achieve the same result.

Forget forward and return RESTART will result in a hrtimer interrupt
overrun warning and if you fail to figure that out when implementing
your callback then you (and the AI you are relying on) should go and
resort to HTML coding.

> - The base lock is not unlocked before relocked, so the forward and restarting

What means unlocked before relocked?

>   is atomic. So it's impossible to have the condition where the forward occurs,
>   but before returning RESTART, the hrtimer is concurrently queued.

What's the problem with that?

If the callback invokes hrtimer_forward_safe_from_callback() then you
have these scenarios:

#A
        callback()
          safe_forward()
            lock()
            if (!queued)  // true
               forward()
            unlock()
                                        lock()
                                        start()
                                        unlock()
            return RESTART
          lock()
          if (!queued)    // false
             start()
          unlock()

#B
        callback()
          safe_forward()
                                        lock()
                                        start()
                                        unlock()
            lock()
            if (!queued)  // false
               forward()
            unlock()
            return RESTART

          lock()
          if (!queued)    // true
             start()
          unlock()

In both cases the external start() wins.

Sure in #A it overwrites the forward result, but that's what the user
asked for and it is not at all different from this sequence:

        callback()
          safe_forward()
            lock()
            if (!queued)  // true
               forward()
            unlock()
            return RESTART
          lock()
          if (!queued)    // true
             start()
          unlock()
                                        lock()
                                        start()
                                        unlock()

IOW, you cannot prevent the forward result from being overwritten and it
does not matter at all whether the overwrite happens before or after the
callback returned RESTART. The outcome is exactly the same. No?

And no, checking hrtimer_is_queued() before invoking the start sequence
does not prevent that simply because that check is lockless:

        callback()
          safe_forward()
            lock()
            if (!queued)  // true
               forward()
            unlock()
            return RESTART
          lock()
          if (!queued)    // true	if (!queued)      // true
             start()
          unlock()
                                          lock()
                                          start()
                                          unlock()

Even if it would be protected by the base lock it would result in a
temporary state which can be invalid when invoking the function:

        callback()
          safe_forward()
            lock()
            if (!queued) // true
               forward()
            unlock()
            return RESTART
            				lock()
            				cond = !queued  // true
                                        unlock()
          lock()
          if (!queued)  // true	      
             start()
          unlock()
          				if (cond)       // true
                                          lock()
                                          start()
                                          unlock()

No?

To prevent that you'd need a start_if_not_queued() function:

        callback()
          safe_forward()
            lock()
            if (!queued) // true
               forward()
            unlock()
            return RESTART
          lock()
          if (!queued)  // true	      
             start()
          unlock()
                                        lock()
          				if (!queued)    // false
                                          start()
                                        unlock()

Even that would not solve all possible problems either. That's an
application problem. The core can only provide tools to avoid damage but
it cannot prevent application logic bugs at all.

Adding complexity to prevent that has been pointed out to be the wrong
solution by Dijkstra long ago:

  "Complexity breeds bugs. Simplicity is the prerequisite for
   reliability."

Don't get me wrong. I'm a great fan of Rust, but I have a background in
ADA programming (admittedly from decades ago) and I studied the
limitations of provided safety measures in practice enough to know that
Dijkstra is absolutely right. There is a reason why ADA grew a massive
static analysis toolset around it which is sadly not easily exploitable
due to the design decisions of Rust which borrowed a lot of the
incomplete ADA concepts... But that's a different discussion to have.

> - Unlock/relock have an overhead (we didn't quantify how much impact this will
>   be, though).

The extra lock operations won't be measurable except for situations
which have high lock contention on the base lock independent of the
problematic scenario. If that's the case then the extra lock/unlock pair
will just add to the noise.

> I supposed another alternative is to add a mode in hrtimer core where the base
> lock is not unlocked before calling the callback, and expose base lock APIs
> (like option 1) so that the Rust hrtimer abstraction can unlock it before
> calling the callbacks.

That's even worse and Option 1, i.e. the core function I proposed is
_NOT_ exposing base lock at all.

So what's your actual argument that you can't build a "safe" Rust API
around this?

Thanks,

        tglx

Re: [PATCH 1/6] hrtimer: add expiry injecting callback variant

[Gary Guo]

On Wed Sep 30, 2026 at 10:43 PM BST, Thomas Gleixner wrote:
> Gary!
>
> On Wed, Sep 30 2026 at 14:14, Gary Guo wrote:
>> On Tue Sep 29, 2026 at 9:56 PM BST, Thomas Gleixner wrote:
>>> hrtimer_forward_safe_from_callback(timer)
>>> {
>>>         base = lock_running_timer_base(timer);
>>>         if (!hrtimer_is_queued(timer))
>>>         	hrtimer_forward(timer);
>>>         unlock_timer_base(base);
>>> }
>>
>> FWIW we did discuss about this option. Here's the list of all options that we
>> come up with:
>>
>> 1. Take base lock before calling forward and release it afterward (the one you
>>    mentioned above). This one requires exposing the base lock (at least to Rust
>>    abstraction).
>
> Why?
>
> With the above C core function Rust does not even know that the
> base lock exists. Rust invokes the function and relies on the guarantees
> it provides.

Sure, if all expiry read/update functions have their _safe_from_callback
variant.

>
> And the function is not Rust specific at all. You can use it to paper
> over the i915 bugs too, no?
>
>> 2. Prevent timer operation from within the callback and have hrtimer core doing
>>    it (Andreas's patch).
>
> Which adds overhead into the hotpath and creates yet another weird
> "scratch my itch" API.
>
>> 3. Force users to stop the timer before re-arming. This was dismissed because
>>    stopping the timer will require waiting for the callback, and this is prone to
>>    deadlock condition.
>
> Obviously and that's one of the reasons that the code is implemented the
> way it is, which puts some reasonable responsibility to the users for
> the sake of simplicity and performance.
>
> Now you want to reverse that and add overhead and complexity to the core
> to cater for the potential stupidity of users without even solving _all_
> related problems:

It's not our initiative to reverse that design. That was changed long time ago:
https://lore.kernel.org/all/tip-5de2755c8c8b3a6b8414870e2c284914a2b42e4d@git.kernel.org/

Enforcing this would be more favourable to Rust API's design. FUJITA proposed an
API which would require user to cancel the timer first before re-arming.

The issue is that for users that do want a concurrent restart, like in perf
core's use case, special care would need to be taken to avoid deadlock, as
cancellation code path must not have any lock held that is shared with the
callback.

>
>    You again forgot that simply setting the new expiry time from the
>    callback without invoking forward() has exactly the same issue.
>
> Maybe you can prevent that on the Rust side, but the C side still allows
> that so your magic new callback is just providing a false sense of safety.

That is indeed something we didn't consider, as Rust abstraction currently does
not provide a way to do it. We could change the forward return value to just be
the new expire time.

    typedef enum hrtimer_restart (*hrtimer_ext_func_t)(struct hrtimer *timer, ktime_t *expires);

I've kept Andreas's name to not have to bikeshed on it.

With such callbacks, users would be abel to use a helper like:

    u64 hrtimer_compute_forward(ktime_t *expires, ktime_t now, ktime_t interval)

to forward the timer.

Alternatively, instead of putting this into the callback signature, struct
hrtimer could stores an additional expires_for_callback field which gets updated
from callbacks. I do think existing C callbacks can be converted to use such API
without too much churn.

>
>> 4. Add a lock to Rust side that all users must take to forward / start. We don't
>>    want to add a new lock just for this, and taking base lock would be more
>>    favourable.
>
> There is a reason why it is documented that this needs external
> serialization.
>
> Neither this magic inject callback variant nor what I proposed solve the
> underlying problem of two competing contexts which try to rearm the
> timer to a context dependent expiry time.
>
> All they can do is prevent inconsistent state, but the price to pay in
> terms of overhead and complexity are very different. And as I said above
> neither one of them solves the 'set expiry directly from the callback'
> problem.
>
>> We consider (2) the best option because it avoids several pitfalls with (1):
>>
>> - It is impossible to forget to forward and return restart, or forward but
>>   return norestart.
>
> Forward and forget to return RESTART is harmless. All what happens is that the
> timer won't fire so some device won't work as expected. There are a
> gazillion of other ways to achieve the same result.
>
> Forget forward and return RESTART will result in a hrtimer interrupt
> overrun warning and if you fail to figure that out when implementing
> your callback then you (and the AI you are relying on) should go and
> resort to HTML coding.
>
>> - The base lock is not unlocked before relocked, so the forward and restarting
>
> What means unlocked before relocked?

It's the example that you've given. If one calls
hrtimer_forward_safe_from_callback and then immediately return RESTART, it would
unlock the base lock within hrtimer_forward_safe_from_callback, and upon
returning hrtimer core relocks the base lock.

>
>>   is atomic. So it's impossible to have the condition where the forward occurs,
>>   but before returning RESTART, the hrtimer is concurrently queued.
>
> What's the problem with that?
>
> [snip]
>
> To prevent that you'd need a start_if_not_queued() function:
>
> [snip]
>
> Even that would not solve all possible problems either. That's an
> application problem. The core can only provide tools to avoid damage but
> it cannot prevent application logic bugs at all.

Ack. FWIW we're not trying to prevent all application logic bug. It's not
possible. But what's important that we try to achieve is for a user bug to be
contained and not cause hrtimer core to complete breakdown.

A buggy hrtimer user that never refires? If we can somehow prevent that, great,
but that's not required.

A buggy hrtimer user that breaks the timer wheel completely by making its data
structure internally inconsistent? That's what we're trying to prevent, at
least for Rust users. As it currently stands, an badly timed hrtimer_forward (or
hrtimer_set_expires) within callback can cause hrtimer core rbtree to violate
its invariant. And that's what we want to avoid.

> Adding complexity to prevent that has been pointed out to be the wrong
> solution by Dijkstra long ago:
>
>   "Complexity breeds bugs. Simplicity is the prerequisite for
>    reliability."

It's a trade-off, really. You can have simple code and complex rules, that's one
way of complexity. More complex code and simpler rule is a different way of
complexity.

The way I see this is that hrtimer code will be less complex to reason about if
expires modification is not possible without the base lock held.

- "timer->expires may only be touched with base lock held" is very simple rule,
  and yes the code would need some slight additional complexity.

- "timer->expires" may be touched without the base lock held, if the hrtimer is
  not queued, and no code can possibly queue it while it is touched." results in
  simpler code, but the rule is more complex, and it's _non-local_ meaning that
  the callback code may look innocent, and all it takes is a hrtimer_start from
  outside the callback to mess things up.

> Don't get me wrong. I'm a great fan of Rust, but I have a background in
> ADA programming (admittedly from decades ago) and I studied the
> limitations of provided safety measures in practice enough to know that
> Dijkstra is absolutely right. There is a reason why ADA grew a massive
> static analysis toolset around it which is sadly not easily exploitable
> due to the design decisions of Rust which borrowed a lot of the
> incomplete ADA concepts... But that's a different discussion to have.
>
>> - Unlock/relock have an overhead (we didn't quantify how much impact this will
>>   be, though).
>
> The extra lock operations won't be measurable except for situations
> which have high lock contention on the base lock independent of the
> problematic scenario. If that's the case then the extra lock/unlock pair
> will just add to the noise.

Ack.

I shall add that this is more than a single lock/unlock. hrtimer_get_expires
would need to take that lock as well.

>
>> I supposed another alternative is to add a mode in hrtimer core where the base
>> lock is not unlocked before calling the callback, and expose base lock APIs
>> (like option 1) so that the Rust hrtimer abstraction can unlock it before
>> calling the callbacks.
>
> That's even worse and Option 1, i.e. the core function I proposed is
> _NOT_ exposing base lock at all.
>
> So what's your actual argument that you can't build a "safe" Rust API
> around this?

Sure, if all expiry read/update functions have their _safe_from_callback
variant.

Best,
Gary

>
> Thanks,
>
>         tglx