[PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

[PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

[FUJITA Tomonori]

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

HrTimer::expires() previously read node.expires via a volatile load, which
can race with C-side updates. Rework the API so it is only callable with
exclusive access or from the callback context.

Introduce expires_unchecked() with an explicit safety contract, switch
HrTimer::expires() to Pin<&mut Self>, add
HrTimerCallbackContext::expires(), and route the read through
hrtimer_get_expires() via a Rust helper.

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

Note that I will send a separate patch to remove
HrTimerCallbackContext after the discussion concludes.

v3:
- Change the signature of raw_expires()
- Rename raw_expires() to expires_unchecked()
v2: https://lore.kernel.org/rust-for-linux/20260224132507.315637-1-tomo@aliasing.net/
- Add Fixes and Closes tags
- Fix and improve comments
v1: https://lore.kernel.org/rust-for-linux/20260110115838.3109895-1-fujita.tomonori@gmail.com/
---
 rust/helpers/time.c         |  6 +++++
 rust/kernel/time/hrtimer.rs | 46 ++++++++++++++++++++++++++-----------
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/rust/helpers/time.c b/rust/helpers/time.c
index 32f495970493..ef8999621399 100644
--- a/rust/helpers/time.c
+++ b/rust/helpers/time.c
@@ -2,6 +2,7 @@
 
 #include <linux/delay.h>
 #include <linux/ktime.h>
+#include <linux/hrtimer.h>
 #include <linux/timekeeping.h>
 
 __rust_helper void rust_helper_fsleep(unsigned long usecs)
@@ -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 856d2d929a00..a4aea06e2c2d 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -224,27 +224,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 either have exclusive access to `self`, or be within the context of the
+    /// timer callback.
+    #[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>
+    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() }
     }
 }
 
@@ -729,6 +738,17 @@ pub fn forward(&mut self, now: HrTimerInstant<T>, interval: Delta) -> u64 {
     pub fn forward_now(&mut self, duration: Delta) -> u64 {
         self.forward(HrTimerInstant::<T>::now(), duration)
     }
+
+    /// Return the time expiry for the timer.
+    ///
+    /// This function is identical to [`HrTimer::expires()`] except that it may only be used from
+    /// within the context of a [`HrTimer`] callback.
+    pub fn expires(&self) -> HrTimerInstant<T> {
+        // 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 { self.0.as_ref().expires_unchecked() }
+    }
 }
 
 /// Use to implement the [`HasHrTimer<T>`] trait.

base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
-- 
2.43.0

