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 raw_expires() 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.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/time.c         |  6 +++++
 rust/kernel/time/hrtimer.rs | 49 +++++++++++++++++++++++++++----------
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/rust/helpers/time.c b/rust/helpers/time.c
index 67a36ccc3ec4..5be4170dc429 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>
 
 void rust_helper_fsleep(unsigned long usecs)
@@ -38,3 +39,8 @@ 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..2c6340db1a09 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -224,27 +224,39 @@ 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
+    ///
+    /// - `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.
+    #[inline]
+    unsafe fn raw_expires(self_ptr: *const Self) -> HrTimerInstant<T>
+    where
+        T: HasHrTimer<T>,
+    {
+        // SAFETY:
+        // - The C API requirements for this function are fulfilled by our safety contract.
+        // - `self_ptr` is guaranteed to point to a valid `Self` via 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_ptr))) }
+    }
+
     /// 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: `raw_expires` 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>`, the pointer passed to `raw_expires` points to a
+        // valid `Self` that we have exclusive access to.
+        unsafe { Self::raw_expires(this) }
     }
 }
 
@@ -729,6 +741,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 this [`HrTimer`].
+    ///
+    /// 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 { HrTimer::<T>::raw_expires(self.0.as_ptr()) }
+    }
 }
 
 /// Use to implement the [`HasHrTimer<T>`] trait.

base-commit: 9ace4753a5202b02191d54e9fdf7f9e3d02b85eb
-- 
2.43.0

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> 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 raw_expires() 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.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Patch looks good to me, but I just want to check with Lyude about their
use case in the rvkms driver. I think that is why we did the racy
implementation originally. In C we have stuff like this:


    /**
    * drm_crtc_vblank_get_vblank_timeout - Returns the vblank timeout
    * @crtc: The CRTC
    * @vblank_time: Returns the next vblank timestamp
    *
    * The helper drm_crtc_vblank_get_vblank_timeout() returns the next vblank
    * timestamp of the CRTC's vblank timer according to the timer's expiry
    * time.
    */
    void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
    {
      struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
      struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
      u64 cur_count;
      ktime_t cur_time;

      if (!READ_ONCE(vblank->enabled)) {
        *vblank_time = ktime_get();
        return;
      }

      /*
      * A concurrent vblank timeout could update the expires field before
      * we compare it with the vblank time. Hence we'd compare the old
      * expiry time to the new vblank time; deducing the timer had already
      * expired. Reread until we get consistent values from both fields.
      */
      do {
        cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
        *vblank_time = READ_ONCE(vtimer->timer.node.expires);
      } while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));

      if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
        return; /* Already expired */

      /*
      * To prevent races we roll the hrtimer forward before we do any
      * interrupt processing - this is how real hw works (the interrupt
      * is only generated after all the vblank registers are updated)
      * and what the vblank core expects. Therefore we need to always
      * correct the timestamp by one frame.
      */
      *vblank_time = ktime_sub(*vblank_time, vtimer->interval);
    }
    EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);


Also, we got some new docs for `read_volatile` that allow us to read
memory outside Rust of any allocation that are not "valid for read" [1],
meaning racy reads are OK as far as I understand. So the original
implementation might actually be OK, although the number might not be
correct always.


Best regards,
Andreas Hindborg


[1] https://doc.rust-lang.org/std/ptr/fn.read_volatile.html

