hw/i386/pc: Fix crash that occurs when introspecting TYPE_PC_MACHINE machines

[PATCH] hw/i386/pc: Fix crash that occurs when introspecting TYPE_PC_MACHINE machines

Posted by Thomas Huth 1 year ago

QEMU currently crashes when you try to inspect the machines based on
TYPE_PC_MACHINE for their properties:

 $ echo '{ "execute": "qmp_capabilities" }
         { "execute": "qom-list-properties","arguments":
                      { "typename": "pc-q35-10.0-machine"}}' \
   | ./qemu-system-x86_64 -M pc -qmp stdio
 {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
  "package": "v9.2.0-1070-g87e115c122-dirty"}, "capabilities": ["oob"]}}
 {"return": {}}
 Segmentation fault (core dumped)

This happens because TYPE_PC_MACHINE machines add a machine_init-
done_notifier in their instance_init function - but instance_init
of machines are not only called for machines that are realized,
but also for machines that are introspected, so in this case the
listener is added for a q35 machine that is never realized. But
since there is already a running pc machine, the listener function
is triggered immediately, causing a crash since it was not for the
right machine it was meant for.

Such listener functions must never be installed from an instance_init
function. Let's do it from pc_basic_device_init() instead - this
function is called from the MachineClass->init() function instead,
i.e. guaranteed to be only called once in the lifetime of a QEMU
process.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2779
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/pc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b46975c8a4..85b8a76455 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1241,6 +1241,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
     /* Super I/O */
     pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
                     pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
+
+    pcms->machine_done.notify = pc_machine_done;
+    qemu_add_machine_init_done_notifier(&pcms->machine_done);
 }
 
 void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
@@ -1714,9 +1717,6 @@ static void pc_machine_initfn(Object *obj)
     if (pcmc->pci_enabled) {
         cxl_machine_init(obj, &pcms->cxl_devices_state);
     }
-
-    pcms->machine_done.notify = pc_machine_done;
-    qemu_add_machine_init_done_notifier(&pcms->machine_done);
 }
 
 static void pc_machine_reset(MachineState *machine, ResetType type)
-- 
2.48.1

Re: [PATCH] hw/i386/pc: Fix crash that occurs when introspecting TYPE_PC_MACHINE machines

Posted by Philippe Mathieu-Daudé 1 year ago

Hi Thomas,

On 17/1/25 20:21, Thomas Huth wrote:
> QEMU currently crashes when you try to inspect the machines based on
> TYPE_PC_MACHINE for their properties:
> 
>   $ echo '{ "execute": "qmp_capabilities" }
>           { "execute": "qom-list-properties","arguments":
>                        { "typename": "pc-q35-10.0-machine"}}' \
>     | ./qemu-system-x86_64 -M pc -qmp stdio
>   {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
>    "package": "v9.2.0-1070-g87e115c122-dirty"}, "capabilities": ["oob"]}}
>   {"return": {}}
>   Segmentation fault (core dumped)
> 
> This happens because TYPE_PC_MACHINE machines add a machine_init-
> done_notifier in their instance_init function - but instance_init
> of machines are not only called for machines that are realized,
> but also for machines that are introspected, so in this case the
> listener is added for a q35 machine that is never realized. But
> since there is already a running pc machine, the listener function
> is triggered immediately, causing a crash since it was not for the
> right machine it was meant for.
> 
> Such listener functions must never be installed from an instance_init
> function. Let's do it from pc_basic_device_init() instead - this
> function is called from the MachineClass->init() function instead,
> i.e. guaranteed to be only called once in the lifetime of a QEMU
> process.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2779
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/i386/pc.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b46975c8a4..85b8a76455 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1241,6 +1241,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>       /* Super I/O */
>       pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>                       pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
> +
> +    pcms->machine_done.notify = pc_machine_done;

I could accept if we rename:

   pc_machine_done() -> pc_basic_device_init_done[_notifier]()

> +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>   }
>   
>   void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
> @@ -1714,9 +1717,6 @@ static void pc_machine_initfn(Object *obj)
>       if (pcmc->pci_enabled) {
>           cxl_machine_init(obj, &pcms->cxl_devices_state);
>       }
> -
> -    pcms->machine_done.notify = pc_machine_done;
> -    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>   }
>   
>   static void pc_machine_reset(MachineState *machine, ResetType type)

Re: [PATCH] hw/i386/pc: Fix crash that occurs when introspecting TYPE_PC_MACHINE machines

