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