On Mon Jan 19, 2026 at 12:29 PM GMT, Andreas Hindborg wrote:
> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>
>> 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 raw_expires() 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.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>
> Patch looks good to me, but I just want to check with Lyude about their
> use case in the rvkms driver. I think that is why we did the racy
> implementation originally. In C we have stuff like this:
>
>
>     /**
>     * drm_crtc_vblank_get_vblank_timeout - Returns the vblank timeout
>     * @crtc: The CRTC
>     * @vblank_time: Returns the next vblank timestamp
>     *
>     * The helper drm_crtc_vblank_get_vblank_timeout() returns the next vblank
>     * timestamp of the CRTC's vblank timer according to the timer's expiry
>     * time.
>     */
>     void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
>     {
>       struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>       struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
>       u64 cur_count;
>       ktime_t cur_time;
>
>       if (!READ_ONCE(vblank->enabled)) {
>         *vblank_time = ktime_get();
>         return;
>       }
>
>       /*
>       * A concurrent vblank timeout could update the expires field before
>       * we compare it with the vblank time. Hence we'd compare the old
>       * expiry time to the new vblank time; deducing the timer had already
>       * expired. Reread until we get consistent values from both fields.
>       */
>       do {
>         cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
>         *vblank_time = READ_ONCE(vtimer->timer.node.expires);
>       } while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
>
>       if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
>         return; /* Already expired */
>
>       /*
>       * To prevent races we roll the hrtimer forward before we do any
>       * interrupt processing - this is how real hw works (the interrupt
>       * is only generated after all the vblank registers are updated)
>       * and what the vblank core expects. Therefore we need to always
>       * correct the timestamp by one frame.
>       */
>       *vblank_time = ktime_sub(*vblank_time, vtimer->interval);
>     }
>     EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);
>
>
> Also, we got some new docs for `read_volatile` that allow us to read
> memory outside Rust of any allocation that are not "valid for read" [1],
> meaning racy reads are OK as far as I understand. So the original
> implementation might actually be OK, although the number might not be
> correct always.

The wording is for MMIO and should not be relied on if the accessed memory is
C memory. Also, `HrTimer` is going to be a Rust allocation.

Even if we don't treat it Rust allocation, it's also only "fine" in a sense that
you don't get UB for doing it. But the value you read can still be completely
meaningless if the updater is not atomic (it would be valid compiler
implementation to, say, turn a non-atomic write into a write of a garbage value
and then an overwrite of the actual data).

I think the usage you quoted is just wrong, as on 32-bit platforms this could
well read a teared value.

Best,
Gary

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

> On Mon Jan 19, 2026 at 12:29 PM GMT, Andreas Hindborg wrote:
>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>
>>> 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 raw_expires() 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.
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>
>> Patch looks good to me, but I just want to check with Lyude about their
>> use case in the rvkms driver. I think that is why we did the racy
>> implementation originally. In C we have stuff like this:
>>
>>
>>     /**
>>     * drm_crtc_vblank_get_vblank_timeout - Returns the vblank timeout
>>     * @crtc: The CRTC
>>     * @vblank_time: Returns the next vblank timestamp
>>     *
>>     * The helper drm_crtc_vblank_get_vblank_timeout() returns the next vblank
>>     * timestamp of the CRTC's vblank timer according to the timer's expiry
>>     * time.
>>     */
>>     void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
>>     {
>>       struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>>       struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
>>       u64 cur_count;
>>       ktime_t cur_time;
>>
>>       if (!READ_ONCE(vblank->enabled)) {
>>         *vblank_time = ktime_get();
>>         return;
>>       }
>>
>>       /*
>>       * A concurrent vblank timeout could update the expires field before
>>       * we compare it with the vblank time. Hence we'd compare the old
>>       * expiry time to the new vblank time; deducing the timer had already
>>       * expired. Reread until we get consistent values from both fields.
>>       */
>>       do {
>>         cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
>>         *vblank_time = READ_ONCE(vtimer->timer.node.expires);
>>       } while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
>>
>>       if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
>>         return; /* Already expired */
>>
>>       /*
>>       * To prevent races we roll the hrtimer forward before we do any
>>       * interrupt processing - this is how real hw works (the interrupt
>>       * is only generated after all the vblank registers are updated)
>>       * and what the vblank core expects. Therefore we need to always
>>       * correct the timestamp by one frame.
>>       */
>>       *vblank_time = ktime_sub(*vblank_time, vtimer->interval);
>>     }
>>     EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);
>>
>>
>> Also, we got some new docs for `read_volatile` that allow us to read
>> memory outside Rust of any allocation that are not "valid for read" [1],
>> meaning racy reads are OK as far as I understand. So the original
>> implementation might actually be OK, although the number might not be
>> correct always.
>
> The wording is for MMIO and should not be relied on if the accessed memory is
> C memory. Also, `HrTimer` is going to be a Rust allocation.

