Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] The patch adds an inline comment and commit message justifying a layout change based on `HrTimerCallbackContext`, a type that was completely removed earlier in the patch series, resulting in a stale and confusing soundness rationale. Pre-existing issues: - [High] The callback implementations for `Pin<&mut T>` and `Pin<Box<T>>` create a mutable reference (`&mut T`) while `cancel()` can concurrently access `T.timer` via a raw pointer, causing Undefined Behavior due to aliasing violations. -- --- Patch [6]: [PATCH 6/6] rust: hrtimer: Make HrTimer repr(transparent) --- commit ed14b61ad275b0efb05d77ed06f7d0d89b06560d Author: FUJITA Tomonori <[email protected]> rust: hrtimer: Make HrTimer repr(transparent) This commit changes the representation of HrTimer from repr(C) to repr(transparent). This prevents adding multiple fields to the struct and helps enforce soundness regarding shared and mutable references. Link: https://msgid.link/[email protected] > HrTimerCallbackContext acquires a &HrTimer<T> from a > NonNull<HrTimer<T>> while a &mut HrTimer<T> can exist at the same > time. This is sound only because HrTimer's sole field is [Severity: Medium] Is this justification accurate? The HrTimerCallbackContext type appears to have been removed earlier in this patch series. Should the commit message be updated to reflect the current design? > Opaque<bindings::hrtimer>, which puts every byte behind an UnsafeCell. [Severity: High] This is a pre-existing issue, but does relying on Opaque and UnsafeCell to justify aliasing with a mutable reference risk undefined behavior? The commit message notes that a mutable reference to HrTimer can exist at the same time as other accesses. While UnsafeCell relaxes aliasing for shared references, creating an exclusive mutable reference asserts exclusive access to the entire memory footprint. When RawHrTimerCallback::run executes for pinned mutable references or boxed timers, it creates an exclusive mutable reference. Concurrently, a user thread might call cancel, which reads the timer memory via a raw pointer. This concurrent read would invalidate the uniqueness tag of the active mutable reference, which could cause the compiler to misoptimize. > diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs > index 2a9abc9f5d8c8..ab7c568b88553 100644 > --- a/rust/kernel/time/hrtimer.rs > +++ b/rust/kernel/time/hrtimer.rs > @@ -427,8 +427,12 @@ > /// # Invariants > /// > /// * `self.timer` is initialized by `bindings::hrtimer_setup_ext`. > +// `repr(transparent)` is not merely about layout. `HrTimerCallbackContext` > acquires a > +// `&HrTimer<T>` while a `&mut HrTimer<T>` may exist, which is sound only > because every byte of > +// this type sits inside `Opaque`. Being transparent rejects a second field > at compile time, > +// but it does not enforce that the remaining field stays `Opaque`. [Severity: Medium] Is this comment still accurate? As mentioned regarding the commit message, the HrTimerCallbackContext type seems to no longer exist in the tree. Should this inline documentation be updated to use a current example? > #[pin_data] > -#[repr(C)] > +#[repr(transparent)] > pub struct HrTimer<T> { [ ... ] > diff --git a/rust/kernel/time/hrtimer/pin_mut.rs > b/rust/kernel/time/hrtimer/pin_mut.rs > index 04f9d8cbddcd2..2bba3c41d6e9a 100644 > --- a/rust/kernel/time/hrtimer/pin_mut.rs > +++ b/rust/kernel/time/hrtimer/pin_mut.rs > @@ -91,7 +91,7 @@ impl<'a, T> RawHrTimerCallback for Pin<&'a mut T> > expires: bindings::ktime_t, > fwd: *mut bindings::hrtimer_forward_args, > ) -> bindings::hrtimer_restart { > - // `HrTimer` is `repr(C)` > + // `HrTimer` is `repr(transparent)` > let timer_ptr = ptr.cast::<HrTimer<T>>(); [Severity: High] This isn't a bug introduced by this patch, but does the rest of this callback function violate aliasing rules? A bit further down in RawHrTimerCallback::run for pinned mutable references and boxed timers, the code creates an exclusive mutable reference from the pointer. If a user thread concurrently calls cancel while the timer is running, that cancellation reads the timer base via a parent raw pointer. Because creating the mutable reference asserts exclusive access to the whole memory footprint, the concurrent read from cancel would invalidate that reference and could lead to undefined behavior. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
