[RFC,v2,0/7] QOM: Singleton interface

Message

Peter Xu Oct. 29, 2024, 9:16 p.m. UTC

v1: https://lore.kernel.org/r/[e-mail]

This patchset introduces the singleton interface for QOM.  I didn't add a
changelog because there're quite a few changes here and there, plus new
patches.  So it might just be easier to re-read, considering the patchset
isn't large.

I switched v2 into RFC, because we have reviewer concerns (Phil and Dan so
far) that it could be error prone to try to trap every attempts to create
an object.  My argument is, if we already have abstract class, meanwhile we
do not allow instantiation of abstract class, so the complexity is already
there.  I prepared patch 1 this time to collect and track all similar
random object creations; it might be helpful as a cleanup on its own to
deduplicate some similar error messages.  Said that, I'm still always open
to rejections to this proposal.

I hope v2 looks slightly cleaner by having not only object_new_allowed()
but also object_new_or_fetch().

Patch layout:

Patch 1-2:        The patches to introduce QOM singleton interface
Patch 3-5:        Add support for vIOMMU singleton, some qdev change needed
Patch 6-7:        Add support for migration singleton, fix dangle pointer

Below are copy-paste of the commit message of patch 2, that I could have
put into the cover letter too.

====8<====

The singleton interface declares an object class which can only create one
instance globally.

Backgrounds / Use Cases
=======================

There can be some existing classes that can start to benefit from it.  One
example is vIOMMU implementations.

QEMU emulated vIOMMUs are normally implemented on top of a generic device,
however it's special enough to normally only allow one instance of it for
the whole system, attached to the root complex.

These vIOMMU classes can be singletons in this case, so that QEMU can fail
or detect yet another attempt of creating such devices for more than once,
which can be fatal errors to a system.

We used to have some special code guarding it from happening.  In x86,
pc_machine_device_pre_plug_cb() has code to detect when vIOMMU is created
more than once, for instance.  With singleton class, logically we could
consider dropping the special code, but start to rely on QOM to make sure
there's only one vIOMMU for the whole system emulation.

There is a similar demand raising recently (even if the problem existed
over years) in migration.

Firstly, the migration object can currently be created randomly, even
though not wanted, e.g. during qom-list-properties QMP commands.  Ideally,
there can be cases where we want to have an object walking over the
properties, we could use the existing migration object instead of
dynamically create one.

Meanwhile, migration has a long standing issue on current_migration
pointer, where it can point to freed data after the migration object is
finalized.  It is debatable that the pointer can be cleared after the main
thread (1) join() the migration thread first, then (2) release the last
refcount for the migration object and clear the pointer.  However there's
still major challenges [1].  With singleton, we could have a slightly but
hopefully working workaround to clear the pointer during finalize().

Design
======

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 the whole QEMU lifecycle.
Whenever possible (e.g., on object_new_allowed() checks), pretty error
message will be generated to report an error.  QOM also guards at the core
of object_new() so that any further violation of trying to create a
singleton more than once will crash QEMU as a programming error.

For example, qom-list-properties, device-list-properties, etc., will be
smart enough to not try to create temporary singleton objects if the class
is a singleton class and if there's existing instance created.  Such usages
should be rare, and this patch introduced object_new_or_fetch() just for
it, which either create a new temp object when available, or fetch the
instance if we found an existing singleton instance.  There're only two
such use cases.

Meanwhile, we also guard device-add or similar paths using the singleton
check in object_new_allowed(), so that it'll fail properly if a singleton
class instantiate more than one object.

Glib Singleton implementation
=============================

One note here to mention the Glib implementation of singletons [1].

QEMU chose not to follow Glib's implementation because Glib's version is
not thread safe on the constructor, so that two concurrent g_object_new()
on a single can race.  It's not ideal to QEMU, as QEMU has to not only
support the event-driven context which is normally lock-free, but also
the case where threads are heavily used.