Why do you think the wording is only valid for MMIO accesses?

I guess you are right about the allocation being a Rust allocation,
since it is allocated by Rust. However, the value we interact with is
behind an `Opaque`, so I think treating this as a non-rust allocation
should be fine?

>
> Even if we don't treat it Rust allocation, it's also only "fine" in a sense that
> you don't get UB for doing it. But the value you read can still be completely
> meaningless if the updater is not atomic (it would be valid compiler
> implementation to, say, turn a non-atomic write into a write of a garbage value
> and then an overwrite of the actual data).

This is exactly the behavior I expect. In case of concurrent writes, the
value might be garbage, but in the absence of concurrent writes, the
value will be correct.

> I think the usage you quoted is just wrong, as on 32-bit platforms this could
> well read a teared value.

Referring to the C code above, it seems like what people in C land will
do is read the value until it seems to be stable yolo ahead with that
value.

Not saying that this is what we should do, just saying that with the
updated docs for `ptr::read_volatile`, the original API is not UB. Also,
I want to confirm this change with Lyude, because I seem to recall there
was a reason we made this race possible in the first place.

If nobody has a really good reason for this race being present, we
should of course just remove it.


Best regards,
Andreas Hindborg

On Mon Jan 19, 2026 at 2:29 PM GMT, Andreas Hindborg wrote:
> "Gary Guo" <gary@garyguo.net> writes:
>
>> On Mon Jan 19, 2026 at 12:29 PM GMT, Andreas Hindborg wrote:
>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>
>>>> 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 raw_expires() 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.
>>>>
>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>>
>>> Patch looks good to me, but I just want to check with Lyude about their
>>> use case in the rvkms driver. I think that is why we did the racy
>>> implementation originally. In C we have stuff like this:
>>>
>>>
>>>     /**
>>>     * drm_crtc_vblank_get_vblank_timeout - Returns the vblank timeout
>>>     * @crtc: The CRTC
>>>     * @vblank_time: Returns the next vblank timestamp
>>>     *
>>>     * The helper drm_crtc_vblank_get_vblank_timeout() returns the next vblank
>>>     * timestamp of the CRTC's vblank timer according to the timer's expiry
>>>     * time.
>>>     */
>>>     void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
>>>     {
>>>       struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>>>       struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
>>>       u64 cur_count;
>>>       ktime_t cur_time;
>>>
>>>       if (!READ_ONCE(vblank->enabled)) {
>>>         *vblank_time = ktime_get();
>>>         return;
>>>       }
>>>
>>>       /*
>>>       * A concurrent vblank timeout could update the expires field before
>>>       * we compare it with the vblank time. Hence we'd compare the old
>>>       * expiry time to the new vblank time; deducing the timer had already
>>>       * expired. Reread until we get consistent values from both fields.
>>>       */
>>>       do {
>>>         cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
>>>         *vblank_time = READ_ONCE(vtimer->timer.node.expires);
>>>       } while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
>>>
>>>       if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
>>>         return; /* Already expired */
>>>
>>>       /*
>>>       * To prevent races we roll the hrtimer forward before we do any
>>>       * interrupt processing - this is how real hw works (the interrupt
>>>       * is only generated after all the vblank registers are updated)
>>>       * and what the vblank core expects. Therefore we need to always
>>>       * correct the timestamp by one frame.
>>>       */
>>>       *vblank_time = ktime_sub(*vblank_time, vtimer->interval);
>>>     }
>>>     EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);
>>>
>>>
>>> Also, we got some new docs for `read_volatile` that allow us to read
>>> memory outside Rust of any allocation that are not "valid for read" [1],
>>> meaning racy reads are OK as far as I understand. So the original
>>> implementation might actually be OK, although the number might not be
>>> correct always.
>>
>> The wording is for MMIO and should not be relied on if the accessed memory is
>> C memory. Also, `HrTimer` is going to be a Rust allocation.
>
> Why do you think the wording is only valid for MMIO accesses?

