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

"Gary Guo" <[email protected]> writes:

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

With this solution we would have to restrict calls to `forward` and
`expires`. Maybe that would be OK, but it would be restricting the API
further.

As I understand the problem space, we have (on Rust side):

 - `start` and `forward` may race. `forward` is callable on exclusive
   reference to HrTimer or in callback context, but otherwise lacks
   synchronization. `start` is serialized on the base lock but is
   callable at any time.
 - `start` and `expires` may race because `start` writes the expiration and
   `expires` reads it. The latter has no synchronization and is callable
   on shared reference to `HrTimer`.
 - `forward` and `expires` may race because `HrTimer::expires` takes a
   shared reference and is callable at any time concurrently.

I think the solution suggested by Tomo is OK, but we could also add
synchronization to `start`, `forward` and `expires` on the rust side.
Would that not solve the problem for us?

This way we can still run the handler without lock. Only if we call
`forward` or read the expiry in the handler would we take the lock.

This would allow the API as originally described on the rust side.


Best regards,
Andreas Hindborg