[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a '
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev' |
Date: |
Mon, 21 Aug 2017 14:33:27 +1000 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
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 <address@hidden>
> >
> > 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
signature.asc
Description: PGP signature