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

[Eduardo Habkost]

On Mon, Nov 20, 2017 at 03:59:51PM +0100, Igor Mammedov wrote:
> On Mon, 20 Nov 2017 12:44:54 -0200
> Eduardo Habkost <address@hidden> 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 <address@hidden>  
> > 
> > Reviewed-by: Eduardo Habkost <address@hidden>
> > 
> > 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?

I'd prefer to, so the error message would make sense even if out
of context.  But you still have my Reviewed-by in case Michael
wants to apply this version.


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

-- 
Eduardo