[PATCH 0/4] Fix forward()/expires() racing with concurrent arming

[PATCH 0/4] Fix forward()/expires() racing with concurrent arming

[FUJITA Tomonori]

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

This series started from the review of patches 3 and 4 [1]: a hrtimer
can be armed from any CPU at any time, including while its callback
runs, so restricting HrTimer::expires() to the callback context is not
by itself enough to remove the race.

It turned out that expires() is not the only problem. A callback may
also change its expiry time with hrtimer_forward(), which is sound
only because __run_hrtimer() dequeues the timer for the duration of
the callback. Arming the same timer from another CPU puts it back into
the rbtree while the callback runs, so hrtimer_forward() then changes
the expiry of a timer that is queued, without the base lock and
without re-checking the ordering, which leaves the tree unsorted.

Two of the four pointer types cannot construct that
situation. Starting a Pin<Box<T, A>> moves the box into the handle,
and starting a Pin<&mut T> consumes the exclusive borrow, so in both
cases nothing is left to arm the timer with. Arc<T> is Clone and
Pin<&T> is Copy, and both of their start functions are reachable from
safe code, so safe Rust could arm a timer whose callback was running.

"No arming while the callback runs" cannot be expressed in the type
system, because the callback begins when the timer expires rather than
at any point in the Rust program, so patches 1 and 2 use the stronger
"no arming while armed" instead. hrtimer_cancel() waits for the
handler to return, which makes that the point where the right to arm
can be handed back. The right to arm is split out of Arc<T> into
HrTimerArc<T> and out of Pin<&T> into HrTimerPin<'a, T>, both
non-clonable and consumed by start, modelled on ListArc; the object
itself stays shareable through plain Arc references and shared pinned
references respectively.

Patches 3 and 4 are the previously posted expires() and
repr(transparent) patches, unchanged. With patches 1 and 2 in place,
the callback context has no concurrent writer of node.expires. So
HrTimerCallbackContext::expires() is sound.

[1]: https://lore.kernel.org/rust-for-linux/20260807233039.1091842-1-tomo@flapping.org/

FUJITA Tomonori (4):
  rust: hrtimer: Introduce HrTimerArc to make arming exclusive
  rust: hrtimer: Introduce HrTimerPin to make arming exclusive
  rust: hrtimer: Restrict expires() to safe contexts
  rust: hrtimer: Make HrTimer repr(transparent)

 rust/helpers/time.c                 |   6 ++
 rust/kernel/time/hrtimer.rs         | 135 ++++++++++++++++------------
 rust/kernel/time/hrtimer/arc.rs     | 113 +++++++++++++++++------
 rust/kernel/time/hrtimer/pin.rs     | 107 +++++++++++++++-------
 rust/kernel/time/hrtimer/pin_mut.rs |   2 +-
 rust/kernel/time/hrtimer/tbox.rs    |   2 +-
 6 files changed, 249 insertions(+), 116 deletions(-)


base-commit: 643a7c306b8ce32743d4f94dd700c8588be37e66
-- 
2.43.0

[PATCH v1 1/4] rust: hrtimer: Introduce HrTimerArc to make arming exclusive

[FUJITA Tomonori]

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

A hrtimer callback may change its own expiry time with
hrtimer_forward(), which is sound only because __run_hrtimer() dequeues
the timer for the duration of the callback. Arming the same timer from
another CPU puts it back into the rbtree while the callback runs, so
that expiry update lands on a queued timer and the tree ordering goes
stale. Arc<T> is Clone and Arc<T>::start() is safe, so safe Rust can arm
a timer on any CPU, including while its callback is running.

"No arming while the callback runs" cannot be expressed in the type
system, because the callback begins when the timer expires rather than
at any point in the Rust program, so use the stronger "no arming while
armed" instead. hrtimer_cancel() waits for the handler to return, which
makes that the point where the right to arm can be handed back.
Pin<Box<T, A>> and Pin<&mut T> already work this way through ownership
and exclusive borrow.

