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

[FUJITA Tomonori]

On Mon, 02 Nov 2026 15:31:35 +0100
Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:

> "FUJITA Tomonori" <tomo@xxxxxxxxxxxx> writes:
> 
>> From: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
> 
> This looks good to me.
> 
> Reviewed-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>

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.

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