Because it is intent for this paragraph, and is mentioned in the text. This
specific text is introduced to allow hardware MMIO access and the need to be
able to access otherwise always-invalid pointers, such as pointers to 0 and end
of address space.

>
> I guess you are right about the allocation being a Rust allocation,
> since it is allocated by Rust. However, the value we interact with is
> behind an `Opaque`, so I think treating this as a non-rust allocation
> should be fine?

No, it is not. Opaque cancels out some alias requirements but it is still a Rust
allocation. Everything that Rust code interacts directly, and this includes if
the allocation is made from C side, must stay follow the usual rules.

>
>>
>> Even if we don't treat it Rust allocation, it's also only "fine" in a sense that
>> you don't get UB for doing it. But the value you read can still be completely
>> meaningless if the updater is not atomic (it would be valid compiler
>> implementation to, say, turn a non-atomic write into a write of a garbage value
>> and then an overwrite of the actual data).
>
> This is exactly the behavior I expect. In case of concurrent writes, the
> value might be garbage, but in the absence of concurrent writes, the
> value will be correct.
>
>> I think the usage you quoted is just wrong, as on 32-bit platforms this could
>> well read a teared value.
>
> Referring to the C code above, it seems like what people in C land will
> do is read the value until it seems to be stable yolo ahead with that
> value.

It is non-obvious to me on why this code is correct. The updater seems to do a
hrtimer_forward_now and then call into drivers to update the vblank count. So
in this sequence:

    CPU1                 CPU2
                         first drm_crtc_vblank_count_and_time
    hrtimer triggered         
    hrtimer_forward_now  READ_ONCE(expires) <- racy read
                         second drm_crtc_vblank_count_and_time
    store_vblank

you will get a teared read of garbage value while the vblank count is stable.

Best,
Gary

>
> Not saying that this is what we should do, just saying that with the
> updated docs for `ptr::read_volatile`, the original API is not UB. Also,
> I want to confirm this change with Lyude, because I seem to recall there
> was a reason we made this race possible in the first place.
>
> If nobody has a really good reason for this race being present, we
> should of course just remove it.
>
>
> Best regards,
> Andreas Hindborg

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

