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

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Jan 19, 2026 at 01:07:58PM +0000, Gary Guo wrote:
> On Mon Jan 19, 2026 at 12:29 PM GMT, Andreas Hindborg wrote:
> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> >
> >> 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 raw_expires() 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.
> >>
> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> >
> > Patch looks good to me, but I just want to check with Lyude about their
> > use case in the rvkms driver. I think that is why we did the racy
> > implementation originally. In C we have stuff like this:
> >
> >
> >     /**
> >     * drm_crtc_vblank_get_vblank_timeout - Returns the vblank timeout
> >     * @crtc: The CRTC
> >     * @vblank_time: Returns the next vblank timestamp
> >     *
> >     * The helper drm_crtc_vblank_get_vblank_timeout() returns the next vblank
> >     * timestamp of the CRTC's vblank timer according to the timer's expiry
> >     * time.
> >     */
> >     void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
> >     {
> >       struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> >       struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
> >       u64 cur_count;
> >       ktime_t cur_time;
> >
> >       if (!READ_ONCE(vblank->enabled)) {
> >         *vblank_time = ktime_get();
> >         return;
> >       }
> >
> >       /*
> >       * A concurrent vblank timeout could update the expires field before
> >       * we compare it with the vblank time. Hence we'd compare the old
> >       * expiry time to the new vblank time; deducing the timer had already
> >       * expired. Reread until we get consistent values from both fields.
> >       */
> >       do {
> >         cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
> >         *vblank_time = READ_ONCE(vtimer->timer.node.expires);
> >       } while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
> >
> >       if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
> >         return; /* Already expired */
> >
> >       /*
> >       * To prevent races we roll the hrtimer forward before we do any
> >       * interrupt processing - this is how real hw works (the interrupt
> >       * is only generated after all the vblank registers are updated)
> >       * and what the vblank core expects. Therefore we need to always
> >       * correct the timestamp by one frame.
> >       */
> >       *vblank_time = ktime_sub(*vblank_time, vtimer->interval);
> >     }
> >     EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);
> >
> >
> > Also, we got some new docs for `read_volatile` that allow us to read
> > memory outside Rust of any allocation that are not "valid for read" [1],
> > meaning racy reads are OK as far as I understand. So the original
> > implementation might actually be OK, although the number might not be
> > correct always.
> 
> The wording is for MMIO and should not be relied on if the accessed memory is
> C memory. Also, `HrTimer` is going to be a Rust allocation.
> 

Right, "memory outside Rust AM" should mean memory that outside both
Rust and C in kernel (i.e. userspace, MMIO, etc.) It's not a
"out-of-UB" free card.

> Even if we don't treat it Rust allocation, it's also only "fine" in a sense that
> you don't get UB for doing it. But the value you read can still be completely
> meaningless if the updater is not atomic (it would be valid compiler
> implementation to, say, turn a non-atomic write into a write of a garbage value
> and then an overwrite of the actual data).
> 
> I think the usage you quoted is just wrong, as on 32-bit platforms this could
> well read a teared value.
> 

Agreed. It's a data race any way, unless there is a proof that ->exipres
won't be updated during read.

Regards,
Boqun

> Best,
> Gary