Re: [PATCH v3] rust: hrtimer: Restrict expires() to safe contexts

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

FUJITA Tomonori <tomo@flapping.org> writes:

> On Thu, 18 Jun 2026 11:56:59 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "FUJITA Tomonori" <tomo@aliasing.net> writes:
>> 
>>> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>>
>>> HrTimer::expires() previously read node.expires via a volatile load, which
>>> can race with C-side updates. Rework the API so it is only callable with
>>> exclusive access or from the callback context.
>>>
>>> Introduce expires_unchecked() with an explicit safety contract, switch
>>> HrTimer::expires() to Pin<&mut Self>, add
>>> HrTimerCallbackContext::expires(), and route the read through
>>> hrtimer_get_expires() via a Rust helper.
>>>
>>> Fixes: 4b0147494275 ("rust: hrtimer: Add HrTimer::expires()")
>>> Closes: https://lore.kernel.org/rust-for-linux/87ldi7f4o1.fsf@t14s.mail-host-address-is-not-set/
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> 
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> One thing I'd like to double check before merging this,
> HrTImerCallbackContext::expires() now does:
>
> unsafe { self.0.as_ref().expires_unchecked() }
>
> which briefly makes a &HrTimer<T> from the NonNull<HrTimer<T>>.
>
> This is only sound today because `HrTimer<T>`'s sole field is
> `Opaque<bindings::hrtimer>`. As you pointed out earlier, if we add a
> field to HrTimer, "stuff breaks".
>
> Should `expires_unchecked` just keep taking `self_ptr: *const Self`,
> like `raw_forward()` (and like v2 did), so
> `HrTimerCallbackContext::expires()` can stay on `self.0.as_ptr()` and
> never form the reference at all?

Either way is fine for me, but Gary seems to gravitate towards the
current solution, so maybe keep that? We can add a "// NOTE:" on the
struct definition saying that soundness depends on all fields being OK
with this caveat.


Best regards,
Andreas Hindborg