Re: [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 20 Nov 2017 12:44:54 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Nov 20, 2017 at 03:34:13PM +0100, Igor Mammedov wrote:
> > when qemu is started with '-no-acpi' CLI option, an attempt
> > to unplug a CPU using device_del results in null pointer
> > dereference at:
> > 
> >   #0 object_get_class
> >   #1 pc_machine_device_unplug_request_cb
> >   #2 qmp_marshal_device_del
> > 
> > which is caused by pcms->acpi_dev == NULL due to ACPI support
> > being disabled.
> > 
> > Considering that ACPI support is necessary for unplug to work,
> > check that it's enabled and fail unplug request gracefully
> > if no acpi device were found.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> But I have one small suggestion:
> 
> > ---
> >  hw/i386/pc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index c3afe5b..d80cec3 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1842,6 +1842,11 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
> >      X86CPU *cpu = X86_CPU(dev);
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >  
> > +    if (!pcms->acpi_dev) {
> > +        error_setg(&local_err, "not supported without acpi");  
> 
> I suggest "CPU hot unplug not supported without ACPI" instead.
I've thought about it but considering error is issued in context of
device_del command, I've opted for more concise variant.

Would you still like me to respin patch with your suggestion?

> 
> > +        goto out;
> > +    }
> > +
> >      pc_find_cpu_slot(MACHINE(pcms), cpu->apic_id, &idx);
> >      assert(idx != -1);
> >      if (idx == 0) {
> > -- 
> > 2.7.4
> >   
>