> On Mon Jan 19, 2026 at 2:29 PM GMT, Andreas Hindborg wrote:
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>>> On Mon Jan 19, 2026 at 12:29 PM GMT, Andreas Hindborg wrote:
>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>
>>>>> 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 raw_expires() 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.
>>>>>
>>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>>>
>>>> Patch looks good to me, but I just want to check with Lyude about their
>>>> use case in the rvkms driver. I think that is why we did the racy
>>>> implementation originally. In C we have stuff like this:
>>>>
>>>>
>>>>     /**
>>>>     * drm_crtc_vblank_get_vblank_timeout - Returns the vblank timeout
>>>>     * @crtc: The CRTC
>>>>     * @vblank_time: Returns the next vblank timestamp
>>>>     *
>>>>     * The helper drm_crtc_vblank_get_vblank_timeout() returns the next vblank
>>>>     * timestamp of the CRTC's vblank timer according to the timer's expiry
>>>>     * time.
>>>>     */
>>>>     void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
>>>>     {
>>>>       struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>>>>       struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
>>>>       u64 cur_count;
>>>>       ktime_t cur_time;
>>>>
>>>>       if (!READ_ONCE(vblank->enabled)) {
>>>>         *vblank_time = ktime_get();
>>>>         return;
>>>>       }
>>>>
>>>>       /*
>>>>       * A concurrent vblank timeout could update the expires field before
>>>>       * we compare it with the vblank time. Hence we'd compare the old
>>>>       * expiry time to the new vblank time; deducing the timer had already
>>>>       * expired. Reread until we get consistent values from both fields.
>>>>       */
>>>>       do {
>>>>         cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
>>>>         *vblank_time = READ_ONCE(vtimer->timer.node.expires);
>>>>       } while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
>>>>
>>>>       if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
>>>>         return; /* Already expired */
>>>>
>>>>       /*
>>>>       * To prevent races we roll the hrtimer forward before we do any
>>>>       * interrupt processing - this is how real hw works (the interrupt
>>>>       * is only generated after all the vblank registers are updated)
>>>>       * and what the vblank core expects. Therefore we need to always
>>>>       * correct the timestamp by one frame.
>>>>       */
>>>>       *vblank_time = ktime_sub(*vblank_time, vtimer->interval);
>>>>     }
>>>>     EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);
>>>>
>>>>
>>>> Also, we got some new docs for `read_volatile` that allow us to read
>>>> memory outside Rust of any allocation that are not "valid for read" [1],
>>>> meaning racy reads are OK as far as I understand. So the original
>>>> implementation might actually be OK, although the number might not be
>>>> correct always.
>>>
>>> The wording is for MMIO and should not be relied on if the accessed memory is
>>> C memory. Also, `HrTimer` is going to be a Rust allocation.
>>
>> Why do you think the wording is only valid for MMIO accesses?
>
> Because it is intent for this paragraph, and is mentioned in the text. This
> specific text is introduced to allow hardware MMIO access and the need to be
> able to access otherwise always-invalid pointers, such as pointers to 0 and end
> of address space.

The text does not say "only valid for MMIO locations", it says "intended
for". It also would make no sense if it did, because MMIO locations are
not special in that sense.

>>
>> I guess you are right about the allocation being a Rust allocation,
>> since it is allocated by Rust. However, the value we interact with is
>> behind an `Opaque`, so I think treating this as a non-rust allocation
>> should be fine?
>
> No, it is not. Opaque cancels out some alias requirements but it is still a Rust
> allocation. Everything that Rust code interacts directly, and this includes if
> the allocation is made from C side, must stay follow the usual rules.

As long as we are not ever making a reference pointing within the
allocation or doing non-volatile operations on it, it we should be able
to consider the region as a non-rust allocation, right? If not, why not?

I am aware that this does not apply for the expires field, I'm just
asking for the sake of clarification.

>
>>
>>>
>>> Even if we don't treat it Rust allocation, it's also only "fine" in a sense that
>>> you don't get UB for doing it. But the value you read can still be completely
>>> meaningless if the updater is not atomic (it would be valid compiler
>>> implementation to, say, turn a non-atomic write into a write of a garbage value
>>> and then an overwrite of the actual data).
>>
>> This is exactly the behavior I expect. In case of concurrent writes, the
>> value might be garbage, but in the absence of concurrent writes, the
>> value will be correct.
>>
>>> I think the usage you quoted is just wrong, as on 32-bit platforms this could
>>> well read a teared value.
>>
>> Referring to the C code above, it seems like what people in C land will
>> do is read the value until it seems to be stable yolo ahead with that
>> value.
>
> It is non-obvious to me on why this code is correct. The updater seems to do a
> hrtimer_forward_now and then call into drivers to update the vblank count. So
> in this sequence:
>
>     CPU1                 CPU2
>                          first drm_crtc_vblank_count_and_time
>     hrtimer triggered
>     hrtimer_forward_now  READ_ONCE(expires) <- racy read
>                          second drm_crtc_vblank_count_and_time
>     store_vblank
>
> you will get a teared read of garbage value while the vblank count is stable.

Right, that is interesting. And it will not help them to flip the
operations on `expires` and `count`. I guess they need to update `count`
twice to make it sound, effectively implementing a seqlock.


Best regards,
Andreas Hindborg