Posted by Thomas Huth 1 year ago

On 29/01/2025 08.11, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 17/1/25 20:21, Thomas Huth wrote:
>> QEMU currently crashes when you try to inspect the machines based on
>> TYPE_PC_MACHINE for their properties:
>>
>>   $ echo '{ "execute": "qmp_capabilities" }
>>           { "execute": "qom-list-properties","arguments":
>>                        { "typename": "pc-q35-10.0-machine"}}' \
>>     | ./qemu-system-x86_64 -M pc -qmp stdio
>>   {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
>>    "package": "v9.2.0-1070-g87e115c122-dirty"}, "capabilities": ["oob"]}}
>>   {"return": {}}
>>   Segmentation fault (core dumped)
>>
>> This happens because TYPE_PC_MACHINE machines add a machine_init-
>> done_notifier in their instance_init function - but instance_init
>> of machines are not only called for machines that are realized,
>> but also for machines that are introspected, so in this case the
>> listener is added for a q35 machine that is never realized. But
>> since there is already a running pc machine, the listener function
>> is triggered immediately, causing a crash since it was not for the
>> right machine it was meant for.
>>
>> Such listener functions must never be installed from an instance_init
>> function. Let's do it from pc_basic_device_init() instead - this
>> function is called from the MachineClass->init() function instead,
>> i.e. guaranteed to be only called once in the lifetime of a QEMU
>> process.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2779
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/i386/pc.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index b46975c8a4..85b8a76455 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1241,6 +1241,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>>       /* Super I/O */
>>       pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>>                       pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
>> +
>> +    pcms->machine_done.notify = pc_machine_done;
> 
> I could accept if we rename:
> 
>    pc_machine_done() -> pc_basic_device_init_done[_notifier]()

Not sure whether that's such a good idea: The notifier is called when the 
machine init has been done, not when the device init has been done. It just 
happens to be instantiated from a function called pc_basic_device_init() 
now, but that does not really mean that the notifier function should get a 
similar name, should it?

  Thomas


>> +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>>   }
>>   void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
>> @@ -1714,9 +1717,6 @@ static void pc_machine_initfn(Object *obj)
>>       if (pcmc->pci_enabled) {
>>           cxl_machine_init(obj, &pcms->cxl_devices_state);
>>       }
>> -
>> -    pcms->machine_done.notify = pc_machine_done;
>> -    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>>   }
>>   static void pc_machine_reset(MachineState *machine, ResetType type)
>

Re: [PATCH] hw/i386/pc: Fix crash that occurs when introspecting TYPE_PC_MACHINE machines

Posted by Thomas Huth 1 year ago

On 17/01/2025 20.21, Thomas Huth wrote:
> QEMU currently crashes when you try to inspect the machines based on
> TYPE_PC_MACHINE for their properties:
> 
>   $ echo '{ "execute": "qmp_capabilities" }
>           { "execute": "qom-list-properties","arguments":
>                        { "typename": "pc-q35-10.0-machine"}}' \
>     | ./qemu-system-x86_64 -M pc -qmp stdio
>   {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
>    "package": "v9.2.0-1070-g87e115c122-dirty"}, "capabilities": ["oob"]}}
>   {"return": {}}
>   Segmentation fault (core dumped)
> 
> This happens because TYPE_PC_MACHINE machines add a machine_init-
> done_notifier in their instance_init function - but instance_init
> of machines are not only called for machines that are realized,
> but also for machines that are introspected, so in this case the
> listener is added for a q35 machine that is never realized. But
> since there is already a running pc machine, the listener function
> is triggered immediately, causing a crash since it was not for the
> right machine it was meant for.
> 
> Such listener functions must never be installed from an instance_init
> function. Let's do it from pc_basic_device_init() instead - this
> function is called from the MachineClass->init() function instead,
> i.e. guaranteed to be only called once in the lifetime of a QEMU
> process.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2779
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/i386/pc.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b46975c8a4..85b8a76455 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1241,6 +1241,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>       /* Super I/O */
>       pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>                       pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
> +
> +    pcms->machine_done.notify = pc_machine_done;
> +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>   }
>   
>   void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
> @@ -1714,9 +1717,6 @@ static void pc_machine_initfn(Object *obj)
>       if (pcmc->pci_enabled) {
>           cxl_machine_init(obj, &pcms->cxl_devices_state);
>       }
> -
> -    pcms->machine_done.notify = pc_machine_done;
> -    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>   }
>   
>   static void pc_machine_reset(MachineState *machine, ResetType type)

