* [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug
@ 2017-11-20 14:34 Igor Mammedov
2017-11-20 14:44 ` Eduardo Habkost
0 siblings, 1 reply; 6+ messages in thread
From: Igor Mammedov @ 2017-11-20 14:34 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, pbonzini, ehabkost, rth
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(+)
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) {
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug
2017-11-20 14:34 [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug Igor Mammedov
@ 2017-11-20 14:44 ` Eduardo Habkost
2017-11-20 14:59 ` Igor Mammedov
0 siblings, 1 reply; 6+ messages in thread
From: Eduardo Habkost @ 2017-11-20 14:44 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst, pbonzini, rth
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
>
--
Eduardo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug
2017-11-20 14:44 ` Eduardo Habkost
@ 2017-11-20 14:59 ` Igor Mammedov
2017-11-20 17:05 ` Eduardo Habkost
0 siblings, 1 reply; 6+ messages in thread
From: Igor Mammedov @ 2017-11-20 14:59 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, mst, pbonzini, rth
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
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug
2017-11-20 14:59 ` Igor Mammedov
@ 2017-11-20 17:05 ` Eduardo Habkost
2017-11-28 11:28 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Eduardo Habkost @ 2017-11-20 17:05 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst, pbonzini, rth
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
> > >
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug
2017-11-20 17:05 ` Eduardo Habkost
@ 2017-11-28 11:28 ` Paolo Bonzini
2017-11-28 13:23 ` Igor Mammedov
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2017-11-28 11:28 UTC (permalink / raw)
To: Eduardo Habkost, Igor Mammedov; +Cc: qemu-devel, mst, rth
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug
2017-11-28 11:28 ` Paolo Bonzini
@ 2017-11-28 13:23 ` Igor Mammedov
0 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2017-11-28 13:23 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Eduardo Habkost, qemu-devel, mst, rth
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-28 13:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-20 14:34 [Qemu-devel] [PATCH for-2.11] pc: fix crash on attempted cpu unplug Igor Mammedov
2017-11-20 14:44 ` Eduardo Habkost
2017-11-20 14:59 ` Igor Mammedov
2017-11-20 17:05 ` Eduardo Habkost
2017-11-28 11:28 ` Paolo Bonzini
2017-11-28 13:23 ` Igor Mammedov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).