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

[David Gibson <david@gibson.dropbear.id.au>]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]