Re: [PATCH 3/6] rust: hrtimer: use the expiry injecting callback variant

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Safe Rust code can trigger a kernel WARN_ON_ONCE (and panic) by returning a zero interval in `HrTimerRestart::Forward`.
- [Low] Missing `#[inline]` attribute on small abstraction functions.
--

--- Patch [3]: [PATCH 3/6] rust: hrtimer: use the expiry injecting callback variant ---
commit 237e017d8140a6e80f2fd3f4c1d9d1b8d7532ab6
Author: Andreas Hindborg <[email protected]>

rust: hrtimer: use the expiry injecting callback variant
    
A timer callback could modify the expiry of its timer with
HrTimerCallbackContext::forward(). This switches the abstraction to the expiry
injecting callback variant provided by hrtimer_setup_ext(). The callback
receives the expiry by value, snapshotted under the timer base lock, and
requests a restart by returning HrTimerRestart::Forward { now, interval }.

Link: https://lore.kernel.org/r/[email protected]

> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index 2d7f1131a8131..e6570a6162035 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs

[ ... ]

> @@ -847,19 +847,62 @@ unsafe fn start(this: *const Self, expires: <Self::TimerMode as HrTimerMode>::Ex

[ ... ]

> +impl<T: HasHrTimer<T>> HrTimerRestart<T> {
> +    /// Request that the timer be forwarded past the current time by `interval`
> +    /// and restarted.
> +    pub fn forward_now(interval: Delta) -> Self {

[Severity: Low]
This isn't a bug, but should this function be annotated with #[inline]?

The Rust subsystem coding guidelines mandate that small abstraction functions
should have this annotation.

> +        Self::Forward {
> +            now: HrTimerInstant::<T>::now(),
> +            interval,
> +        }
> +    }
> +
> +    /// Convert to the C representation, filling `fwd` with the forward
> +    /// request.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `fwd` must be valid for writing a `bindings::hrtimer_forward_args`.
> +    pub(crate) unsafe fn into_c(

[Severity: Low]
This isn't a bug, but should this small conversion function also be annotated
with #[inline]?

> +        self,
> +        fwd: *mut bindings::hrtimer_forward_args,
> +    ) -> bindings::hrtimer_restart {
> +        match self {
> +            Self::NoRestart => bindings::hrtimer_restart_HRTIMER_NORESTART,
> +            Self::Forward { now, interval } => {
> +                // SAFETY: By our safety contract, `fwd` is valid for writing.
> +                unsafe {
> +                    *fwd = bindings::hrtimer_forward_args {
> +                        now: now.as_nanos(),
> +                        interval: interval.as_nanos(),
> +                    }
> +                };

[Severity: High]
Does this code allow safe Rust to trigger a kernel panic?

If a safe Rust module constructs HrTimerRestart::Forward with a zero interval
(e.g., via HrTimerRestart::forward_now(Delta::ZERO)) and returns it from the
timer callback, the zero interval is blindly copied into fwd.interval here.

The C side execution in __run_hrtimer() will then trigger a WARN_ON_ONCE:

kernel/time/hrtimer.c:__run_hrtimer() {
    /* An expiry injecting callback requesting a restart must forward. */
    if (fn_ext && restart == HRTIMER_RESTART && WARN_ON_ONCE(!fwd.interval))
        restart = HRTIMER_NORESTART;
    ...
}

If the kernel is configured with panic_on_warn, this could lead to a kernel
panic.

Should there be a check to validate that the interval is non-zero before
passing it to C, or should it be enforced when creating
HrTimerRestart::Forward?

> +                bindings::hrtimer_restart_HRTIMER_RESTART
> +            }
> +        }
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3