Split the right to arm out of Arc<T> into HrTimerArc<T>, a non-clonable
wrapper modelled on ListArc that is created from a Pin<UniqueArc<T>> and
consumed by start(); the object stays shareable through plain Arc
references.

Fixes: 3f2a5ba784b8 ("rust: hrtimer: Add HrTimerCallbackContext and ::forward()")
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time/hrtimer.rs     |  33 +++++-----
 rust/kernel/time/hrtimer/arc.rs | 111 +++++++++++++++++++++++++-------
 2 files changed, 103 insertions(+), 41 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 2d7f1131a813..a7587db1d552 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -172,12 +172,12 @@
 //! #     sync::{
 //! #         atomic::{ordering, Atomic},
 //! #         completion::Completion,
-//! #         Arc, ArcBorrow,
+//! #         ArcBorrow,
 //! #     },
 //! #     time::{
 //! #         hrtimer::{
-//! #             RelativeMode, HrTimer, HrTimerCallback, HrTimerPointer, HrTimerRestart,
-//! #             HasHrTimer, HrTimerCallbackContext
+//! #             RelativeMode, HrTimer, HrTimerArc, HrTimerCallback, HrTimerPointer,
+//! #             HrTimerRestart, HasHrTimer, HrTimerCallbackContext
 //! #         },
 //! #         Delta, Monotonic,
 //! #     },
@@ -204,7 +204,7 @@
 //! }
 //!
 //! impl HrTimerCallback for ArcIntrusiveHrTimer {
-//!     type Pointer<'a> = Arc<Self>;
+//!     type Pointer<'a> = HrTimerArc<Self>;
 //!
 //!     fn run(
 //!         this: ArcBorrow<'_, Self>,
@@ -229,11 +229,12 @@
 //!     }
 //! }
 //!
-//! let has_timer = Arc::pin_init(ArcIntrusiveHrTimer::new(), GFP_KERNEL)?;
-//! let _handle = has_timer.clone().start(Delta::from_micros(200));
+//! let has_timer = HrTimerArc::pin_init(ArcIntrusiveHrTimer::new(), GFP_KERNEL)?;
+//! let shared = has_timer.clone_arc();
+//! let _handle = has_timer.start(Delta::from_micros(200));
 //!
-//! while has_timer.flag.load(ordering::Relaxed) != 5 {
-//!     has_timer.cond.wait_for_completion();
+//! while shared.flag.load(ordering::Relaxed) != 5 {
+//!     shared.cond.wait_for_completion();
 //! }
 //!
 //! pr_info!("Counted to 5\n");
@@ -589,18 +590,17 @@ pub fn expires(&self) -> HrTimerInstant<T>
 /// `Self` must be [`Sync`] because it is passed to timer callbacks in another
 /// thread of execution (hard or soft interrupt context).
 ///
