This patchset introduces the singleton interface for QOM.

The singleton interface is as simple as "this class can only create one
instance".

We used to have similar demand when working on all kinds of vIOMMUs,
because in most cases that I am aware of, vIOMMU must be a singleton as
it's closely bound to the machine and also the root PCIe systems.  We used
to have a bunch of special code guarding those, for example, X86 has
pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
than once.

There is a similar demand raising recently (even if the problem existed
over years) when we were looking at a migration bug, that part of it was
involved with the current_migration global pointer being referenced
anywhere, even after the migration object was finalize()ed.  So far without
singleton support, there's no way to reset the variable properly.
Declaring migration object to be a singleton also just makes sense, e.g.,
dynamically creating migration objects on the fly with QMP commands doesn't
sound right..

The idea behind is pretty simple: any object that can only be created once
can now declare the TYPE_SINGLETON interface, then QOM facilities will make
sure it won't be created more than once.  For example, qom-list-properties,
device-list-properties, etc., will be smart enough to not try to create
temporary singleton objects now.  Meanwhile, we also guard device-add paths
so that it'll fail properly if it's created more than once (and iff it's a
TYPE_DEVICE first).

The 1st patch introduces the QOM interface, while I made both x86-iommu and
migration as the initial users of it, so it's really more about the 1st
patch to discuss first.  I didn't yet touch ARM/SMMU as it's also more
complicated; I also didn't yet dig anything else that may apply, but I do
feel like this can apply more than the two attached here.  Hopefully the
two use cases can be good enough to start the discussion.

Thanks,

Peter Xu (4):
  qom: TYPE_SINGLETON interface
  x86/iommu: Make x86-iommu a singleton object
  migration: Make migration object a singleton object
  migration: Reset current_migration properly

 include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
 hw/i386/x86-iommu.c             | 14 ++++++++++
 migration/migration.c           | 36 ++++++++++++++++++++++---
 qom/object.c                    |  3 +++
 qom/object_interfaces.c         | 24 +++++++++++++++++
 qom/qom-qmp-cmds.c              | 22 ++++++++++++---
 system/qdev-monitor.c           |  7 +++++
 7 files changed, 147 insertions(+), 6 deletions(-)

-- 
2.45.0

Peter Xu <peterx@redhat.com> writes:

> This patchset introduces the singleton interface for QOM.
>
> The singleton interface is as simple as "this class can only create one
> instance".
>
> We used to have similar demand when working on all kinds of vIOMMUs,
> because in most cases that I am aware of, vIOMMU must be a singleton as
> it's closely bound to the machine and also the root PCIe systems.  We used
> to have a bunch of special code guarding those, for example, X86 has
> pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> than once.
>
> There is a similar demand raising recently (even if the problem existed
> over years) when we were looking at a migration bug, that part of it was
> involved with the current_migration global pointer being referenced
> anywhere, even after the migration object was finalize()ed.  So far without
> singleton support, there's no way to reset the variable properly.
> Declaring migration object to be a singleton also just makes sense, e.g.,
> dynamically creating migration objects on the fly with QMP commands doesn't
> sound right..
>
> The idea behind is pretty simple: any object that can only be created once
> can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> sure it won't be created more than once.  For example, qom-list-properties,
> device-list-properties, etc., will be smart enough to not try to create
> temporary singleton objects now.

QOM design assumption: object_new() followed by object_unref() is always
possible and has no side effect.

QOM introspection relies on this.  Your PATCH 1 makes non-class
properties of singletons invisible in introspection.  Making something
with such properties a singleton would be a regression.

