Re: [PATCH v1 2/2] rust: hrtimer: Use hrtimer_forward_safe_from_callback()

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

FUJITA Tomonori <tomo@flapping.org> writes:

> On Mon, 02 Nov 2026 15:31:35 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "FUJITA Tomonori" <tomo@flapping.org> writes:
>> 
>>> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>>
>>> Safe Rust code can start a timer while its callback runs. A timer
>>> started through Arc<T> or Pin<&T> can be started again from another
>>> clone or copy of the pointer. HrTimerCallbackContext::forward() calls
>>> hrtimer_forward(), which writes the expiry without the base lock, so
>>> the write can race with the concurrent hrtimer_start().
>>>
>>> Call hrtimer_forward_safe_from_callback() instead. It does the update
>>> with the base lock held. If the timer is already queued by a concurrent
>>> start, the expiry is left alone and 0 is returned.
>>>
>>> HrTimer::forward() keeps calling hrtimer_forward(). It takes
>>> Pin<&mut Self>, so no other reference to the timer exists and no
>>> concurrent start is possible. Narrow the safety requirement of
>>> raw_forward() to exclusive access, since the callback context no longer
>>> uses it.
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> 
>> This looks good to me.
>> 
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> Thanks!
>
>> While reading this, I thought we should expand the safety requirement
>> for the handle trait, because even though handles keep the callback
>> target alive, they do not own the target. The armed timer does. And so a
>> (future) &mut method on the handle is not allowed to mutably access the
>> target. This implicit today, but I thought maybe we should make it
>> explicit.
>
> I agree that we should make this explicit. It does not depend on this
> series, so could you send it as a separate patch? A few comments below.

Yes, I did not intend to hijack this series. Just wanted your input.

>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> index ab7c568b8855..84f45a7a3a17 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -771,6 +771,14 @@ fn run(
>>  /// is running. If the timer handler is running when the handle is dropped, the
>>  /// drop method must wait for the handler to return before returning.
>>  ///
>> +/// While the timer is armed, the callback target is borrowed by the timer: the
>> +/// timer callback can create a reference to the target at any time, and for
>> +/// some pointer types that reference is exclusive.
>
> The module doc defines the timer states as stopped, started and
> running, and does not use "armed". Could we use these terms here?

Yes, good point.

> The
> callback creates the reference only in the running state, but the
> requirement needs to hold in both the started and running states,
> because a started timer can move to the running state at any time.

>
>> +/// Implementers must not create
>> +/// a reference to the callback target that would conflict with the reference
>> +/// that [`RawHrTimerCallback::run`] creates for the pointer type in question.
>> +/// Deriving raw pointers from the target, as the [`Self::cancel`]
>> +/// implementations do, always satisfies this.
>
> Even without creating a reference, an access through a raw pointer can
> be UB, for example a write to a field that the callback also uses.

Right, but creating the pointer and using it for offset calculations is
fine. I can try to make that more clear.

> `cancel` is fine because it only accesses the `HrTimer` field, which
> is wrapped in `Opaque`, and hrtimer_cancel() takes the base lock. So I
> think the requirement should be about accesses, not about references.

I see. I'll try to rephrase a bit.

Best regards,
Andreas Hindborg