Re: [PATCH 1/6] hrtimer: add expiry injecting callback variant

["Gary Guo" <gary@garyguo.net>]

On Tue Sep 29, 2026 at 9:56 PM BST, Thomas Gleixner wrote:
> On Tue, Aug 25 2026 at 14:16, Andreas Hindborg wrote:
>> A hrtimer callback may modify its own expiry with hrtimer_forward()
>> and read it with hrtimer_get_expires(). Both run after
>> __run_hrtimer() has dropped the cpu_base->lock, so they race with a
>> concurrent hrtimer_start_range_ns() on another CPU, which rewrites
>> node.expires under the base lock and requeues the timer:
>>
>> - The unlocked read-modify-write of node.expires in
>>   hrtimer_forward() is a data race against the locked write in the
>>   start path.
>>
>> - The is_queued check in hrtimer_forward() is racy with
>>   a time-of-check-time-of-use bug as well: a concurrent
>>   start can enqueue the timer between the check and the expiry
>>   update, and forwarding an already queued timer changes the expiry
>>   of a node inside the timerqueue without re-sorting, leaving the
>>   tree unordered.
>>
>> Timer users which both forward in the callback and arm from other contexts
>> must provide their own serialization, e.g. perf's cpc->hrtimer_lock plus
>> hrtimer_active flag, see commit 4cfafd3082af ("sched,perf: Fix periodic
>> timers"). The requirement is subtle and not enforced; i915_pmu and taprio
>> currently get it wrong. For the Rust hrtimer abstraction it is a soundness
>> problem: safe code can arm a timer whose callback is running, so callback
>> context forward and expiry reads cannot be offered as safe API.
>>
>> Add an alternative callback variant that removes the race
>> structurally instead of requiring serialization. An expiry injecting
>> callback receives the expiry snapshotted under the base lock by
>> value and, to restart the timer, fills a struct hrtimer_forward_args
>> and returns HRTIMER_RESTART. __run_hrtimer() then applies the
>> forward and the enqueue with the base lock held. The callback never
>> accesses live timer state.
>>
>> If a concurrent start enqueued the timer while the callback ran, the
>> restart request is discarded and the start wins, matching the
>> existing "restart == HRTIMER_RESTART && !timer->is_queued" handling
>> for classic callbacks. The is_queued check is reliable here: while
>> base->running == timer, hrtimer_try_to_cancel() bails out before
>> remove_hrtimer() and the timer cannot switch bases, so only a
>> concurrent start can enqueue it, and the start path writes the
>> expiry and is_queued in the same critical section. Thus !is_queued
>> at requeue time guarantees the expiry still equals the snapshot
>> handed to the callback: the deferred hrtimer_forward() cannot hit
>> its concurrent start check, and an overrun count the callback
>> derived from the snapshot is consistent with the forward that is
>> applied.
>>
>> The new callback pointer shares storage with the classic one in an
>> anonymous union, discriminated by a new is_ext flag placed in
>> existing padding; sizeof(struct hrtimer) is unchanged and the
>> classic callback path is unaffected. hrtimer_update_function()
>> rejects timers with an expiry injecting callback.
>
> Aside of the horrible name (what is "ext"?) the whole mechanism is
> creating a false sense of "safety" as the same problem exists when the
> timer callback simply sets the new expiry time without invoking
> hrtimer_forward().
>
> Also why is Rust special and wants to delegate the external
> serialization requirement to the hrtimer core code and thereby adding a
> boatload of extra conditionals into the hotpath to distinguish between
> the two callback variants?
>
> Yes, you found an example for the downside of this design in the i915
> code. That's not surprising because i915 is generally known for ignoring
> documentation and making up their own rules just because they can. So
> I'm not accepting this as an argument at all.
>
> I agree with you that the lack of enforcement of that external
> serialization rule is suboptimal. External serialization is hard to
> validate/enforce in general though it's not impossible.
>
> But let's first take a step back and look at the larger picture.
>
> The obvious question is:
>
>    Why is the base lock dropped when invoking the callback?
>
> The answer is simply that the callback might and in many cases will
> acquire a lock which is used in the reverse lock order for the purpose
> of external serialization.
>
> If you think about that then it's pretty obvious that you can introduce
> a safe variant of hrtimer_forward() which can be invoked from the
> callback:
>
> hrtimer_forward_safe_from_callback(timer)
> {
>         base = lock_running_timer_base(timer);
>         if (!hrtimer_is_queued(timer))
>         	hrtimer_forward(timer);
>         unlock_timer_base(base);
> }

FWIW we did discuss about this option. Here's the list of all options that we
come up with:

1. Take base lock before calling forward and release it afterward (the one you
   mentioned above). This one requires exposing the base lock (at least to Rust
   abstraction).

2. Prevent timer operation from within the callback and have hrtimer core doing
   it (Andreas's patch).

3. Force users to stop the timer before re-arming. This was dismissed because
   stopping the timer will require waiting for the callback, and this is prone to
   deadlock condition.

4. Add a lock to Rust side that all users must take to forward / start. We don't
   want to add a new lock just for this, and taking base lock would be more
   favourable.

We consider (2) the best option because it avoids several pitfalls with (1):

- It is impossible to forget to forward and return restart, or forward but
  return norestart.

- The base lock is not unlocked before relocked, so the forward and restarting
  is atomic. So it's impossible to have the condition where the forward occurs,
  but before returning RESTART, the hrtimer is concurrently queued.

- Unlock/relock have an overhead (we didn't quantify how much impact this will
  be, though).

I supposed another alternative is to add a mode in hrtimer core where the base
lock is not unlocked before calling the callback, and expose base lock APIs
(like option 1) so that the Rust hrtimer abstraction can unlock it before
calling the callbacks.

Best,
Gary

>
> That prevents the scenario you described in a completely safe way, no?
>
> Coming back to validation/enforcement of external serialization. That's
> a problem which has been solved in other places already. Look at the
> seqlock code for inspiration.