Re: [PATCH v2] dump-guest-memory: Use BQL to protect dump finalize process

[Peter Xu]

On Wed, Nov 17, 2021 at 11:43:42AM +0100, Laszlo Ersek wrote:
> On 11/16/21 14:54, 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 parallel.  That's racy.
> > 
> > Fix it by protecting the finalizing phase of dump-guest-memory using BQL as
> > well, as qmp_dump_guest_memory() (which is the QEMU main thread) requires 
> > BQL.
> > 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 <lersek@redhat.com>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > v2:
> > - Fix parallel spelling [Marc-Andre]
> > - Add r-b for Marc-Andre
> > - Mention that qmp_dump_guest_memory() is with BQL held [Laszlo]
> > ---
> >  dump/dump.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks (again) for the review (and spot/analyze the issue)!

-- 
Peter Xu