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 <pbonz...@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-7c99ed862...@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 <pbonz...@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....@intel.com>
> Signed-off-by: Paolo Bonzini <pbonz...@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 <pbonz...@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 <pbonz...@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
>
>
>