-/// Starting a timer returns a [`HrTimerHandle`] that can be used to manipulate
-/// the timer. Note that it is OK to call the start function repeatedly, and
-/// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
-/// exist. A timer can be manipulated through any of the handles, and a handle
-/// may represent a cancelled timer.
+/// Starting a timer consumes `Self` and returns a [`HrTimerHandle`] that can be
+/// used to manipulate the timer. As a timer in the **started** or **running**
+/// state cannot be started again, at most one [`HrTimerHandle`] for a timer
+/// exists at a time. A handle may represent a cancelled timer.
 pub trait HrTimerPointer: Sync + Sized {
     /// The operational mode associated with this timer.
     ///
     /// This defines how the expiration value is interpreted.
     type TimerMode: HrTimerMode;
 
-    /// A handle representing a started or restarted timer.
+    /// A handle representing a started timer.
     ///
     /// If the timer is running or if the timer callback is executing when the
     /// handle is dropped, the drop method of [`HrTimerHandle`] should not return
@@ -610,8 +610,7 @@ pub trait HrTimerPointer: Sync + Sized {
     /// leak the handle.
     type TimerHandle: HrTimerHandle;
 
-    /// Start the timer with expiry after `expires` time units. If the timer was
-    /// already running, it is restarted with the new expiry time.
+    /// Start the timer with expiry after `expires` time units.
     fn start(self, expires: <Self::TimerMode as HrTimerMode>::Expires) -> Self::TimerHandle;
 }
 
@@ -1110,7 +1109,7 @@ unsafe fn timer_container_of(
 }
 
 mod arc;
-pub use arc::ArcHrTimerHandle;
+pub use arc::{ArcHrTimerHandle, HrTimerArc};
 mod pin;
 pub use pin::PinHrTimerHandle;
 mod pin_mut;
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
index 7be82bcb352a..2134d12d558c 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -8,47 +8,71 @@
 use super::HrTimerMode;
 use super::HrTimerPointer;
 use super::RawHrTimerCallback;
-use crate::sync::Arc;
-use crate::sync::ArcBorrow;
+use crate::alloc::Flags;
+use crate::error::{Error, Result};
+use crate::init::InPlaceInit;
+use crate::sync::{Arc, ArcBorrow, UniqueArc};
+use core::pin::Pin;
+use pin_init::PinInit;
 
-/// A handle for an `Arc<HasHrTimer<T>>` returned by a call to
-/// [`HrTimerPointer::start`].
-pub struct ArcHrTimerHandle<T>
+/// A wrapper around [`Arc`] that's guaranteed unique.
+///
+/// The `HrTimerArc` type can be thought of as a special reference to a refcounted object that owns
+/// the permission to arm the [`HrTimer`] stored in the refcounted object. By ensuring that each
+/// object has only one `HrTimerArc` reference, the owner of that reference is assured exclusive
+/// access to the arming operation. When a timer is started, the returned [`ArcHrTimerHandle`] takes
+/// ownership of the `HrTimerArc` reference.
+///
+/// While this `HrTimerArc` is unique, there still might exist normal [`Arc`] references to the
+/// object. Use [`HrTimerArc::clone_arc`] to obtain one.
+///
+/// # Invariants
+///
+/// * Each reference counted object has at most one `HrTimerArc`.
+pub struct HrTimerArc<T>
 where
     T: HasHrTimer<T>,
 {
-    pub(crate) inner: Arc<T>,
+    arc: Arc<T>,
 }
 
-// SAFETY: We implement drop below, and we cancel the timer in the drop
-// implementation.
-unsafe impl<T> HrTimerHandle for ArcHrTimerHandle<T>
+impl<T> HrTimerArc<T>
 where
     T: HasHrTimer<T>,
 {
-    fn cancel(&mut self) -> bool {
-        let self_ptr = Arc::as_ptr(&self.inner);
-
-        // SAFETY: As we obtained `self_ptr` from a valid reference above, it
-        // must point to a valid `T`.
-        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self_ptr) };
+    /// Use the given pin-initializer to pin-initialize a `T` inside of a new `HrTimerArc`.
+    #[inline]
+    pub fn pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Self>
+    where
+        Error: From<E>,
+    {
+        Ok(Self::from(UniqueArc::pin_init(init, flags)?))
+    }
 
-        // SAFETY: As `timer_ptr` points into `T` and `T` is valid, `timer_ptr`
-        // must point to a valid `HrTimer` instance.
-        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
+    /// Clone an [`Arc`] from this `HrTimerArc`.
+    ///
+    /// The returned [`Arc`] can be used to access the object, but not to arm its timer.
+    #[inline]
+    pub fn clone_arc(&self) -> Arc<T> {
+        self.arc.clone()
     }
 }
 
