[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug |
Date: |
Tue, 28 Nov 2017 12:28:43 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
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