Re: [PATCH 6/6] rust: hrtimer: Make HrTimer repr(transparent)

[sashiko-bot@kernel.org]

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 <fujita.tomonori@gmail.com>

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/20260813134834.1562995-5-tomo@flapping.org

> 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/20260825-expires-v2-v1-0-90411c6217c7@kernel.org?part=6