Re: [Qemu-devel] [PATCH] cpus.c: Fix race condition in cpu_stop_current()

[Peter Maydell]

On Mon, 10 Dec 2018 at 11:06, Alex Bennée <address@hidden> wrote:
>
>
> Peter Maydell <address@hidden> writes:
> > We discussed this a little while back:
> > https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00154.html
> > and Jaap reported a bug which I suspect of being the same thing:
> > https://lists.gnu.org/archive/html/qemu-discuss/2018-10/msg00014.html
> >
> > Annoyingly I have lost the test case that demonstrated this
> > race, but I analysed it at the time and this should definitely
> > fix it. I have opted not to try to address any of the other
> > possible cleanup here (eg vm_stop() has a potential similar
> > race if called from a vcpu thread I suspect), since it gets
> > pretty tangled.
> >
> > Jaap: could you test whether this patch fixes the issue you
> > were seeing, please?
> > ---
> >  cpus.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/cpus.c b/cpus.c
> > index 0ddeeefc14f..b09b7027126 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -2100,7 +2100,8 @@ void qemu_init_vcpu(CPUState *cpu)
> >  void cpu_stop_current(void)
> >  {
> >      if (current_cpu) {
> > -        qemu_cpu_stop(current_cpu, true);
> > +        current_cpu->stop = true;
> > +        cpu_exit(current_cpu);
>
> Should the FIXME in vm_stop also be fixed?
>
>         /*
>          * FIXME: should not return to device code in case
>          * vm_stop() has been requested.
>          */
>         cpu_stop_current();
>         return 0;

This is one of the things I had in mind with:
> > I have opted not to try to address any of the other
> > possible cleanup here (eg vm_stop() has a potential similar
> > race if called from a vcpu thread I suspect), since it gets
> > pretty tangled.

though I might actually have meant pause_all_vcpus().
(For pause_all_vcpus() I think the correct thing is to
fix the hw/i386/kvmvapic.c code to work in some other way,
and then assert that pause_all_vcpus() is never called from
the VCPU thread.)

At any rate, this code is quite complicated, so I think
it's worth just fixing this specific race without getting
tangled up in everything else we could potentially refactor.

I'm not even sure how we would arrange for vm_stop() to
avoid returning to device emulation code if it has been
called from there -- I would favour instead changing/defining
the semantics to be like the shutdown-request and reset-request
where the device code expects that control will return but
the VM stop happens at the next opportunity, ie as soon
as the execution of the insn which wrote to the device
register has completed.

thanks
-- PMM