1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'

Thomas Huth posted 1 patch 6 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1502994790-21856-1-git-send-email-thuth@redhat.com
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
hw/ppc/spapr.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
Posted by Thomas Huth 6 years, 8 months ago 
QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
machine without specifying its 'memdev' property. Let's add a sanity
check to the pre_plug handler to fix this issue.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f7a1972..22d400a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2808,10 +2808,17 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 {
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm);
-    uint64_t size = memory_region_size(mr);
+    MemoryRegion *mr;
+    uint64_t size;
     char *mem_dev;
 
+    if (!dimm->hostmem) {
+        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
+        return;
+    }
+
+    mr = ddc->get_memory_region(dimm);
+    size = memory_region_size(mr);
     if (size % SPAPR_MEMORY_BLOCK_SIZE) {
         error_setg(errp, "Hotplugged memory size must be a multiple of "
                       "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
-- 
1.8.3.1
Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
Posted by David Gibson 6 years, 8 months ago 
On Thu, Aug 17, 2017 at 08:33:10PM +0200, Thomas Huth wrote:
> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> machine without specifying its 'memdev' property. Let's add a sanity
> check to the pre_plug handler to fix this issue.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Thanks for all these patches fixing little bugs in 2.10.

> ---
>  hw/ppc/spapr.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f7a1972..22d400a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2808,10 +2808,17 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  {
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t size = memory_region_size(mr);
> +    MemoryRegion *mr;
> +    uint64_t size;
>      char *mem_dev;
>  
> +    if (!dimm->hostmem) {

Isn't checking dimm->hostmem directly here an abstraction violation?
Could we just check for a NULL return from get_memory_region instead?


> +        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> +        return;
> +    }
> +
> +    mr = ddc->get_memory_region(dimm);
> +    size = memory_region_size(mr);
>      if (size % SPAPR_MEMORY_BLOCK_SIZE) {
>          error_setg(errp, "Hotplugged memory size must be a multiple of "
>                        "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
Posted by Thomas Huth 6 years, 8 months ago 
On 18.08.2017 03:25, David Gibson wrote:
> On Thu, Aug 17, 2017 at 08:33:10PM +0200, Thomas Huth wrote:
>> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
>> machine without specifying its 'memdev' property. Let's add a sanity
>> check to the pre_plug handler to fix this issue.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Thanks for all these patches fixing little bugs in 2.10.

... or 2.11 ;-) ... not sure if there will be another RC next week or
the final 2.10 release?

Anyway, the fixes are required for a new qtest that I'm working on
(calling device_add + device_del for all available devices), that's why
I'm coming up with all these patches now. There is another crash with
one of the ppc64 devices, where I don't know how to fix it yet - so if
somebody got a clue, help is appreciated:

$ qemu-system-ppc64 -nographic -S -nodefaults -monitor stdio -M pseries
QEMU 2.9.92 monitor - type 'help' for more information
(qemu) device_add macio-oldworld,id=x
(qemu) device_del x
(qemu) **
ERROR:qemu/qom/object.c:1611:object_get_canonical_path_component:
 assertion failed: (obj->parent != NULL)
Aborted (core dumped)

>> ---
>>  hw/ppc/spapr.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index f7a1972..22d400a 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2808,10 +2808,17 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>  {
>>      PCDIMMDevice *dimm = PC_DIMM(dev);
>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
>> -    uint64_t size = memory_region_size(mr);
>> +    MemoryRegion *mr;
>> +    uint64_t size;
>>      char *mem_dev;
>>  
>> +    if (!dimm->hostmem) {
> 
> Isn't checking dimm->hostmem directly here an abstraction violation?
> Could we just check for a NULL return from get_memory_region instead?

The crash happens within get_memory_region: pc_dimm_get_memory_region()
calls host_memory_backend_get_memory(), which calls
host_memory_backend_mr_inited() - and that function dereferences the
NULL pointer.

I could add an additional check to one of the called functions and
return NULL in case the pointer is already NULL ... do you prefer that?
Let me know, then I'll send a v2...

> 
>> +        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
>> +        return;
>> +    }
>> +
>> +    mr = ddc->get_memory_region(dimm);
>> +    size = memory_region_size(mr);
>>      if (size % SPAPR_MEMORY_BLOCK_SIZE) {
>>          error_setg(errp, "Hotplugged memory size must be a multiple of "
>>                        "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> 

 Thomas
Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
Posted by David Gibson 6 years, 8 months ago 
On Fri, Aug 18, 2017 at 09:23:53AM +0200, Thomas Huth wrote:
> On 18.08.2017 03:25, David Gibson wrote:
> > On Thu, Aug 17, 2017 at 08:33:10PM +0200, Thomas Huth wrote:
> >> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> >> machine without specifying its 'memdev' property. Let's add a sanity
> >> check to the pre_plug handler to fix this issue.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > Thanks for all these patches fixing little bugs in 2.10.
> 
> ... or 2.11 ;-) ... not sure if there will be another RC next week or
> the final 2.10 release?
> 
> Anyway, the fixes are required for a new qtest that I'm working on
> (calling device_add + device_del for all available devices), that's why
> I'm coming up with all these patches now. There is another crash with
> one of the ppc64 devices, where I don't know how to fix it yet - so if
> somebody got a clue, help is appreciated:
> 
> $ qemu-system-ppc64 -nographic -S -nodefaults -monitor stdio -M pseries
> QEMU 2.9.92 monitor - type 'help' for more information
> (qemu) device_add macio-oldworld,id=x
> (qemu) device_del x
> (qemu) **
> ERROR:qemu/qom/object.c:1611:object_get_canonical_path_component:
>  assertion failed: (obj->parent != NULL)
> Aborted (core dumped)
> 
> >> ---
> >>  hw/ppc/spapr.c | 11 +++++++++--
> >>  1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index f7a1972..22d400a 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -2808,10 +2808,17 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>  {
> >>      PCDIMMDevice *dimm = PC_DIMM(dev);
> >>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> >> -    uint64_t size = memory_region_size(mr);
> >> +    MemoryRegion *mr;
> >> +    uint64_t size;
> >>      char *mem_dev;
> >>  
> >> +    if (!dimm->hostmem) {
> > 
> > Isn't checking dimm->hostmem directly here an abstraction violation?
> > Could we just check for a NULL return from get_memory_region instead?
> 
> The crash happens within get_memory_region: pc_dimm_get_memory_region()
> calls host_memory_backend_get_memory(), which calls
> host_memory_backend_mr_inited() - and that function dereferences the
> NULL pointer.
> 
> I could add an additional check to one of the called functions and
> return NULL in case the pointer is already NULL ... do you prefer that?
> Let me know, then I'll send a v2...

Ah, right.  Yeah, I think this is essentially a bug in
get_memory_region() or one of its called functions.  They're unsafe to
call in circumstances that the caller can't really control of
determine (without breaking the abstraction wall).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.