Friendly ping!

  Thomas

Re: [PATCH] hw/i386/pc: Fix crash that occurs when introspecting TYPE_PC_MACHINE machines

Posted by Michael S. Tsirkin 1 year ago

On Wed, Jan 29, 2025 at 08:00:40AM +0100, Thomas Huth wrote:
> On 17/01/2025 20.21, Thomas Huth wrote:
> > QEMU currently crashes when you try to inspect the machines based on
> > TYPE_PC_MACHINE for their properties:
> > 
> >   $ echo '{ "execute": "qmp_capabilities" }
> >           { "execute": "qom-list-properties","arguments":
> >                        { "typename": "pc-q35-10.0-machine"}}' \
> >     | ./qemu-system-x86_64 -M pc -qmp stdio
> >   {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
> >    "package": "v9.2.0-1070-g87e115c122-dirty"}, "capabilities": ["oob"]}}
> >   {"return": {}}
> >   Segmentation fault (core dumped)
> > 
> > This happens because TYPE_PC_MACHINE machines add a machine_init-
> > done_notifier in their instance_init function - but instance_init
> > of machines are not only called for machines that are realized,
> > but also for machines that are introspected, so in this case the
> > listener is added for a q35 machine that is never realized. But
> > since there is already a running pc machine, the listener function
> > is triggered immediately, causing a crash since it was not for the
> > right machine it was meant for.
> > 
> > Such listener functions must never be installed from an instance_init
> > function. Let's do it from pc_basic_device_init() instead - this
> > function is called from the MachineClass->init() function instead,
> > i.e. guaranteed to be only called once in the lifetime of a QEMU
> > process.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2779
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   hw/i386/pc.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index b46975c8a4..85b8a76455 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1241,6 +1241,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
> >       /* Super I/O */
> >       pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
> >                       pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
> > +
> > +    pcms->machine_done.notify = pc_machine_done;
> > +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
> >   }
> >   void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
> > @@ -1714,9 +1717,6 @@ static void pc_machine_initfn(Object *obj)
> >       if (pcmc->pci_enabled) {
> >           cxl_machine_init(obj, &pcms->cxl_devices_state);
> >       }
> > -
> > -    pcms->machine_done.notify = pc_machine_done;
> > -    qemu_add_machine_init_done_notifier(&pcms->machine_done);
> >   }
> >   static void pc_machine_reset(MachineState *machine, ResetType type)
> 
> Friendly ping!
> 
>  Thomas


donnu how i missed it.  pls address Philip's comment though.

Re: [PATCH] hw/i386/pc: Fix crash that occurs when introspecting TYPE_PC_MACHINE machines

Posted by Thomas Huth 1 year ago

On 29/01/2025 21.04, Michael S. Tsirkin wrote:
> On Wed, Jan 29, 2025 at 08:00:40AM +0100, Thomas Huth wrote:
>> On 17/01/2025 20.21, Thomas Huth wrote:
>>> QEMU currently crashes when you try to inspect the machines based on
>>> TYPE_PC_MACHINE for their properties:
>>>
>>>    $ echo '{ "execute": "qmp_capabilities" }
>>>            { "execute": "qom-list-properties","arguments":
>>>                         { "typename": "pc-q35-10.0-machine"}}' \
>>>      | ./qemu-system-x86_64 -M pc -qmp stdio
>>>    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
>>>     "package": "v9.2.0-1070-g87e115c122-dirty"}, "capabilities": ["oob"]}}
>>>    {"return": {}}
>>>    Segmentation fault (core dumped)
>>>
>>> This happens because TYPE_PC_MACHINE machines add a machine_init-
>>> done_notifier in their instance_init function - but instance_init
>>> of machines are not only called for machines that are realized,
>>> but also for machines that are introspected, so in this case the
>>> listener is added for a q35 machine that is never realized. But
>>> since there is already a running pc machine, the listener function
>>> is triggered immediately, causing a crash since it was not for the
>>> right machine it was meant for.
>>>
>>> Such listener functions must never be installed from an instance_init
>>> function. Let's do it from pc_basic_device_init() instead - this
>>> function is called from the MachineClass->init() function instead,
>>> i.e. guaranteed to be only called once in the lifetime of a QEMU
>>> process.
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2779
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>    hw/i386/pc.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index b46975c8a4..85b8a76455 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1241,6 +1241,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>>>        /* Super I/O */
>>>        pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>>>                        pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
>>> +
>>> +    pcms->machine_done.notify = pc_machine_done;
>>> +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>>>    }
>>>    void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
>>> @@ -1714,9 +1717,6 @@ static void pc_machine_initfn(Object *obj)
>>>        if (pcmc->pci_enabled) {
>>>            cxl_machine_init(obj, &pcms->cxl_devices_state);
>>>        }
>>> -
>>> -    pcms->machine_done.notify = pc_machine_done;
>>> -    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>>>    }
>>>    static void pc_machine_reset(MachineState *machine, ResetType type)
>>
>> Friendly ping!
>>
>>   Thomas
> 
> 
> donnu how i missed it.  pls address Philip's comment though.

  Hi Michael,