On Tue Jan 20, 2026 at 8:23 AM GMT, Andreas Hindborg wrote:
> "Gary Guo" <gary@garyguo.net> writes:
>>>>>
>>>>> Also, we got some new docs for `read_volatile` that allow us to read
>>>>> memory outside Rust of any allocation that are not "valid for read" [1],
>>>>> meaning racy reads are OK as far as I understand. So the original
>>>>> implementation might actually be OK, although the number might not be
>>>>> correct always.
>>>>
>>>> The wording is for MMIO and should not be relied on if the accessed memory is
>>>> C memory. Also, `HrTimer` is going to be a Rust allocation.
>>>
>>> Why do you think the wording is only valid for MMIO accesses?
>>
>> Because it is intent for this paragraph, and is mentioned in the text. This
>> specific text is introduced to allow hardware MMIO access and the need to be
>> able to access otherwise always-invalid pointers, such as pointers to 0 and end
>> of address space.
>
> The text does not say "only valid for MMIO locations", it says "intended
> for". It also would make no sense if it did, because MMIO locations are
> not special in that sense.
>
> <...>
>
> As long as we are not ever making a reference pointing within the
> allocation or doing non-volatile operations on it, it we should be able
> to consider the region as a non-rust allocation, right? If not, why not?

The important thing is that it is something that is never handled at all by
non-volatile means throughout the lifetime of the object. MMIO is speical in
this regard.

So for example, if you put it on the stack, then it is already within the
purview of the AM. You might be able to argue the otherwise if the thing is
created using pin_init and then you never initialize the memory in the first
place (which is not the case if you just use `Opauqe::uninit` by the way,
because there's a move, you'd need to use `ffi_init` and then do nothing in the
callback).

So yes, you can probably get what you want if this is behind a `Opaque` and you
never creates reference to the internal value or touch the memory in other
means. But this is all very sketchy, and I would just say "don't rely on this
behaviour unless you're doing MMIO". Use LKMM for anything else.

>
> I am aware that this does not apply for the expires field, I'm just
> asking for the sake of clarification.
>
>>
>>>
>>>>
>>>> Even if we don't treat it Rust allocation, it's also only "fine" in a sense that
>>>> you don't get UB for doing it. But the value you read can still be completely
>>>> meaningless if the updater is not atomic (it would be valid compiler
>>>> implementation to, say, turn a non-atomic write into a write of a garbage value
>>>> and then an overwrite of the actual data).
>>>
>>> This is exactly the behavior I expect. In case of concurrent writes, the
>>> value might be garbage, but in the absence of concurrent writes, the
>>> value will be correct.
>>>
>>>> I think the usage you quoted is just wrong, as on 32-bit platforms this could
>>>> well read a teared value.
>>>
>>> Referring to the C code above, it seems like what people in C land will
>>> do is read the value until it seems to be stable yolo ahead with that
>>> value.
>>
>> It is non-obvious to me on why this code is correct. The updater seems to do a
>> hrtimer_forward_now and then call into drivers to update the vblank count. So
>> in this sequence:
>>
>>     CPU1                 CPU2
>>                          first drm_crtc_vblank_count_and_time
>>     hrtimer triggered
>>     hrtimer_forward_now  READ_ONCE(expires) <- racy read
>>                          second drm_crtc_vblank_count_and_time
>>     store_vblank
>>
>> you will get a teared read of garbage value while the vblank count is stable.
>
> Right, that is interesting. And it will not help them to flip the
> operations on `expires` and `count`. I guess they need to update `count`
> twice to make it sound, effectively implementing a seqlock.

Alternatively just update `expires` field to be `atomic64_t` and use 64-bit
atomic ops always. But it then it means locks on 32-bit systems without 64-bit
atomics.

But anyway I think this explains why I would encourage to use LKMM rather than
just do a `read_volatile` (or READ_ONCE), because it is very unclear what
semantics you actually want.

Best,
Gary