It could be QEMU's desire to properly support multi-threading by default on
such new interface.  The "bad" side effect of that is, QEMU's object_new()
on singletons can assert failures if the singleton existed, but that's also
part of the design so as to forbid such from happening, taking which as a
programming error.  Meanwhile since we have pretty ways to fail elsewhere
on qdev creations, it should already guard us in a clean way, from anywhere
that the user could try to create the singleton more than once.

The current QEMU impl also guarantees object_new() always return a newly
allocated object as long as properly returned, rather than silently return
an existing object as what Glib's impl would do.  I see it a benefit, so as
to avoid unknown caller manipulate a global object, wrongly assuming it was
temporarily created.

[1] https://lore.kernel.org/qemu-devel/20190228122822.GD4970@work-vm/
[2] https://lore.kernel.org/r/[e-mail]

Thanks,

Peter Xu (7):
  qom: Track dynamic initiations of random object class
  qom: TYPE_SINGLETON interface
  qdev: Make device_set_realized() be fully prepared with !machine
  qdev: Make qdev_get_machine() safe before machine creates
  x86/iommu: Make x86-iommu a singleton object
  migration: Make migration object a singleton object
  migration: Reset current_migration properly

 qapi/qdev.json                  |  2 +-
 qapi/qom.json                   |  2 +-
 include/qom/object.h            | 25 ++++++++++++++++
 include/qom/object_interfaces.h | 51 +++++++++++++++++++++++++++++++++
 chardev/char.c                  |  4 +--
 hw/core/cpu-common.c            | 13 ++++++---
 hw/core/qdev.c                  | 20 +++++++++++--
 hw/i386/x86-iommu.c             | 26 +++++++++++++++--
 migration/migration.c           | 36 +++++++++++++++++++++--
 qom/object.c                    | 50 ++++++++++++++++++++++++++++++--
 qom/object_interfaces.c         | 33 +++++++++++++++++++--
 qom/qom-qmp-cmds.c              |  4 +--
 system/qdev-monitor.c           |  4 +--
 13 files changed, 243 insertions(+), 27 deletions(-)

Comments

Daniel P. Berrangé Oct. 30, 2024, 9:48 a.m. UTC | #1

On Tue, Oct 29, 2024 at 05:16:00PM -0400, Peter Xu wrote:
> v1: https://lore.kernel.org/r/[e-mail]

> Meanwhile, migration has a long standing issue on current_migration
> pointer, where it can point to freed data after the migration object is
> finalized.  It is debatable that the pointer can be cleared after the main
> thread (1) join() the migration thread first, then (2) release the last
> refcount for the migration object and clear the pointer.  However there's
> still major challenges [1].  With singleton, we could have a slightly but
> hopefully working workaround to clear the pointer during finalize().

I'm still not entirely convinced that this singleton proposal is
fixing the migration problem correctly.

Based on discussions in v1, IIUC, the situation is that we have
migration_shutdown() being called from qemu_cleanup(). The former
will call object_unref(current_migration), but there may still
be background migration threads running that access 'current_migration',
and thus a potential use-after-free.

Based on what the 7th patch here does, the key difference is that
the finalize() method for MigrationState will set 'current_migration'
to NULL after free'ing it.

I don't believe that is safe.

Back to the current code, if there is a use-after-free today, that
implies that the background threads are *not* holding their own
reference on 'current_migration', allowing the object to be free'd
while they're still using it. If they held their own reference then
the object_unref in migration_shutdown would not have any use after
free risk.

The new code is not changing the ref counting done by any threads.
Therefore if there's a use-after-free in existing code, AFAICT, the
same use-after-free *must* still exist in the current code.

The 7th patch only fixes the use-after-free, *if and only if* the
background thread tries to access 'current_migration', /after/
finalize as completed. The use-after-free in this case, has been
turned into a NULL pointer reference.

