Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements

[Zhao Liu <zhao1.liu@intel.com>]

On Mon, Mar 03, 2025 at 04:58:57PM +0100, Paolo Bonzini wrote:
> Date: Mon, 3 Mar 2025 16:58:57 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and
>  express pinning requirements
> 
> On 3/3/25 14:48, Zhao Liu wrote:
> > > @@ -156,7 +157,7 @@ pub struct HPETTimer {
> > >       /// timer N index within the timer block (`HPETState`)
> > >       #[doc(alias = "tn")]
> > >       index: usize,
> > > -    qemu_timer: Option<Box<Timer>>,
> > > +    qemu_timer: Option<Pin<Box<Timer>>>,
> > 
> > I'm removing this Option<> wrapper in migration series. This is because
> > Option<> can't be treated as pointer as you mentioned in [*].
> > 
> > So for this reason, does this mean that VMStateField cannot accept
> > Option<>? I realize that all the current VMStateFlags don't seem
> > compatible with Option<> unless a new flag is introduced.
> > 
> > [*]: https://lore.kernel.org/qemu-devel/9a0389fa-765c-443b-ac2f-7c99ed862982@redhat.com/
> 
> Ok, so let's get rid of the option.  I didn't really like it anyway...
> 
> If the Timer is embedded in the HPETTimer, there needs to be some
> "unsafe" in order to make sure that the pinning is observed, and also
> because an uninitialized Timer is bad and can cause a NULL pointer
> dereference in modify()... i.e. Timer shouldn't have implemented
> Default!

Yes! Good point.

> However, the lifetime checks in init_full() are preserved, so overall
> this is better---at least for now.  Linux also had unsafe initialization
> for quite some time, so I'm okay with it.

The overall design is okay for me too.

> The replacements for this patch are below.

I have comments about current Opaque<> implemation below...
  
> From 2d74bdf176b2fbeb6205396d0021f68a9e72bde1 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@redhat.com>
> Date: Mon, 3 Mar 2025 16:27:08 +0100
> Subject: [PATCH 1/2] rust: hpet: embed Timer without the Option and Box
>  indirection
> 
> This simplifies things for migration, since Option<Box<QEMUTimer>> does not
> implement VMState.
> 
> This also shows a soundness issue because Timer::new() will leave a NULL
> timer list pointer, which can then be dereferenced by Timer::modify().  It
> will be fixed shortly.

Good catch!

> Suggested-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/timer/hpet/src/hpet.rs | 59 ++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 31 deletions(-)

Thanks. This cleanup is fine for me!

...

> From 276020645786b6537c50bb37795f281b5d630f27 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@redhat.com>
> Date: Fri, 14 Feb 2025 12:06:13 +0100
> Subject: [PATCH 2/2] rust: timer: wrap QEMUTimer with Opaque<> and express
>  pinning requirements
> 
> Timers must be pinned in memory, because modify() stores a pointer to them
> in the TimerList.  To express this requirement, change init_full() to take
> a pinned reference.  Because the only way to obtain a Timer is through
> Timer::new(), which is unsafe, modify() can assume that the timer it got
> was later initialized; and because the initialization takes a Pin<&mut
> Timer> modify() can assume that the timer is pinned.  In the future the
> pinning requirement will be expressed through the pin_init crate instead.
> 
> Note that Timer is a bit different from other users of Opaque, in that
> it is created in Rust code rather than C code.  This is why it has to
> use the unsafe constructors provided by Opaque; and in fact Timer::new()
> is also unsafe, because it leaves it to the caller to invoke init_full()
> before modify().  Without a call to init_full(), modify() will cause a
> NULL pointer dereference.
> 
> An alternative could be to combine new() + init_full() by returning a
> pinned box; however, using a reference makes it easier to express
> the requirement that the opaque outlives the timer.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  meson.build                    |  7 -----
>  rust/hw/timer/hpet/src/hpet.rs | 10 ++++++--
>  rust/qemu-api/src/timer.rs     | 47 ++++++++++++++++++++++++++--------
>  3 files changed, 44 insertions(+), 20 deletions(-)

...

>  impl Timer {
>      pub const MS: u32 = bindings::SCALE_MS;
>      pub const US: u32 = bindings::SCALE_US;
>      pub const NS: u32 = bindings::SCALE_NS;
> -    pub fn new() -> Self {
> -        Default::default()
> -    }
> -
> -    const fn as_mut_ptr(&self) -> *mut Self {
> -        self as *const Timer as *mut _
> +    /// Create a `Timer` struct without initializing it.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The timer must be initialized before it is armed with
> +    /// [`modify`](Self::modify).
> +    pub unsafe fn new() -> Self {
> +        // SAFETY: requirements relayed to callers of Timer::new
> +        Self(unsafe { Opaque::zeroed() })

We should use Opaque::uninit()? Because MaybeUninit::<bindings::QEMUTimer>::zeroed()
marks the timer as initialized, which disables MaybeUninit's ability to check for
initialization. e.g.,

// No compiling error or runtime panic
let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::zeroed();
let _t = unsafe { t.assume_init() };

Further more, I spent some time trying to figure out if MaybeUninit in
Opaque<> could help identify UB caused by uninitialized Timer, but I found
it doesn't work. :-(

There're 2 cases:

// No compiling error or runtime panic
let mut v: UnsafeCell<MaybeUninit<bindings::QEMUTimer>> = UnsafeCell::new(MaybeUninit::uninit());
let _v = unsafe { v.get_mut().assume_init() };

// Runtime panic: Illegal instruction
let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::uninit();
let _t = unsafe { t.assume_init() };

I understand that the outer UnsafeCell wrapper makes MaybeUninit's
checks not work.

But when I adjust MaybeUninit as the outer wrapper, the UB check can
work:

// Runtime panic: Illegal instruction
let v: MaybeUninit<UnsafeCell<bindings::QEMUTimer>> = MaybeUninit::uninit();
let _v = unsafe { v.assume_init() };

And there's another example:

https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#method.raw_get

Compared with linux's Opaque, it also puts MaybeUninit on the outermost
layer.

Emm, I guess now we have UnsafeCell<MaybeUninit<>> because interior
mutability is expected... but this layout breaks MaybeUninit's functionality.

> +    /// Create a new timer with the given attributes.
>      pub fn init_full<'timer, 'opaque: 'timer, T, F>(
> -        &'timer mut self,
> +        self: Pin<&'timer mut Self>,
>          timer_list_group: Option<&TimerListGroup>,
>          clk_type: ClockType,
>          scale: u32,
> @@ -51,7 +71,7 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
>          // SAFETY: the opaque outlives the timer
>          unsafe {
>              timer_init_full(
> -                self,
> +                self.as_mut_ptr(),
>                  if let Some(g) = timer_list_group {
>                      g as *const TimerListGroup as *mut _
>                  } else {
> @@ -67,14 +87,19 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
>      }
>      pub fn modify(&self, expire_time: u64) {
> +        // SAFETY: the only way to obtain a Timer safely is via methods that
> +        // take a Pin<&mut Self>, therefore the timer is pinned

The SAFETY should also be ensured by MaybeUninit, I think. But I haven't
verified if MaybeUninit<UnsafeCell<>> can work on FFI case...

>          unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
>      }
>      pub fn delete(&self) {
> +        // SAFETY: the only way to obtain a Timer safely is via methods that
> +        // take a Pin<&mut Self>, therefore the timer is pinned
>          unsafe { timer_del(self.as_mut_ptr()) }
>      }
>  }
> +// FIXME: use something like PinnedDrop from the pinned_init crate
>  impl Drop for Timer {
>      fn drop(&mut self) {
>          self.delete()
> -- 
> 2.48.1
> 
> 
>