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

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

"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.


Best regards,
Andreas Hindborg


[1] https://doc.rust-lang.org/std/ptr/fn.read_volatile.html