-impl<T> Drop for ArcHrTimerHandle<T>
+impl<T> From<Pin<UniqueArc<T>>> for HrTimerArc<T>
 where
     T: HasHrTimer<T>,
 {
-    fn drop(&mut self) {
-        self.cancel();
+    /// Convert a pinned [`UniqueArc`] into a [`HrTimerArc`].
+    #[inline]
+    fn from(unique: Pin<UniqueArc<T>>) -> Self {
+        // INVARIANT: We have a `UniqueArc`, so there is no `HrTimerArc` for this object.
+        Self {
+            arc: Arc::from(unique),
+        }
     }
 }
 
-impl<T> HrTimerPointer for Arc<T>
+impl<T> HrTimerPointer for HrTimerArc<T>
 where
     T: 'static,
     T: Send + Sync,
@@ -66,12 +90,51 @@ fn start(
         //  - We keep `self` alive by wrapping it in a handle below.
         //  - Since we generate the pointer passed to `start` from a valid
         //    reference, it is a valid pointer.
-        unsafe { T::start(Arc::as_ptr(&self), expires) };
+        unsafe { T::start(Arc::as_ptr(&self.arc), expires) };
         ArcHrTimerHandle { inner: self }
     }
 }
 
-impl<T> RawHrTimerCallback for Arc<T>
+/// A handle for a [`HrTimerArc`] returned by a call to [`HrTimerPointer::start`].
+///
+/// This handle owns the [`HrTimerArc`] reference for the object, so the timer cannot be armed
+/// again while this handle exists.
+pub struct ArcHrTimerHandle<T>
+where
+    T: HasHrTimer<T>,
+{
+    pub(crate) inner: HrTimerArc<T>,
+}
+
+// SAFETY: We implement drop below, and we cancel the timer in the drop
+// implementation.
+unsafe impl<T> HrTimerHandle for ArcHrTimerHandle<T>
+where
+    T: HasHrTimer<T>,
+{
+    fn cancel(&mut self) -> bool {
+        let self_ptr = Arc::as_ptr(&self.inner.arc);
+
+        // SAFETY: As we obtained `self_ptr` from a valid reference above, it
+        // must point to a valid `T`.
+        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self_ptr) };
+
+        // SAFETY: As `timer_ptr` points into `T` and `T` is valid, `timer_ptr`
+        // must point to a valid `HrTimer` instance.
+        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<T> Drop for ArcHrTimerHandle<T>
+where
+    T: HasHrTimer<T>,
+{
+    fn drop(&mut self) {
+        self.cancel();
+    }
+}
+
+impl<T> RawHrTimerCallback for HrTimerArc<T>
 where
     T: 'static,
     T: HasHrTimer<T>,
-- 
2.43.0

[PATCH v1 2/4] rust: hrtimer: Introduce HrTimerPin to make arming exclusive

[FUJITA Tomonori]

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

Pin<&T> has the same hole that Arc<T> had: it is Copy and
ScopedHrTimerPointer::start_scoped() is safe, so a copy captured by the
closure can arm a timer while it is already armed and its callback may
be running.

Split the right to arm out of Pin<&T> into HrTimerPin<'a, T>, which is
created from a Pin<&'a mut T> and consumed by start_scoped(). The borrow
checker supplies the exclusivity here, and the closure keeps reading the
object through the shared pinned reference returned by
HrTimerPin::as_ref().

All four pointer types now separate sharing an object from arming its
timer, so the restart operation no longer exists in the safe API. Drop
it from the documentation.

Fixes: 3f2a5ba784b8 ("rust: hrtimer: Add HrTimerCallbackContext and ::forward()")
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time/hrtimer.rs     |  50 +++++++--------
 rust/kernel/time/hrtimer/pin.rs | 105 +++++++++++++++++++++++---------
 2 files changed, 98 insertions(+), 57 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index a7587db1d552..d94275f2e93f 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -9,15 +9,15 @@
 //!
 //! States:
 //!
-//! - Stopped: initialized but not started, or cancelled, or not restarted.
-//! - Started: initialized and started or restarted.
+//! - Stopped: initialized but not started, cancelled, or the callback returned
+//!   `NoRestart`.
+//! - Started: initialized and started, or the callback returned `Restart`.
 //! - Running: executing the callback.
 //!
 //! Operations:
 //!
 //! * Start
 //! * Cancel