Re: [PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

[Andreas Hindborg]

"FUJITA Tomonori" <tomo@aliasing.net> writes:

> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>
> HrTimer::expires() previously read node.expires via a volatile load, which
> can race with C-side updates. Rework the API so it is only callable with
> exclusive access or from the callback context.
>
> Introduce expires_unchecked() with an explicit safety contract, switch
> HrTimer::expires() to Pin<&mut Self>, add
> HrTimerCallbackContext::expires(), and route the read through
> hrtimer_get_expires() via a Rust helper.
>
> 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>

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

Best regards,
Andreas Hindborg

Re: [PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

[Miguel Ojeda]

On Thu, Jun 18, 2026 at 11:57 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>

Does this mean you will pick it up via timekeeping-next or should this
go through rust-fixes? (asking since sometimes you give a review tag
but then pick it up yourself)

Thanks!

Cheers,
Miguel

Re: [PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

[Andreas Hindborg]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> On Thu, Jun 18, 2026 at 11:57 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> Does this mean you will pick it up via timekeeping-next or should this
> go through rust-fixes? (asking since sometimes you give a review tag
> but then pick it up yourself)

I was planning to take it for the next one. I don't think it is urgent.


Best regards,
Andreas Hindborg

Re: [PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

[FUJITA Tomonori]

On Thu, 18 Jun 2026 11:56:59 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> "FUJITA Tomonori" <tomo@aliasing.net> writes:
> 
>> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>
>> HrTimer::expires() previously read node.expires via a volatile load, which
>> can race with C-side updates. Rework the API so it is only callable with
>> exclusive access or from the callback context.
>>
>> Introduce expires_unchecked() with an explicit safety contract, switch
>> HrTimer::expires() to Pin<&mut Self>, add
>> HrTimerCallbackContext::expires(), and route the read through
>> hrtimer_get_expires() via a Rust helper.
>>
>> 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>
> 
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>

One thing I'd like to double check before merging this,
HrTImerCallbackContext::expires() now does:

unsafe { self.0.as_ref().expires_unchecked() }

which briefly makes a &HrTimer<T> from the NonNull<HrTimer<T>>.

This is only sound today because `HrTimer<T>`'s sole field is
`Opaque<bindings::hrtimer>`. As you pointed out earlier, if we add a
field to HrTimer, "stuff breaks".

Should `expires_unchecked` just keep taking `self_ptr: *const Self`,
like `raw_forward()` (and like v2 did), so
`HrTimerCallbackContext::expires()` can stay on `self.0.as_ptr()` and
never form the reference at all?

Re: [PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

[Andreas Hindborg]

FUJITA Tomonori <tomo@flapping.org> writes:

> On Thu, 18 Jun 2026 11:56:59 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "FUJITA Tomonori" <tomo@aliasing.net> writes:
>> 
>>> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>>
>>> HrTimer::expires() previously read node.expires via a volatile load, which
>>> can race with C-side updates. Rework the API so it is only callable with
>>> exclusive access or from the callback context.
>>>
>>> Introduce expires_unchecked() with an explicit safety contract, switch
>>> HrTimer::expires() to Pin<&mut Self>, add
>>> HrTimerCallbackContext::expires(), and route the read through
>>> hrtimer_get_expires() via a Rust helper.
>>>
>>> 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>
>> 
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> One thing I'd like to double check before merging this,
> HrTImerCallbackContext::expires() now does:
>
> unsafe { self.0.as_ref().expires_unchecked() }
>
> which briefly makes a &HrTimer<T> from the NonNull<HrTimer<T>>.
>
> This is only sound today because `HrTimer<T>`'s sole field is
> `Opaque<bindings::hrtimer>`. As you pointed out earlier, if we add a
> field to HrTimer, "stuff breaks".
>
> Should `expires_unchecked` just keep taking `self_ptr: *const Self`,
> like `raw_forward()` (and like v2 did), so
> `HrTimerCallbackContext::expires()` can stay on `self.0.as_ptr()` and
> never form the reference at all?

Either way is fine for me, but Gary seems to gravitate towards the
current solution, so maybe keep that? We can add a "// NOTE:" on the
struct definition saying that soundness depends on all fields being OK
with this caveat.


Best regards,
Andreas Hindborg

Re: [PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

[Miguel Ojeda]

On Wed, Jul 15, 2026 at 3:37 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Either way is fine for me, but Gary seems to gravitate towards the
> current solution, so maybe keep that? We can add a "// NOTE:" on the
> struct definition saying that soundness depends on all fields being OK
> with this caveat.

If something actually breaks, then apart from the note, what about
`repr(transparent)` to enforce it?

Cheers,
Miguel

Re: [PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

[Gary Guo]

On Wed Jul 15, 2026 at 12:22 PM BST, FUJITA Tomonori wrote:
> On Thu, 18 Jun 2026 11:56:59 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "FUJITA Tomonori" <tomo@aliasing.net> writes:
>> 
>>> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>>
>>> HrTimer::expires() previously read node.expires via a volatile load, which
>>> can race with C-side updates. Rework the API so it is only callable with
>>> exclusive access or from the callback context.
>>>
>>> Introduce expires_unchecked() with an explicit safety contract, switch
>>> HrTimer::expires() to Pin<&mut Self>, add
>>> HrTimerCallbackContext::expires(), and route the read through
>>> hrtimer_get_expires() via a Rust helper.
>>>
>>> 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>
>> 
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> One thing I'd like to double check before merging this,
> HrTImerCallbackContext::expires() now does:
>
> unsafe { self.0.as_ref().expires_unchecked() }
>
> which briefly makes a &HrTimer<T> from the NonNull<HrTimer<T>>.
>
> This is only sound today because `HrTimer<T>`'s sole field is
> `Opaque<bindings::hrtimer>`. As you pointed out earlier, if we add a
> field to HrTimer, "stuff breaks".

What breaks? You're just converting it to shared reference.

Best,
Gary

>
> Should `expires_unchecked` just keep taking `self_ptr: *const Self`,
> like `raw_forward()` (and like v2 did), so
> `HrTimerCallbackContext::expires()` can stay on `self.0.as_ptr()` and
> never form the reference at all?