On Mon, Jan 19, 2026 at 01:07:58PM +0000, Gary Guo wrote:
> On Mon Jan 19, 2026 at 12:29 PM GMT, Andreas Hindborg wrote:
> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> >
> >> 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 raw_expires() 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.
> >>
> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> >
> > Patch looks good to me, but I just want to check with Lyude about their
> > use case in the rvkms driver. I think that is why we did the racy
> > implementation originally. In C we have stuff like this:
> >
> >
> >     /**
> >     * drm_crtc_vblank_get_vblank_timeout - Returns the vblank timeout
> >     * @crtc: The CRTC
> >     * @vblank_time: Returns the next vblank timestamp
> >     *
> >     * The helper drm_crtc_vblank_get_vblank_timeout() returns the next vblank
> >     * timestamp of the CRTC's vblank timer according to the timer's expiry
> >     * time.
> >     */
> >     void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
> >     {
> >       struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> >       struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
> >       u64 cur_count;
> >       ktime_t cur_time;
> >
> >       if (!READ_ONCE(vblank->enabled)) {
> >         *vblank_time = ktime_get();
> >         return;
> >       }
> >
> >       /*
> >       * A concurrent vblank timeout could update the expires field before
> >       * we compare it with the vblank time. Hence we'd compare the old
> >       * expiry time to the new vblank time; deducing the timer had already
> >       * expired. Reread until we get consistent values from both fields.
> >       */
> >       do {
> >         cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
> >         *vblank_time = READ_ONCE(vtimer->timer.node.expires);
> >       } while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
> >
> >       if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
> >         return; /* Already expired */
> >
> >       /*
> >       * To prevent races we roll the hrtimer forward before we do any
> >       * interrupt processing - this is how real hw works (the interrupt
> >       * is only generated after all the vblank registers are updated)
> >       * and what the vblank core expects. Therefore we need to always
> >       * correct the timestamp by one frame.
> >       */
> >       *vblank_time = ktime_sub(*vblank_time, vtimer->interval);
> >     }
> >     EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);
> >
> >
> > Also, we got some new docs for `read_volatile` that allow us to read
> > memory outside Rust of any allocation that are not "valid for read" [1],
> > meaning racy reads are OK as far as I understand. So the original
> > implementation might actually be OK, although the number might not be
> > correct always.
> 
> The wording is for MMIO and should not be relied on if the accessed memory is
> C memory. Also, `HrTimer` is going to be a Rust allocation.
> 

Right, "memory outside Rust AM" should mean memory that outside both
Rust and C in kernel (i.e. userspace, MMIO, etc.) It's not a
"out-of-UB" free card.

> Even if we don't treat it Rust allocation, it's also only "fine" in a sense that
> you don't get UB for doing it. But the value you read can still be completely
> meaningless if the updater is not atomic (it would be valid compiler
> implementation to, say, turn a non-atomic write into a write of a garbage value
> and then an overwrite of the actual data).
> 
> I think the usage you quoted is just wrong, as on 32-bit platforms this could
> well read a teared value.
> 

Agreed. It's a data race any way, unless there is a proof that ->exipres
won't be updated during read.

Regards,
Boqun

> Best,
> Gary

On 10/01/2026 12:58, FUJITA Tomonori wrote:
> 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 raw_expires() 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.


Do we want some tags here? E.g.:

Fixes: 4b0147494275 ("rust: hrtimer: Add HrTimer::expires()")
Closes: 
https://lore.kernel.org/rust-for-linux/87ldi7f4o1.fsf@t14s.mail-host-address-is-not-set/

Some quite minor nits below (which might be fixed while applying?):

> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>   rust/helpers/time.c         |  6 +++++
>   rust/kernel/time/hrtimer.rs | 49 +++++++++++++++++++++++++++----------
>   2 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
> index 67a36ccc3ec4..5be4170dc429 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>
>   
>   void rust_helper_fsleep(unsigned long usecs)
> @@ -38,3 +39,8 @@ 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..2c6340db1a09 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -224,27 +224,39 @@ 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
> +    ///
> +    /// - `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

Would "... pointed to ..." be better than "... pointed at ..."?