A background thread could be accessing the 'current_migration' pointer
*concurrently* with the finalize method executing though. In this case
we still have a use after free problem, only the time window in which
it exists has been narrowed a little.

Shouldn't the problem with migration be solved by every migration thread
holding a reference on current_migration, that the thread releases when
it exits, such that MigrationState is only finalized once every thread
has exited ? That would not require any join() synchronization point.

With regards,
Daniel

Peter Xu Oct. 30, 2024, 1:13 p.m. UTC | #2

On Wed, Oct 30, 2024 at 09:48:07AM +0000, Daniel P. Berrangé wrote:
> On Tue, Oct 29, 2024 at 05:16:00PM -0400, Peter Xu wrote:
> > v1: https://lore.kernel.org/r/[e-mail]
> 
> > Meanwhile, migration has a long standing issue on current_migration
> > pointer, where it can point to freed data after the migration object is
> > finalized.  It is debatable that the pointer can be cleared after the main
> > thread (1) join() the migration thread first, then (2) release the last
> > refcount for the migration object and clear the pointer.  However there's
> > still major challenges [1].  With singleton, we could have a slightly but
> > hopefully working workaround to clear the pointer during finalize().
> 
> I'm still not entirely convinced that this singleton proposal is
> fixing the migration problem correctly.
> 
> Based on discussions in v1, IIUC, the situation is that we have
> migration_shutdown() being called from qemu_cleanup(). The former
> will call object_unref(current_migration), but there may still
> be background migration threads running that access 'current_migration',
> and thus a potential use-after-free.

migration thread is fine, it takes a refcount at the entry.

And btw, taking it at the entry is racy, we've just fixed it, see (in my
next migration pull):

https://lore.kernel.org/qemu-devel/[e-mail]/

The access reported was, IIUC, outside migration code, but after both
main/migration threads released the refcount, hence after finalize().  It
could be a random migration_is_running() call very late in device code, for
example.

> 
> Based on what the 7th patch here does, the key difference is that
> the finalize() method for MigrationState will set 'current_migration'
> to NULL after free'ing it.

Yes.  But this show case series isn't complete.  We need a migration-side
lock finally to make it safe to access.  For that, see:

https://lore.kernel.org/qemu-devel/[e-mail]/

> 
> I don't believe that is safe.

I hope after the other series applied it will be 100% safe, even though I
agree it's tricky.  But hopefully QOM is very clean, the trickly part is
still within migration, and it should be less tricky than migration
implement a refcount on top of Object..

> 
> Back to the current code, if there is a use-after-free today, that
> implies that the background threads are *not* holding their own
> reference on 'current_migration', allowing the object to be free'd
> while they're still using it. If they held their own reference then
> the object_unref in migration_shutdown would not have any use after
> free risk.
> 
> The new code is not changing the ref counting done by any threads.
> Therefore if there's a use-after-free in existing code, AFAICT, the
> same use-after-free *must* still exist in the current code.
> 
> The 7th patch only fixes the use-after-free, *if and only if* the
> background thread tries to access 'current_migration', /after/
> finalize as completed. The use-after-free in this case, has been
> turned into a NULL pointer reference.
> 
> A background thread could be accessing the 'current_migration' pointer
> *concurrently* with the finalize method executing though. In this case
> we still have a use after free problem, only the time window in which
> it exists has been narrowed a little.
> 
> Shouldn't the problem with migration be solved by every migration thread
> holding a reference on current_migration, that the thread releases when
> it exits, such that MigrationState is only finalized once every thread
> has exited ? That would not require any join() synchronization point.

I think the question is whether things like migration_is_running() is
allowed to be used anywhere, even after migration_shutdown().  My answer
is, it should be ok to be used anywhere, and we don't necessarilly need to
limit that.  In that case the caller doesn't need to take a refcount
because it's an immediate query.  It can simply check its existance with
the lock (after my patch 8 of the other series applied, which depends on
this qom series).

Thanks,