I think we should *not* rename pc_machine_done() since this is about a 
"machine_done" notifier, not about a "basic_device_init_done" or whatever 
notifier. So I'd prefer if we can keep this patch as it is. Unless you 
disagree, could you please pick this up?

  Thanks,
   Thomas

Re: [PATCH] hw/i386/pc: Fix crash that occurs when introspecting TYPE_PC_MACHINE machines

Posted by Philippe Mathieu-Daudé 1 year ago

On 4/2/25 09:57, Thomas Huth wrote:
> On 29/01/2025 21.04, Michael S. Tsirkin wrote:
>> On Wed, Jan 29, 2025 at 08:00:40AM +0100, Thomas Huth wrote:
>>> On 17/01/2025 20.21, Thomas Huth wrote:
>>>> QEMU currently crashes when you try to inspect the machines based on
>>>> TYPE_PC_MACHINE for their properties:
>>>>
>>>>    $ echo '{ "execute": "qmp_capabilities" }
>>>>            { "execute": "qom-list-properties","arguments":
>>>>                         { "typename": "pc-q35-10.0-machine"}}' \
>>>>      | ./qemu-system-x86_64 -M pc -qmp stdio
>>>>    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
>>>>     "package": "v9.2.0-1070-g87e115c122-dirty"}, "capabilities": 
>>>> ["oob"]}}
>>>>    {"return": {}}
>>>>    Segmentation fault (core dumped)
>>>>
>>>> This happens because TYPE_PC_MACHINE machines add a machine_init-
>>>> done_notifier in their instance_init function - but instance_init
>>>> of machines are not only called for machines that are realized,
>>>> but also for machines that are introspected, so in this case the
>>>> listener is added for a q35 machine that is never realized. But
>>>> since there is already a running pc machine, the listener function
>>>> is triggered immediately, causing a crash since it was not for the
>>>> right machine it was meant for.
>>>>
>>>> Such listener functions must never be installed from an instance_init
>>>> function. Let's do it from pc_basic_device_init() instead - this
>>>> function is called from the MachineClass->init() function instead,
>>>> i.e. guaranteed to be only called once in the lifetime of a QEMU
>>>> process.
>>>>
>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2779
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    hw/i386/pc.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index b46975c8a4..85b8a76455 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -1241,6 +1241,9 @@ void pc_basic_device_init(struct 
>>>> PCMachineState *pcms,
>>>>        /* Super I/O */
>>>>        pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>>>>                        pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
>>>> +
>>>> +    pcms->machine_done.notify = pc_machine_done;
>>>> +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>>>>    }
>>>>    void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus 
>>>> *pci_bus)
>>>> @@ -1714,9 +1717,6 @@ static void pc_machine_initfn(Object *obj)
>>>>        if (pcmc->pci_enabled) {
>>>>            cxl_machine_init(obj, &pcms->cxl_devices_state);
>>>>        }
>>>> -
>>>> -    pcms->machine_done.notify = pc_machine_done;
>>>> -    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>>>>    }
>>>>    static void pc_machine_reset(MachineState *machine, ResetType type)
>>>
>>> Friendly ping!
>>>
>>>   Thomas
>>
>>
>> donnu how i missed it.  pls address Philip's comment though.
> 
>   Hi Michael,
> 
> I think we should *not* rename pc_machine_done() since this is about a 
> "machine_done" notifier, not about a "basic_device_init_done" or 
> whatever notifier. So I'd prefer if we can keep this patch as it is. 
> Unless you disagree, could you please pick this up?

I don't have a strong opinion on this, so fine by me!