Re: [PATCH v2 2/8] rust: hrtimer: Add HrTimer::raw_forward() and forward()

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

"Lyude Paul" <lyude@redhat.com> writes:

> On Wed, 2025-04-23 at 14:13 +0200, Andreas Hindborg wrote:
>> Lyude Paul <lyude@redhat.com> writes:
>> >
>> > +};
>> >  use core::marker::PhantomData;
>> >  use pin_init::PinInit;
>> >
>> > @@ -164,6 +168,36 @@ pub(crate) unsafe fn raw_cancel(this: *const Self) -> bool {
>> >          // handled on the C side.
>> >          unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
>> >      }
>> > +
>> > +    /// Forward the timer expiry for a given timer pointer.
>> > +    ///
>> > +    /// # Safety
>> > +    ///
>> > +    /// `self_ptr` must point to a valid `Self`.
>>
>> I don't think safety requirements are tight enough. We must also have
>> exclusive ownership of the pointee of `self_ptr`.
>
> Are we sure "exclusive ownership" is the right term here? We /technically/ can
> be considered to have unique access over the time expiry since we allow racy
> reads of it, and we only allow writes in situations where the timer access is
> exclusive and the timer isn't started - or from the timer callback itself when
> the timer is started. But we don't have the guarantee of unique access to
> `Self`, and both context and context-less forward() make use of raw_forward()
> since otherwise I wouldn't have really added a raw_ variant in the first
> place.

The function must be safe to call whenever the safety requirements are
satisfied. `self_ptr` pointing to a valid `Self` is not enough for this.
We must also satisfy the conditions for calling
`bindings::hrtimer_forward`, which are a) we are in timer context, or b)
there are no other threads modifying the timer. a) implies b) because
the hrtimer framework holds a lock in timer context, if I recall
correctly.

We can satisfy these by requiring exclusive access, right? In timer
context, it is given by C API contract, outside, we just have to have
&mut ref to the timer.


Best regards,
Andreas Hindborg