[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <877btdpyz2.fsf@t14s.mail-host-address-is-not-set>
Date: Mon, 19 Jan 2026 13:29:37 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, ojeda@...nel.org
Cc: aliceryhl@...gle.com, anna-maria@...utronix.de,
bjorn3_gh@...tonmail.com, boqun.feng@...il.com, dakr@...nel.org,
frederic@...nel.org, gary@...yguo.net, jstultz@...gle.com,
lossin@...nel.org, lyude@...hat.com, sboyd@...nel.org, tglx@...utronix.de,
tmgross@...ch.edu, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v1] rust: hrtimer: Restrict expires() to safe contexts
"FUJITA Tomonori" <fujita.tomonori@...il.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@...il.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.
Best regards,
Andreas Hindborg
[1] https://doc.rust-lang.org/std/ptr/fn.read_volatile.html
Powered by blists - more mailing lists