Anything that violates the design assumption must be delayed to a
suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
is ->realize().  For user-creatable objects (provide interface
TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
something else that probably doesn't even exist, yet.  See "Problem 5:
QOM lacks a clear life cycle" in

    Subject: Dynamic & heterogeneous machines, initial configuration: problems
    Date: Wed, 31 Jan 2024 21:14:21 +0100
    Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
    https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/

>                                   Meanwhile, we also guard device-add paths
> so that it'll fail properly if it's created more than once (and iff it's a
> TYPE_DEVICE first).

[...]

On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > This patchset introduces the singleton interface for QOM.
> >
> > The singleton interface is as simple as "this class can only create one
> > instance".
> >
> > We used to have similar demand when working on all kinds of vIOMMUs,
> > because in most cases that I am aware of, vIOMMU must be a singleton as
> > it's closely bound to the machine and also the root PCIe systems.  We used
> > to have a bunch of special code guarding those, for example, X86 has
> > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > than once.
> >
> > There is a similar demand raising recently (even if the problem existed
> > over years) when we were looking at a migration bug, that part of it was
> > involved with the current_migration global pointer being referenced
> > anywhere, even after the migration object was finalize()ed.  So far without
> > singleton support, there's no way to reset the variable properly.
> > Declaring migration object to be a singleton also just makes sense, e.g.,
> > dynamically creating migration objects on the fly with QMP commands doesn't
> > sound right..
> >
> > The idea behind is pretty simple: any object that can only be created once
> > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > sure it won't be created more than once.  For example, qom-list-properties,
> > device-list-properties, etc., will be smart enough to not try to create
> > temporary singleton objects now.
> 
> QOM design assumption: object_new() followed by object_unref() is always
> possible and has no side effect.

I see.

> 
> QOM introspection relies on this.  Your PATCH 1 makes non-class
> properties of singletons invisible in introspection.  Making something
> with such properties a singleton would be a regression.
> 
> Anything that violates the design assumption must be delayed to a
> suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> is ->realize().  For user-creatable objects (provide interface
> TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> something else that probably doesn't even exist, yet.  See "Problem 5:
> QOM lacks a clear life cycle" in
> 
>     Subject: Dynamic & heterogeneous machines, initial configuration: problems
>     Date: Wed, 31 Jan 2024 21:14:21 +0100
>     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
>     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/

The major challenge here might be that, in migration's use case we want to
do something after the last refcount is released.

IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
migration object will use realize(), while it doesn't yet..), because there
can be more than one thread holding refcount of the object, and we don't
know where to invoke it, and we don't want to do the cleanup if the other
one still holds a refcount.

The only sane place now is in instance_finalize(), which is the only clean
place so far that is invoked after last refcount dropped.

Maybe I can also try do that with a "magic property" with its set/get, as
that's indeed the other hook (basically, object_property_del_all()) that is
only invoked after the final refcount is released.  However I think we
still need the singleton idea somehow..

Thanks,

-- 
Peter Xu

On Fri, Oct 25, 2024 at 11:01:56AM -0400, Peter Xu wrote:
> On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > This patchset introduces the singleton interface for QOM.
> > >
> > > The singleton interface is as simple as "this class can only create one
> > > instance".
> > >
> > > We used to have similar demand when working on all kinds of vIOMMUs,
> > > because in most cases that I am aware of, vIOMMU must be a singleton as
> > > it's closely bound to the machine and also the root PCIe systems.  We used
> > > to have a bunch of special code guarding those, for example, X86 has
> > > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > > than once.
> > >
> > > There is a similar demand raising recently (even if the problem existed
> > > over years) when we were looking at a migration bug, that part of it was
> > > involved with the current_migration global pointer being referenced
> > > anywhere, even after the migration object was finalize()ed.  So far without
> > > singleton support, there's no way to reset the variable properly.
> > > Declaring migration object to be a singleton also just makes sense, e.g.,
> > > dynamically creating migration objects on the fly with QMP commands doesn't
> > > sound right..
> > >
> > > The idea behind is pretty simple: any object that can only be created once
> > > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > > sure it won't be created more than once.  For example, qom-list-properties,
> > > device-list-properties, etc., will be smart enough to not try to create
> > > temporary singleton objects now.
> > 
> > QOM design assumption: object_new() followed by object_unref() is always
> > possible and has no side effect.
> 
> I see.
> 
> > 
> > QOM introspection relies on this.  Your PATCH 1 makes non-class
> > properties of singletons invisible in introspection.  Making something
> > with such properties a singleton would be a regression.
> > 
> > Anything that violates the design assumption must be delayed to a
> > suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> > is ->realize().  For user-creatable objects (provide interface
> > TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> > something else that probably doesn't even exist, yet.  See "Problem 5:
> > QOM lacks a clear life cycle" in
> > 
> >     Subject: Dynamic & heterogeneous machines, initial configuration: problems
> >     Date: Wed, 31 Jan 2024 21:14:21 +0100
> >     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> >     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> 
> The major challenge here might be that, in migration's use case we want to
> do something after the last refcount is released.

This is rather a strange idea, and feels back to front. An object's last
refcount must never be released until code has entirely finished using
the object.

> IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
> migration object will use realize(), while it doesn't yet..), because there
> can be more than one thread holding refcount of the object, and we don't
> know where to invoke it, and we don't want to do the cleanup if the other
> one still holds a refcount.

This sounds like the code is missing some synchronization mechanism
beween the threads, which need to communicate with each other when
they are "done", so that the "primary" thread can then finally
release any resources.

> Maybe I can also try do that with a "magic property" with its set/get, as
> that's indeed the other hook (basically, object_property_del_all()) that is
> only invoked after the final refcount is released.  However I think we
> still need the singleton idea somehow..

Based on the description above it feels like the problem is outside
of QOM, around the lack of synchronization across threads.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tue, Oct 29, 2024 at 10:42:58AM +0000, Daniel P. Berrangé wrote:
> On Fri, Oct 25, 2024 at 11:01:56AM -0400, Peter Xu wrote:
> > On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> > > Peter Xu <peterx@redhat.com> writes:
> > > 
> > > > This patchset introduces the singleton interface for QOM.
> > > >
> > > > The singleton interface is as simple as "this class can only create one
> > > > instance".
> > > >
> > > > We used to have similar demand when working on all kinds of vIOMMUs,
> > > > because in most cases that I am aware of, vIOMMU must be a singleton as
> > > > it's closely bound to the machine and also the root PCIe systems.  We used
> > > > to have a bunch of special code guarding those, for example, X86 has
> > > > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > > > than once.
> > > >
> > > > There is a similar demand raising recently (even if the problem existed
> > > > over years) when we were looking at a migration bug, that part of it was
> > > > involved with the current_migration global pointer being referenced
> > > > anywhere, even after the migration object was finalize()ed.  So far without
> > > > singleton support, there's no way to reset the variable properly.
> > > > Declaring migration object to be a singleton also just makes sense, e.g.,
> > > > dynamically creating migration objects on the fly with QMP commands doesn't
> > > > sound right..
> > > >
> > > > The idea behind is pretty simple: any object that can only be created once
> > > > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > > > sure it won't be created more than once.  For example, qom-list-properties,
> > > > device-list-properties, etc., will be smart enough to not try to create
> > > > temporary singleton objects now.
> > > 
> > > QOM design assumption: object_new() followed by object_unref() is always
> > > possible and has no side effect.
> > 
> > I see.
> > 
> > > 
> > > QOM introspection relies on this.  Your PATCH 1 makes non-class
> > > properties of singletons invisible in introspection.  Making something
> > > with such properties a singleton would be a regression.
> > > 
> > > Anything that violates the design assumption must be delayed to a
> > > suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> > > is ->realize().  For user-creatable objects (provide interface
> > > TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> > > something else that probably doesn't even exist, yet.  See "Problem 5:
> > > QOM lacks a clear life cycle" in
> > > 
> > >     Subject: Dynamic & heterogeneous machines, initial configuration: problems
> > >     Date: Wed, 31 Jan 2024 21:14:21 +0100
> > >     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> > >     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> > 
> > The major challenge here might be that, in migration's use case we want to
> > do something after the last refcount is released.
> 
> This is rather a strange idea, and feels back to front. An object's last
> refcount must never be released until code has entirely finished using
> the object.
> 
> > IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
> > migration object will use realize(), while it doesn't yet..), because there
> > can be more than one thread holding refcount of the object, and we don't
> > know where to invoke it, and we don't want to do the cleanup if the other
> > one still holds a refcount.
> 
> This sounds like the code is missing some synchronization mechanism
> beween the threads, which need to communicate with each other when
> they are "done", so that the "primary" thread can then finally
> release any resources.
> 
> > Maybe I can also try do that with a "magic property" with its set/get, as
> > that's indeed the other hook (basically, object_property_del_all()) that is
> > only invoked after the final refcount is released.  However I think we
> > still need the singleton idea somehow..
> 
> Based on the description above it feels like the problem is outside
> of QOM, around the lack of synchronization across threads.

Right, this used to be discussed here and you were also in the loop:

https://lore.kernel.org/qemu-devel/20190228122822.GD4970@work-vm/

I kind of agree join() would be perfect, disregard the question on whether
QEMU would still benefit from a singleton interface, no matter migration
would rely on that for refcounting, or simply it'll also make sense to just
avoid people creating random migration objects.

So yes, I think migration can benefit from singleton idea for more than one
reason, while the refcount issue here might be even better resolved by
join() in main.

It's just that in the thread Dave raised a few points on possible
challenges on join() in the main thread.  I'm not sure whether we should go
that route instead.  Obviously I am digging one of our legacy rabbit holes
from when I started to look at this "access current_migration anywhere"
issue reported from VFIO.  But if some of us think we should give it a
shot, I can try.  After all, I started digging.

Another simpler but direct solution to "access current_migration" is, we
simply don't free it at all, leaving process exit() to do the job.  Then I
can drop all object_[un]ref() for the migration object.  I think that could
work too, but ugly, for the refcount issue.

Thanks,

-- 
Peter Xu

On Tue, Oct 29, 2024 at 10:45:25AM -0400, Peter Xu wrote:
> On Tue, Oct 29, 2024 at 10:42:58AM +0000, Daniel P. Berrangé wrote:
> > On Fri, Oct 25, 2024 at 11:01:56AM -0400, Peter Xu wrote:
> > > On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> > > > Peter Xu <peterx@redhat.com> writes:
> > > > 
> > > > > This patchset introduces the singleton interface for QOM.
> > > > >
> > > > > The singleton interface is as simple as "this class can only create one
> > > > > instance".
> > > > >
> > > > > We used to have similar demand when working on all kinds of vIOMMUs,
> > > > > because in most cases that I am aware of, vIOMMU must be a singleton as
> > > > > it's closely bound to the machine and also the root PCIe systems.  We used
> > > > > to have a bunch of special code guarding those, for example, X86 has
> > > > > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > > > > than once.
> > > > >
> > > > > There is a similar demand raising recently (even if the problem existed
> > > > > over years) when we were looking at a migration bug, that part of it was
> > > > > involved with the current_migration global pointer being referenced
> > > > > anywhere, even after the migration object was finalize()ed.  So far without
> > > > > singleton support, there's no way to reset the variable properly.
> > > > > Declaring migration object to be a singleton also just makes sense, e.g.,
> > > > > dynamically creating migration objects on the fly with QMP commands doesn't
> > > > > sound right..
> > > > >
> > > > > The idea behind is pretty simple: any object that can only be created once
> > > > > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > > > > sure it won't be created more than once.  For example, qom-list-properties,
> > > > > device-list-properties, etc., will be smart enough to not try to create
> > > > > temporary singleton objects now.
> > > > 
> > > > QOM design assumption: object_new() followed by object_unref() is always
> > > > possible and has no side effect.
> > > 
> > > I see.
> > > 
> > > > 
> > > > QOM introspection relies on this.  Your PATCH 1 makes non-class
> > > > properties of singletons invisible in introspection.  Making something
> > > > with such properties a singleton would be a regression.
> > > > 
> > > > Anything that violates the design assumption must be delayed to a
> > > > suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> > > > is ->realize().  For user-creatable objects (provide interface
> > > > TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> > > > something else that probably doesn't even exist, yet.  See "Problem 5:
> > > > QOM lacks a clear life cycle" in
> > > > 
> > > >     Subject: Dynamic & heterogeneous machines, initial configuration: problems
> > > >     Date: Wed, 31 Jan 2024 21:14:21 +0100
> > > >     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> > > >     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> > > 
> > > The major challenge here might be that, in migration's use case we want to
> > > do something after the last refcount is released.
> > 
> > This is rather a strange idea, and feels back to front. An object's last
> > refcount must never be released until code has entirely finished using
> > the object.
> > 
> > > IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
> > > migration object will use realize(), while it doesn't yet..), because there
> > > can be more than one thread holding refcount of the object, and we don't
> > > know where to invoke it, and we don't want to do the cleanup if the other
> > > one still holds a refcount.
> > 
> > This sounds like the code is missing some synchronization mechanism
> > beween the threads, which need to communicate with each other when
> > they are "done", so that the "primary" thread can then finally
> > release any resources.
> > 
> > > Maybe I can also try do that with a "magic property" with its set/get, as
> > > that's indeed the other hook (basically, object_property_del_all()) that is
> > > only invoked after the final refcount is released.  However I think we
> > > still need the singleton idea somehow..
> > 
> > Based on the description above it feels like the problem is outside
> > of QOM, around the lack of synchronization across threads.
> 
> Right, this used to be discussed here and you were also in the loop:
> 
> https://lore.kernel.org/qemu-devel/20190228122822.GD4970@work-vm/
> 
> I kind of agree join() would be perfect, disregard the question on whether
> QEMU would still benefit from a singleton interface, no matter migration
> would rely on that for refcounting, or simply it'll also make sense to just
> avoid people creating random migration objects.
> 
> So yes, I think migration can benefit from singleton idea for more than one
> reason, while the refcount issue here might be even better resolved by
> join() in main.
> 
> It's just that in the thread Dave raised a few points on possible
> challenges on join() in the main thread.  I'm not sure whether we should go
> that route instead.  Obviously I am digging one of our legacy rabbit holes
> from when I started to look at this "access current_migration anywhere"
> issue reported from VFIO.  But if some of us think we should give it a
> shot, I can try.  After all, I started digging.
> 
> Another simpler but direct solution to "access current_migration" is, we
> simply don't free it at all, leaving process exit() to do the job.  Then I
> can drop all object_[un]ref() for the migration object.  I think that could
> work too, but ugly, for the refcount issue.

I tend to feel that having MigrationState exist for the whole lifetime
of QEMU is a bug, forced on us by the unfortunate need to call
migrate-set-parameters/capabilities separately from the migrate
command, and by the need to query migrate info an arbitrary amount of
time after it finishes.

This puts libvirt in the awkward position of having to manually reset
all migration parameters, just to ensure earlier settings don't
accidentally affect a future migration operation :-( This is a design
that encourages mistakes.

Rather than MigrationState become a singleton, I'd really encourage
trying to see if we can fix the lifecycle design problems, so that
we have a clear beginning & end of the migration operation, with the
state discarded at the end, such that every future migrate starts
from a clean base.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tue, Oct 29, 2024 at 04:04:50PM +0000, Daniel P. Berrangé wrote:
> On Tue, Oct 29, 2024 at 10:45:25AM -0400, Peter Xu wrote:
> > On Tue, Oct 29, 2024 at 10:42:58AM +0000, Daniel P. Berrangé wrote:
> > > On Fri, Oct 25, 2024 at 11:01:56AM -0400, Peter Xu wrote:
> > > > On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> > > > > Peter Xu <peterx@redhat.com> writes:
> > > > > 
> > > > > > This patchset introduces the singleton interface for QOM.
> > > > > >
> > > > > > The singleton interface is as simple as "this class can only create one
> > > > > > instance".
> > > > > >
> > > > > > We used to have similar demand when working on all kinds of vIOMMUs,
> > > > > > because in most cases that I am aware of, vIOMMU must be a singleton as
> > > > > > it's closely bound to the machine and also the root PCIe systems.  We used
> > > > > > to have a bunch of special code guarding those, for example, X86 has
> > > > > > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > > > > > than once.
> > > > > >
> > > > > > There is a similar demand raising recently (even if the problem existed
> > > > > > over years) when we were looking at a migration bug, that part of it was
> > > > > > involved with the current_migration global pointer being referenced
> > > > > > anywhere, even after the migration object was finalize()ed.  So far without
> > > > > > singleton support, there's no way to reset the variable properly.
> > > > > > Declaring migration object to be a singleton also just makes sense, e.g.,
> > > > > > dynamically creating migration objects on the fly with QMP commands doesn't
> > > > > > sound right..
> > > > > >
> > > > > > The idea behind is pretty simple: any object that can only be created once
> > > > > > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > > > > > sure it won't be created more than once.  For example, qom-list-properties,
> > > > > > device-list-properties, etc., will be smart enough to not try to create
> > > > > > temporary singleton objects now.
> > > > > 
> > > > > QOM design assumption: object_new() followed by object_unref() is always
> > > > > possible and has no side effect.
> > > > 
> > > > I see.
> > > > 
> > > > > 
> > > > > QOM introspection relies on this.  Your PATCH 1 makes non-class
> > > > > properties of singletons invisible in introspection.  Making something
> > > > > with such properties a singleton would be a regression.
> > > > > 
> > > > > Anything that violates the design assumption must be delayed to a
> > > > > suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> > > > > is ->realize().  For user-creatable objects (provide interface
> > > > > TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> > > > > something else that probably doesn't even exist, yet.  See "Problem 5:
> > > > > QOM lacks a clear life cycle" in
> > > > > 
> > > > >     Subject: Dynamic & heterogeneous machines, initial configuration: problems
> > > > >     Date: Wed, 31 Jan 2024 21:14:21 +0100
> > > > >     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> > > > >     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> > > > 
> > > > The major challenge here might be that, in migration's use case we want to
> > > > do something after the last refcount is released.
> > > 
> > > This is rather a strange idea, and feels back to front. An object's last
> > > refcount must never be released until code has entirely finished using
> > > the object.
> > > 
> > > > IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
> > > > migration object will use realize(), while it doesn't yet..), because there
> > > > can be more than one thread holding refcount of the object, and we don't
> > > > know where to invoke it, and we don't want to do the cleanup if the other
> > > > one still holds a refcount.
> > > 
> > > This sounds like the code is missing some synchronization mechanism
> > > beween the threads, which need to communicate with each other when
> > > they are "done", so that the "primary" thread can then finally
> > > release any resources.
> > > 
> > > > Maybe I can also try do that with a "magic property" with its set/get, as
> > > > that's indeed the other hook (basically, object_property_del_all()) that is
> > > > only invoked after the final refcount is released.  However I think we
> > > > still need the singleton idea somehow..
> > > 
> > > Based on the description above it feels like the problem is outside
> > > of QOM, around the lack of synchronization across threads.
> > 
> > Right, this used to be discussed here and you were also in the loop:
> > 
> > https://lore.kernel.org/qemu-devel/20190228122822.GD4970@work-vm/
> > 
> > I kind of agree join() would be perfect, disregard the question on whether
> > QEMU would still benefit from a singleton interface, no matter migration
> > would rely on that for refcounting, or simply it'll also make sense to just
> > avoid people creating random migration objects.
> > 
> > So yes, I think migration can benefit from singleton idea for more than one
> > reason, while the refcount issue here might be even better resolved by
> > join() in main.
> > 
> > It's just that in the thread Dave raised a few points on possible
> > challenges on join() in the main thread.  I'm not sure whether we should go
> > that route instead.  Obviously I am digging one of our legacy rabbit holes
> > from when I started to look at this "access current_migration anywhere"
> > issue reported from VFIO.  But if some of us think we should give it a
> > shot, I can try.  After all, I started digging.
> > 
> > Another simpler but direct solution to "access current_migration" is, we
> > simply don't free it at all, leaving process exit() to do the job.  Then I
> > can drop all object_[un]ref() for the migration object.  I think that could
> > work too, but ugly, for the refcount issue.
> 
> I tend to feel that having MigrationState exist for the whole lifetime
> of QEMU is a bug, forced on us by the unfortunate need to call
> migrate-set-parameters/capabilities separately from the migrate
> command, and by the need to query migrate info an arbitrary amount of
> time after it finishes.
> 
> This puts libvirt in the awkward position of having to manually reset
> all migration parameters, just to ensure earlier settings don't
> accidentally affect a future migration operation :-( This is a design
> that encourages mistakes.

I think it would still be easy to add "cap" & "params" arguments support
for the "migrate" QMP command without breaking the current API, iff that
helps in whatever form.  When present, it simply applies the caps and/or
param list first before invoking the migrate command, fail the command if
cap / param check fails.

But I'm not sure whether that's a concern at all for Libvirt, if what
Libvirt currently does is having separate "migrate-set-*" commands prior to
the "migrate".  I may have overlooked the real issue behind on how that
could complicate Libvirt.

> 
> Rather than MigrationState become a singleton, I'd really encourage
> trying to see if we can fix the lifecycle design problems, so that
> we have a clear beginning & end of the migration operation, with the
> state discarded at the end, such that every future migrate starts
> from a clean base.

I assume it implies the join() path as a start.  As long as we're ok we
risk slow quits of QEMU, as I would expect bug reports coming afterwards,
we could try.  I believe there's no way we can resolve all such issues in
one shot.  I doubt whether some of those can be too tricky so we may wish
to go back at some point, spending too much effort but without a direct
benefit yet so far.

Meanwhile we still have the immediate concern that current_migration can
points to freed memory right now with QEMU master branch.  That's simply
wrong.

I thought it was still an improvement to have the singleton idea, so that
even afterwards we can join() properly the idea may still make sense on its
own.  It is also not against the ultimate willingness that we want to
create an obj at start of migration, and clearly destroys it after
migration completes.  When that comes we keep migration object singleton,
but then the tricky operations on current_migration can be gone.

That still guarantees from QOM level that e.g. nobody tries to initiate
migration twice, for example, or accidentally create it somewhere, like in
qom-list-properties.

There's yet another short term plan that I can fix UAF for
current_migration, which is to have a refcount for migration alone on top
of Object, then it only object_unref() on the migration object after we
know the last real refcount is released.  That won't need any QOM change,
so keep the complexity internally.  But I prefer the current proposal,
assuming it could be still helpful in general for QEMU, even though I'm not
sure.

So.. I'm not sure whether we should ignore the current UAF until a whole
refactor of any kind to land, or we should do something to fix it, which is
what I'm trying with the series.

Thanks,

-- 
Peter Xu

On Tue, Oct 29, 2024 at 01:05:04PM -0400, Peter Xu wrote:
> But I'm not sure whether that's a concern at all for Libvirt, if what
> Libvirt currently does is having separate "migrate-set-*" commands prior to
> the "migrate".  I may have overlooked the real issue behind on how that
> could complicate Libvirt.
> 
> > 
> > Rather than MigrationState become a singleton, I'd really encourage
> > trying to see if we can fix the lifecycle design problems, so that
> > we have a clear beginning & end of the migration operation, with the
> > state discarded at the end, such that every future migrate starts
> > from a clean base.
> 
> I assume it implies the join() path as a start.  As long as we're ok we
> risk slow quits of QEMU, as I would expect bug reports coming afterwards,
> we could try.  I believe there's no way we can resolve all such issues in
> one shot.  I doubt whether some of those can be too tricky so we may wish
> to go back at some point, spending too much effort but without a direct
> benefit yet so far.
> 
> Meanwhile we still have the immediate concern that current_migration can
> points to freed memory right now with QEMU master branch.  That's simply
> wrong.

Yeah, if we're accessing freed memory, that's a segv/abrt danger, and
needs fixing quickly.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|