| Message ID | 1511188453-248734-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-2.11] pc: fix crash on attempted cpu unplug | expand |
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. > + goto out; > + } > + > pc_find_cpu_slot(MACHINE(pcms), cpu->apic_id, &idx); > assert(idx != -1); > if (idx == 0) { > -- > 2.7.4 >
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 > > >
On Mon, Nov 20, 2017 at 03:59:51PM +0100, Igor Mammedov wrote: > 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? 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 > > > > > >
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 <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? > > 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
On Tue, 28 Nov 2017 12:28:43 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > 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 <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? > > > > 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. I've thought Michael (CCed) queued v2 but he hasn't sent a pull req yet. > Paolo
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"); + goto out; + } + pc_find_cpu_slot(MACHINE(pcms), cpu->apic_id, &idx); assert(idx != -1); if (idx == 0) {
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> --- hw/i386/pc.c | 5 +++++ 1 file changed, 5 insertions(+)