linux-kernel - Re: [PATCH v1] rust: hrtimer: Restrict expires() to safe contexts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20260119.205816.296364426219257330.fujita.tomonori@gmail.com>
Date: Mon, 19 Jan 2026 20:58:16 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: dirk.behme@...bosch.com, a.hindborg@...nel.org
Cc: fujita.tomonori@...il.com, ojeda@...nel.org, lyude@...hat.com,
 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 Fri, 16 Jan 2026 07:20:44 +0100
Dirk Behme <dirk.behme@...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@...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 ..."?

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?