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

From: Thomas Huth <thuth@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org,
	Alexander Graf <agraf@suse.de>
Subject: Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
Date: Fri, 18 Aug 2017 09:23:53 +0200	[thread overview]
Message-ID: <b08f13f1-39ba-b4c4-a64d-6a84619ecc09@redhat.com> (raw)
In-Reply-To: <20170818012501.GK5509@umbus.fritz.box>

[-- Attachment #1: Type: text/plain, Size: 2825 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]