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

[Paolo Bonzini]

On 20/11/2017 18:05, Eduardo Habkost wrote:
> 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.

I made the change and queued the patch.

Paolo