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

[Peter Xu <peterx@redhat.com>]

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/

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

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

-- 
Peter Xu