> +    ///   within the context of the timer callback.
> +    #[inline]
> +    unsafe fn raw_expires(self_ptr: *const Self) -> HrTimerInstant<T>
> +    where
> +        T: HasHrTimer<T>,
> +    {
> +        // SAFETY:
> +        // - The C API requirements for this function are fulfilled by our safety contract.
> +        // - `self_ptr` is guaranteed to point to a valid `Self` via our safety contract.
> +        // - Timers cannot have negative ktime_t values as their expiration time.

ktime_t -> `ktime_t` ?

> +        unsafe { Instant::from_ktime(bindings::hrtimer_get_expires(Self::raw_get(self_ptr))) }
> +    }
> +
>       /// 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: `raw_expires` does not move `Self`

Missing period at the end

> +        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>`, the pointer passed to `raw_expires` points to a
> +        // valid `Self` that we have exclusive access to.
> +        unsafe { Self::raw_expires(this) }
>       }
>   }
>   
> @@ -729,6 +741,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 this [`HrTimer`].
> +    ///
> +    /// 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 { HrTimer::<T>::raw_expires(self.0.as_ptr()) }
> +    }
>   }
>   
>   /// Use to implement the [`HasHrTimer<T>`] trait.

Best regards

Dirk

On Fri, 16 Jan 2026 07:20:44 +0100
Dirk Behme <dirk.behme@de.bosch.com> wrote:

> On 10/01/2026 12:58, FUJITA Tomonori wrote:
>> 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 raw_expires() 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.
> 
> 
> Do we want some tags here? E.g.:
> 
> Fixes: 4b0147494275 ("rust: hrtimer: Add HrTimer::expires()")
> Closes:
> https://lore.kernel.org/rust-for-linux/87ldi7f4o1.fsf@t14s.mail-host-address-is-not-set/

Make sense.

> Some quite minor nits below (which might be fixed while applying?):
> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>   rust/helpers/time.c         |  6 +++++
>>   rust/kernel/time/hrtimer.rs | 49 +++++++++++++++++++++++++++----------
>>   2 files changed, 42 insertions(+), 13 deletions(-)
>> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
>> index 67a36ccc3ec4..5be4170dc429 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>
>>     void rust_helper_fsleep(unsigned long usecs)
>> @@ -38,3 +39,8 @@ 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..2c6340db1a09 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -224,27 +224,39 @@ 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
>> +    ///
>> +    /// - `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
> 
> Would "... pointed to ..." be better than "... pointed at ..."?

I think that both are understandable, but I agree that 'pointed to'
reads more natural here. I'll change.

>> +    ///   within the context of the timer callback.
>> +    #[inline]
>> +    unsafe fn raw_expires(self_ptr: *const Self) -> HrTimerInstant<T>
>> +    where
>> +        T: HasHrTimer<T>,
>> +    {
>> +        // SAFETY:
>> + // - The C API requirements for this function are fulfilled by our
>> safety contract.
>> + // - `self_ptr` is guaranteed to point to a valid `Self` via our
>> safety contract.
>> + // - Timers cannot have negative ktime_t values as their expiration
>> time.
> 
> ktime_t -> `ktime_t` ?

Of course, I'll fix.

>> + unsafe {
>> Instant::from_ktime(bindings::hrtimer_get_expires(Self::raw_get(self_ptr)))
>> }
>> +    }
>> +
>>       /// 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: `raw_expires` does not move `Self`
> 
> Missing period at the end

I'll fix.

>> +        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>`, the pointer passed to
>> `raw_expires` points to a
>> +        // valid `Self` that we have exclusive access to.
>> +        unsafe { Self::raw_expires(this) }
>>       }
>>   }
>>   @@ -729,6 +741,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 this [`HrTimer`].
>> +    ///
>> + /// 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 { HrTimer::<T>::raw_expires(self.0.as_ptr()) }
>> +    }
>>   }
>>     /// Use to implement the [`HasHrTimer<T>`] trait.

Thanks for the review!


Andreas, do you like me to send v2?