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

[Andreas Hindborg <a.hindborg@kernel.org>]

FUJITA Tomonori <tomo@flapping.org> writes:

> On Mon, 17 Aug 2026 17:26:41 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "Gary Guo" <gary@garyguo.net> writes:
>> 
>>> 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.
>> 
>> 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.
>
> That would work, but I think it needs more than the lock. The lock makes
> start and forward safe against each other, but one of them still loses. If
> start runs first, hrtimer_forward() returns 0 and does nothing, so the
> overrun it would have returned is lost. Some callers use that return
> value.

We can put the lock on the rust side of things. Existing C callers would
not be affected. Calling `forward` from the handler and racing a `start`
from outside needs handling anyway. The zero return value would be an
indicator.

> perf and CFS bandwidth have a flag as well as a lock. The flag is "do
> not arm while armed", which is the same rule the types enforce
> here. rtc and the softlockup watchdog look like they cancel first and
> then start instead. None of them arms a timer that is active, so I
> would rather the abstraction did not allow it either. Does that seem
> reasonable?

I am fine with preventing starting a timer that is Started or Running,
but I am not liking the `UniqueArc` requirement.

I have a use case in `rnull` where I have to start a timer behind an
`Arc` with no way to obtain a `UniqueArc`, so I would prefer if that use
case keeps on working. Without this, I would have to allocate a box and
put it behind a lock, leading to double indirection.

If we can fold in this logic into the API, callers can be simpler.

Best regards,
Andreas Hindborg