-//! * Restart
 //!
 //! Events:
 //!
@@ -42,11 +42,7 @@
 //! --------->|    Stopped      |                 |      Started     +---------->|     Running     |
 //!           |                 |     Cancel      |                  |           |                 |
 //!           |                 |<----------------+                  |           |                 |
-//!           +-----------------+                 +---------------+--+           +-----------------+
-//!                                                     ^         |
-//!                                                     |         |
-//!                                                     +---------+
-//!                                                      Restart
+//!           +-----------------+                 +------------------+           +-----------------+
 //! ```
 //!
 //!
@@ -60,16 +56,13 @@
 //! 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.
+//! 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.
+//! The `start` operation consumes the pointer it is called on, so a timer in the
+//! **started** or **running** state cannot be started again. It has to be
+//! **cancelled** first.
 //!
 //! # Examples
 //!
@@ -253,8 +246,8 @@
 //! #     },
 //! #     time::{
 //! #         hrtimer::{
-//! #             ScopedHrTimerPointer, HrTimer, HrTimerCallback, HrTimerPointer, HrTimerRestart,
-//! #             HasHrTimer, RelativeMode, HrTimerCallbackContext
+//! #             ScopedHrTimerPointer, HrTimer, HrTimerCallback, HrTimerPin, HrTimerPointer,
+//! #             HrTimerRestart, HasHrTimer, RelativeMode, HrTimerCallbackContext
 //! #         },
 //! #         Delta, Monotonic,
 //! #     },
@@ -282,7 +275,7 @@
 //! }
 //!
 //! impl HrTimerCallback for IntrusiveHrTimer {
-//!     type Pointer<'a> = Pin<&'a Self>;
+//!     type Pointer<'a> = HrTimerPin<'a, Self>;
 //!
 //!     fn run(this: Pin<&Self>, _ctx: HrTimerCallbackContext<'_, Self>) -> HrTimerRestart {
 //!         pr_info!("Timer called\n");
@@ -301,9 +294,12 @@
 //! }
 //!
 //! stack_pin_init!( let has_timer = IntrusiveHrTimer::new() );
-//! has_timer.as_ref().start_scoped(Delta::from_micros(200), || {
-//!     while has_timer.flag.load(ordering::Relaxed) != 1 {
-//!         has_timer.cond.wait_for_completion();
+//! let timer_pin = HrTimerPin::new(has_timer);
+//! let shared = timer_pin.as_ref();
+//!
+//! timer_pin.start_scoped(Delta::from_micros(200), || {
+//!     while shared.flag.load(ordering::Relaxed) != 1 {
+//!         shared.cond.wait_for_completion();
 //!     }
 //! });
 //!
