On Tue, Nov 16, 2021 at 12:20:37PM +0100, Laszlo Ersek wrote: > On 11/16/21 04:22, Peter Xu wrote: > > When finalizing the dump-guest-memory with detached mode, we'll first set > > dump > > status to either FAIL or COMPLETE before doing the cleanup, however right > > after > > the dump status change it's possible that another dump-guest-memory qmp > > command > > is sent so both the main thread and dump thread (which is during cleanup) > > could > > be accessing dump state in paralell. That's racy. > > > > Fix it by protecting the finalizing phase of dump-guest-memory using BQL as > > well. To do that, we expand the BQL from dump_cleanup() into > > dump_process(), > > so we will take the BQL longer than before. The critical section must cover > > the status switch from ACTIVE->{FAIL|COMPLETE} so as to make sure there's no > > race any more. > > > > We can also just introduce a specific mutex to serialize the dump process, > > but > > BQL should be enough for now so far, not to mention vm_start() in > > dump_cleanup > > will need BQL anyway, so maybe easier to just use the same mutex. > > > > Reported-by: Laszlo Ersek <ler...@redhat.com> > > Signed-off-by: Peter Xu <pet...@redhat.com> > > --- > > dump/dump.c | 24 ++++++++++++++++++------ > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/dump/dump.c b/dump/dump.c > > index 662d0a62cd..196b7b8ab9 100644 > > --- a/dump/dump.c > > +++ b/dump/dump.c > > @@ -96,13 +96,7 @@ static int dump_cleanup(DumpState *s) > > g_free(s->guest_note); > > s->guest_note = NULL; > > if (s->resume) { > > - if (s->detached) { > > - qemu_mutex_lock_iothread(); > > - } > > vm_start(); > > - if (s->detached) { > > - qemu_mutex_unlock_iothread(); > > - } > > } > > migrate_del_blocker(dump_migration_blocker); > > > > @@ -1873,6 +1867,11 @@ static void dump_process(DumpState *s, Error **errp) > > Error *local_err = NULL; > > DumpQueryResult *result = NULL; > > > > + /* > > + * When running with detached mode, these operations are not run with > > BQL. > > + * It's still safe, because it's protected by setting s->state to > > ACTIVE, > > I think this is a typo: it should be s->status. > > > + * so dump_in_progress() check will block yet another > > dump-guest-memory. > > + */ > > if (s->has_format && s->format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP) { > > #ifdef TARGET_X86_64 > > create_win_dump(s, &local_err); > > @@ -1883,6 +1882,15 @@ static void dump_process(DumpState *s, Error **errp) > > create_vmcore(s, &local_err); > > } > > > > + /* > > + * Serialize the finalizing of dump process using BQL to make sure no > > + * concurrent access to DumpState is allowed. BQL is also required for > > + * dump_cleanup as vm_start() needs it. > > + */ > > + if (s->detached) { > > + qemu_mutex_lock_iothread(); > > + } > > + > > /* make sure status is written after written_size updates */ > > smp_wmb(); > > qatomic_set(&s->status, > > @@ -1898,6 +1906,10 @@ static void dump_process(DumpState *s, Error **errp) > > > > error_propagate(errp, local_err); > > dump_cleanup(s); > > + > > + if (s->detached) { > > + qemu_mutex_unlock_iothread(); > > + } > > } > > > > static void *dump_thread(void *data) > > > > The detached dumping thread now runs dump_cleanup() with the BQL held, so: > > dump_thread() > dump_process() > [ dump status is now FAILED or COMPLETED ] > dump_cleanup() > vm_start() > [ runstate is now "running" I guess? ] > migrate_del_blocker() > g_slist_remove(migration_blockers) <------ read-modify-write #1 > > is now called under the BQL's protection. > > Assuming a new dump request is issued in parallel over QMP, we have (on > another thread -- the main thread I guess?): > > qmp_dump_guest_memory() > [ dumping *not* in progress ] > migrate_add_blocker_internal() > [ runstate is *not* RUN_STATE_SAVE_VM ] > g_slist_prepend(migration_blockers) <------- RMW #2 > > and this is not protected by any *explicit* acquiral of the BQL. > > I know very little of the BQL, unfortunately. *IF* your argument is that > qmp_dump_guest_memory() is entered with the BQL *already held*, then the > patch is fine, IMO. Because in that case, during the short while that > the detached dumping thread is cleaning up, the main thread (?) cannot > acquire the BQL, and so it cannot enter qmp_dump_guest_memory() at all. > If that's your point,
Yes that's the point. qmp_dump_guest_memory() must be with BQL held. For example, dump_init() will call vm_start() if VM is running: if (runstate_is_running()) { vm_stop(RUN_STATE_SAVE_VM); s->resume = true; } else { s->resume = false; } And vm_stop() needs BQL. The same to vm_start() if detached mode is not enabled. If we call qmp_dump_guest_memory() without BQL that'll be a very severe issue, imho. > then: > > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > > *Otherwise*, I don't understand how the patch helps protecting the > "migration_blockers" object. (Because, although RMW#1 is now protected, > RMW#2 is not.) I took it for granted that BQL needs to be taken for qmp_dump_guest_memory() without explicitly explaining. I'll refine the commit message and mention that in v2. Thanks, -- Peter Xu