Re: [PATCH RFC v2 0/7] QOM: Singleton interface

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Oct 30, 2024 at 01:51:54PM -0400, Peter Xu wrote:
> On Wed, Oct 30, 2024 at 04:13:57PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Oct 30, 2024 at 09:13:13AM -0400, Peter Xu wrote:
> > > 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/20241024165627.1372621-1-peterx@redhat.com
> > > > 
> > > > > 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/20241024213056.1395400-2-peterx@redhat.com/
> > 
> > Yep, acquiring the refcount immediately before thread-create
> > is what I meant.
> > 
> > > 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/20241024213056.1395400-9-peterx@redhat.com/
> > > 
> > > > 
> > > > 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..
> > 
> > Ok, so with the other series applied, this does look safe, but
> > it also doesn't seem to really have any dependancy on the
> > single interface code.  Patch 7 here looks sufficient, in combo
> > with the other 2 series to avoid the use-after-free flaws.
> 
> Patch 7, when applied without patch 6 and prior, will crash in
> device-introspect-test, trying to create yet another migration object when
> processing the "device-list-properties" QMP command.  And it turns out
> that's also not the only way QEMU can crash by that.
> 
> Fundamentally it's because patch 7 has global operations within
> init()/finalize() to fix the migration dangling pointer, hence it must not
> be instanciated more than once.

That's a result from moving the "assert()" into the constructor.
The assert(!current_migration) can be kept in migration_object_init,
the constructor could conditionally set current_migration only if it
is NULL, and the finalizer could conditionally clear current_migration
only if it matches the current object. There's no conceptual dependancy
on having a singleton interface in the patch.

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 :|