@@ -618,7 +614,8 @@ pub trait HrTimerPointer: Sync + Sized {
 /// [`HrTimerHandle`] returned by `start` would be unsound. This is the case for
 /// stack allocated timers.
 ///
-/// Typical implementers are pinned references such as [`Pin<&T>`].
+/// Typical implementers are [`HrTimerPin`] and pinned references such as
+/// [`Pin<&mut T>`].
 ///
 /// # Safety
 ///
@@ -640,8 +637,7 @@ pub unsafe trait UnsafeHrTimerPointer: Sync + Sized {
     /// until the timer is stopped and the callback has completed.
     type TimerHandle: HrTimerHandle;
 
-    /// Start the timer after `expires` time units. If the timer was already
-    /// running, it is restarted at the new expiry time.
+    /// Start the timer after `expires` time units.
     ///
     /// # Safety
     ///
@@ -1111,7 +1107,7 @@ unsafe fn timer_container_of(
 mod arc;
 pub use arc::{ArcHrTimerHandle, HrTimerArc};
 mod pin;
-pub use pin::PinHrTimerHandle;
+pub use pin::{HrTimerPin, PinHrTimerHandle};
 mod pin_mut;
 pub use pin_mut::PinMutHrTimerHandle;
 // `box` is a reserved keyword, so prefix with `t` for timer
diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
index 4d39ef781697..f44ac07cb722 100644
--- a/rust/kernel/time/hrtimer/pin.rs
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -10,50 +10,58 @@
 use super::UnsafeHrTimerPointer;
 use core::pin::Pin;
 
-/// A handle for a `Pin<&HasHrTimer>`. When the handle exists, the timer might be
-/// running.
-pub struct PinHrTimerHandle<'a, T>
+/// A wrapper around a pinned shared reference that's guaranteed unique.
+///
+/// The `HrTimerPin` type can be thought of as a special pinned reference to an object that
+/// owns the permission to arm the [`HrTimer`] stored in the object. By ensuring that each
+/// object has only one `HrTimerPin`, the owner of it is assured exclusive access to the arming
+/// operation. Starting a timer consumes the `HrTimerPin`, and the returned
+/// [`PinHrTimerHandle`] keeps the object borrowed, so the timer cannot be armed again until the
+/// handle is dropped.
+///
+/// While this `HrTimerPin` is unique, shared pinned references to the object can still be
+/// obtained with [`HrTimerPin::as_ref`].
+///
+/// # Invariants
+///
+/// * Each object has at most one `HrTimerPin`.
+pub struct HrTimerPin<'a, T>
 where
     T: HasHrTimer<T>,
 {
-    pub(crate) inner: Pin<&'a T>,
+    pin: Pin<&'a T>,
 }
 
-// SAFETY: We cancel the timer when the handle is dropped. The implementation of
-// the `cancel` method will block if the timer handler is running.
-unsafe impl<'a, T> HrTimerHandle for PinHrTimerHandle<'a, T>
+impl<'a, T> HrTimerPin<'a, T>
 where
     T: HasHrTimer<T>,
 {
-    fn cancel(&mut self) -> bool {
-        let self_ptr: *const T = self.inner.get_ref();
-
-        // SAFETY: As we got `self_ptr` from a reference above, it must point to
-        // a valid `T`.
-        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self_ptr) };
-
-        // SAFETY: As `timer_ptr` is derived from a reference, it must point to
-        // a valid and initialized `HrTimer`.
-        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
+    /// Create a `HrTimerPin` from an exclusive pinned reference to a `T`.
+    #[inline]
+    pub fn new(inner: Pin<&'a mut T>) -> Self {
+        // INVARIANT: We have an exclusive reference, so there is no `HrTimerPin` for this
+        // object.
+        Self {
+            pin: inner.into_ref(),
+        }
     }
-}
 
-impl<'a, T> Drop for PinHrTimerHandle<'a, T>
-where
-    T: HasHrTimer<T>,
-{
-    fn drop(&mut self) {
-        self.cancel();
+    /// Get a shared pinned reference to the object.
+    ///
+    /// The returned reference can be used to access the object, but not to arm its timer.
+    #[inline]
+    pub fn as_ref(&self) -> Pin<&'a T> {
+        self.pin
     }
 }
 
 // SAFETY: We capture the lifetime of `Self` when we create a `PinHrTimerHandle`,
 // so `Self` will outlive the handle.
-unsafe impl<'a, T> UnsafeHrTimerPointer for Pin<&'a T>
+unsafe impl<'a, T> UnsafeHrTimerPointer for HrTimerPin<'a, T>
 where
     T: Send + Sync,
     T: HasHrTimer<T>,
-    T: HrTimerCallback<Pointer<'a> = Self>,
+    T: HrTimerCallback<Pointer<'a> = HrTimerPin<'a, T>>,
 {
     type TimerMode = <T as HasHrTimer<T>>::TimerMode;
     type TimerHandle = PinHrTimerHandle<'a, T>;
@@ -63,7 +71,7 @@ unsafe fn start(
         expires: <<T as HasHrTimer<T>>::TimerMode as HrTimerMode>::Expires,
     ) -> Self::TimerHandle {
         // Cast to pointer
-        let self_ptr: *const T = self.get_ref();
+        let self_ptr: *const T = self.pin.get_ref();
 
         // SAFETY:
         //  - As we derive `self_ptr` from a reference above, it must point to a
@@ -71,16 +79,53 @@ unsafe fn start(
         //  - We keep `self` alive by wrapping it in a handle below.
         unsafe { T::start(self_ptr, expires) };
 
-        PinHrTimerHandle { inner: self }
+        PinHrTimerHandle { inner: self.pin }
+    }
+}
+
+/// A handle for a `Pin<&HasHrTimer>`. When the handle exists, the timer might be
+/// running.
+pub struct PinHrTimerHandle<'a, T>
+where
+    T: HasHrTimer<T>,
+{
+    pub(crate) inner: Pin<&'a T>,
+}
+
+// SAFETY: We cancel the timer when the handle is dropped. The implementation of
+// the `cancel` method will block if the timer handler is running.
+unsafe impl<'a, T> HrTimerHandle for PinHrTimerHandle<'a, T>
+where
+    T: HasHrTimer<T>,
+{
+    fn cancel(&mut self) -> bool {
+        let self_ptr: *const T = self.inner.get_ref();
+
+        // SAFETY: As we got `self_ptr` from a reference above, it must point to
+        // a valid `T`.
+        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self_ptr) };
+
+        // SAFETY: As `timer_ptr` is derived from a reference, it must point to
+        // a valid and initialized `HrTimer`.
+        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<'a, T> Drop for PinHrTimerHandle<'a, T>
+where
+    T: HasHrTimer<T>,
+{
+    fn drop(&mut self) {
+        self.cancel();
     }
 }
 
-impl<'a, T> RawHrTimerCallback for Pin<&'a T>
+impl<'a, T> RawHrTimerCallback for HrTimerPin<'a, T>
 where
     T: HasHrTimer<T>,
     T: HrTimerCallback<Pointer<'a> = Self>,
 {
-    type CallbackTarget<'b> = Self;
+    type CallbackTarget<'b> = Pin<&'a T>;
 
     unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
         // `HrTimer` is `repr(C)`
-- 
2.43.0

[PATCH v1 3/4] 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/
Acked-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: FUJITA Tomonori <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 d94275f2e93f..59e9559e7099 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -557,27 +557,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() }
     }
 }
 
@@ -1060,6 +1069,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.
-- 
2.43.0

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

[FUJITA Tomonori]

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>
---
 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 59e9559e7099..2130dd24cccb 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -415,8 +415,12 @@
 /// # Invariants
 ///
 /// * `self.timer` is initialized by `bindings::hrtimer_setup`.
+// `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 2134d12d558c..ce7cff7efe29 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -143,7 +143,7 @@ impl<T> RawHrTimerCallback for HrTimerArc<T>
     type CallbackTarget<'a> = ArcBorrow<'a, T>;
 
     unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> 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 f44ac07cb722..6a0ac4d7dedf 100644
--- a/rust/kernel/time/hrtimer/pin.rs
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -128,7 +128,7 @@ impl<'a, T> RawHrTimerCallback for HrTimerPin<'a, T>
     type CallbackTarget<'b> = Pin<&'a T>;
 
     unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> 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 9d9447d4d57e..65172c9e55e9 100644
--- a/rust/kernel/time/hrtimer/pin_mut.rs
+++ b/rust/kernel/time/hrtimer/pin_mut.rs
@@ -86,7 +86,7 @@ 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 {
-        // `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 aa1ee31a7195..1dd68fcf2bd6 100644
--- a/rust/kernel/time/hrtimer/tbox.rs
+++ b/rust/kernel/time/hrtimer/tbox.rs
@@ -103,7 +103,7 @@ 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 {
-        // `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.43.0

Re: [PATCH 0/4] Fix forward()/expires() racing with concurrent arming

[Gary Guo]

On Thu Aug 13, 2026 at 2:48 PM BST, FUJITA Tomonori wrote:
> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>
> This series started from the review of patches 3 and 4 [1]: a hrtimer
> can be armed from any CPU at any time, including while its callback
> runs, so restricting HrTimer::expires() to the callback context is not
> by itself enough to remove the race.
>
> It turned out that expires() is not the only problem. A callback may
> also change its expiry time with hrtimer_forward(), which is sound
> only because __run_hrtimer() dequeues the timer for the duration of
> the callback. Arming the same timer from another CPU puts it back into
> the rbtree while the callback runs, so hrtimer_forward() then changes
> the expiry of a timer that is queued, without the base lock and
> without re-checking the ordering, which leaves the tree unsorted.
>
> Two of the four pointer types cannot construct that
> situation. Starting a Pin<Box<T, A>> moves the box into the handle,
> and starting a Pin<&mut T> consumes the exclusive borrow, so in both
> cases nothing is left to arm the timer with. Arc<T> is Clone and
> Pin<&T> is Copy, and both of their start functions are reachable from
> safe code, so safe Rust could arm a timer whose callback was running.
>
> "No arming while the callback runs" cannot be expressed in the type
> system, because the callback begins when the timer expires rather than
> at any point in the Rust program, so patches 1 and 2 use the stronger
> "no arming while armed" instead. hrtimer_cancel() waits for the
> handler to return, which makes that the point where the right to arm
> can be handed back. The right to arm is split out of Arc<T> into
> HrTimerArc<T> and out of Pin<&T> into HrTimerPin<'a, T>, both
> non-clonable and consumed by start, modelled on ListArc; the object
> itself stays shareable through plain Arc references and shared pinned
> references respectively.
>
> Patches 3 and 4 are the previously posted expires() and
> repr(transparent) patches, unchanged. With patches 1 and 2 in place,
> the callback context has no concurrent writer of node.expires. So
> HrTimerCallbackContext::expires() is sound.

I am thinking about this and I wonder about a different approach: the only
reason that we're having this issue, is that `expires()` call and
`forward`/`forward_now` is executed outside the protection of the base lock.

The fix is easy -- to ensure that they are executed with the base lock held.
The callback wants either:
* Do not restart the timer
* Call hrtimer_forward[_now] and restart the timer

So, if we change the order from

    unlock base
    restart = fn(timer)
    lock base
    if restart {
        queue
    }

to

    get expires
    unlock base
    restart = fn(timer, expires)
    lock base
    match restart {
        Restart(now, interval) => {
            hrtimer_forward(timer, now, interval);
            queue
        }
        NoRestart => (),
    }

then we completely eradicate this issue.

Alternatively, we can add another spinlock to protect `expires` from race
condition from within callback and concurrent restart -- that is what perf core
does: perf_mux_hrtimer_handler and perf_mux_hrtimer_restart uses the same
hrtimer_lock to prevent race.

But further complicating the type system to prevent concurrent restart sounds
like a bad approach to me.

Best,
Gary

>
> [1]: https://lore.kernel.org/rust-for-linux/20260807233039.1091842-1-tomo@flapping.org/
>
> FUJITA Tomonori (4):
>   rust: hrtimer: Introduce HrTimerArc to make arming exclusive
>   rust: hrtimer: Introduce HrTimerPin to make arming exclusive
>   rust: hrtimer: Restrict expires() to safe contexts
>   rust: hrtimer: Make HrTimer repr(transparent)
>
>  rust/helpers/time.c                 |   6 ++
>  rust/kernel/time/hrtimer.rs         | 135 ++++++++++++++++------------
>  rust/kernel/time/hrtimer/arc.rs     | 113 +++++++++++++++++------
>  rust/kernel/time/hrtimer/pin.rs     | 107 +++++++++++++++-------
>  rust/kernel/time/hrtimer/pin_mut.rs |   2 +-
>  rust/kernel/time/hrtimer/tbox.rs    |   2 +-
>  6 files changed, 249 insertions(+), 116 deletions(-)
>
>
> base-commit: 643a7c306b8ce32743d4f94dd700c8588be37e66