Message-ID: <d817b1db-7f49-4220-ba82-1c8a750c84f1@de.bosch.com>
Date: Fri, 16 Jan 2026 07:20:44 +0100
From: Dirk Behme <dirk.behme@...bosch.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, <a.hindborg@...nel.org>,
	<ojeda@...nel.org>, <lyude@...hat.com>
CC: <aliceryhl@...gle.com>, <anna-maria@...utronix.de>,
	<bjorn3_gh@...tonmail.com>, <boqun.feng@...il.com>, <dakr@...nel.org>,
	<frederic@...nel.org>, <gary@...yguo.net>, <jstultz@...gle.com>,
	<lossin@...nel.org>, <sboyd@...nel.org>, <tglx@...utronix.de>,
	<tmgross@...ch.edu>, <linux-kernel@...r.kernel.org>,
	<rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v1] rust: hrtimer: Restrict expires() to safe contexts

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@...il.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

Powered by blists - more mailing lists