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

On Thu Aug 13, 2026 at 2:48 PM BST, FUJITA Tomonori wrote:
> From: FUJITA Tomonori <[email protected]>
>
> 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/[email protected]/
>
> 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