Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
On 21/08/2017 12:35, David Gibson wrote: > From: Thomas Huth > > QEMU currently crashes when trying to use a 'pc-dimm' on the pseries > machine without specifying its 'memdev' property. This happens because > pc_dimm_get_memory_region() does not check whether the 'memdev' property > has properly been set by the user. Looking closer at this function, it's > also obvious that it is using &error_abort to call another function - and > this is bad in a function that is used in the hot-plugging calling chain > since this can also cause QEMU to exit unexpectedly. > > So let's fix these issues in a proper way now: Add a "Error **errp" > parameter to pc_dimm_get_memory_region() which we use in case the 'memdev' > property has not been set by the user, and which we can use instead of > the &error_abort, and change the callers of get_memory_region() to make > use of this "errp" parameter for proper error checking. > > Signed-off-by: Thomas Huth > Reviewed-by: Igor Mammedov > Signed-off-by: David Gibson > --- > hw/i386/pc.c | 14 -- > hw/mem/nvdimm.c | 2 +- > hw/mem/pc-dimm.c | 14 +++--- > hw/ppc/spapr.c | 42 ++ > include/hw/mem/pc-dimm.h | 2 +- > 5 files changed, 55 insertions(+), 19 deletions(-) > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
On 21.08.2017 14:09, Cornelia Huck wrote:
> On Mon, 21 Aug 2017 20:35:24 +1000
> David Gibson wrote:
>
>> From: Thomas Huth
>>
>> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
>> machine without specifying its 'memdev' property. This happens because
>> pc_dimm_get_memory_region() does not check whether the 'memdev' property
>> has properly been set by the user. Looking closer at this function, it's
>> also obvious that it is using &error_abort to call another function - and
>> this is bad in a function that is used in the hot-plugging calling chain
>> since this can also cause QEMU to exit unexpectedly.
>>
>> So let's fix these issues in a proper way now: Add a "Error **errp"
>> parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
>> property has not been set by the user, and which we can use instead of
>> the &error_abort, and change the callers of get_memory_region() to make
>> use of this "errp" parameter for proper error checking.
>>
>> Signed-off-by: Thomas Huth
>> Reviewed-by: Igor Mammedov
>> Signed-off-by: David Gibson
>> ---
>> hw/i386/pc.c | 14 --
>> hw/mem/nvdimm.c | 2 +-
>> hw/mem/pc-dimm.c | 14 +++---
>> hw/ppc/spapr.c | 42 ++
>> include/hw/mem/pc-dimm.h | 2 +-
>> 5 files changed, 55 insertions(+), 19 deletions(-)
>
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index ea67b461c2..bdf6649083 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v,
>> const char *name,
>> PCDIMMDevice *dimm = PC_DIMM(obj);
>> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>>
>> -mr = ddc->get_memory_region(dimm);
>> +mr = ddc->get_memory_region(dimm, errp);
>> +if (!mr) {
>> +return;
>
> What happens if mr == NULL, but no error was set (backend memory not
> inited case)?
Looks like this currently never happens™ ... otherwise someone would
have experienced a crash in memory_region_size() which derefernces mr.
Anyway, we should eventually modify host_memory_backend_get_memory() to
correctly set the errp in that case. But since this is a slightly
different issue, I think this can go into a separate patch instead... so
I'll sent a separate patch for that later...
Thomas
Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
On Mon, 21 Aug 2017 20:35:24 +1000
David Gibson wrote:
> From: Thomas Huth
>
> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> machine without specifying its 'memdev' property. This happens because
> pc_dimm_get_memory_region() does not check whether the 'memdev' property
> has properly been set by the user. Looking closer at this function, it's
> also obvious that it is using &error_abort to call another function - and
> this is bad in a function that is used in the hot-plugging calling chain
> since this can also cause QEMU to exit unexpectedly.
>
> So let's fix these issues in a proper way now: Add a "Error **errp"
> parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
> property has not been set by the user, and which we can use instead of
> the &error_abort, and change the callers of get_memory_region() to make
> use of this "errp" parameter for proper error checking.
>
> Signed-off-by: Thomas Huth
> Reviewed-by: Igor Mammedov
> Signed-off-by: David Gibson
> ---
> hw/i386/pc.c | 14 --
> hw/mem/nvdimm.c | 2 +-
> hw/mem/pc-dimm.c | 14 +++---
> hw/ppc/spapr.c | 42 ++
> include/hw/mem/pc-dimm.h | 2 +-
> 5 files changed, 55 insertions(+), 19 deletions(-)
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index ea67b461c2..bdf6649083 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v,
> const char *name,
> PCDIMMDevice *dimm = PC_DIMM(obj);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>
> -mr = ddc->get_memory_region(dimm);
> +mr = ddc->get_memory_region(dimm, errp);
> +if (!mr) {
> +return;
What happens if mr == NULL, but no error was set (backend memory not
inited case)?
> +}
> value = memory_region_size(mr);
>
> visit_type_uint64(v, name, &value, errp);
[Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
From: Thomas Huth
QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
machine without specifying its 'memdev' property. This happens because
pc_dimm_get_memory_region() does not check whether the 'memdev' property
has properly been set by the user. Looking closer at this function, it's
also obvious that it is using &error_abort to call another function - and
this is bad in a function that is used in the hot-plugging calling chain
since this can also cause QEMU to exit unexpectedly.
So let's fix these issues in a proper way now: Add a "Error **errp"
parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
property has not been set by the user, and which we can use instead of
the &error_abort, and change the callers of get_memory_region() to make
use of this "errp" parameter for proper error checking.
Signed-off-by: Thomas Huth
Reviewed-by: Igor Mammedov
Signed-off-by: David Gibson
---
hw/i386/pc.c | 14 --
hw/mem/nvdimm.c | 2 +-
hw/mem/pc-dimm.c | 14 +++---
hw/ppc/spapr.c | 42 ++
include/hw/mem/pc-dimm.h | 2 +-
5 files changed, 55 insertions(+), 19 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 59435390ba..21081041d5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1691,10 +1691,15 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-MemoryRegion *mr = ddc->get_memory_region(dimm);
+MemoryRegion *mr;
uint64_t align = TARGET_PAGE_SIZE;
bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
+mr = ddc->get_memory_region(dimm, &local_err);
+if (local_err) {
+goto out;
+}
+
if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
align = memory_region_get_alignment(mr);
}
@@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
PCMachineState *pcms = PC_MACHINE(hotplug_dev);
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-MemoryRegion *mr = ddc->get_memory_region(dimm);
+MemoryRegion *mr;
HotplugHandlerClass *hhc;
Error *local_err = NULL;
+mr = ddc->get_memory_region(dimm, &local_err);
+if (local_err) {
+goto out;
+}
+
hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index db896b0bb6..952fce5ec8 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj)
NULL, NULL);
}
-static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
+static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
{
NVDIMMDevice *nvdimm = NVDIMM(dimm);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index ea67b461c2..bdf6649083 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v,
const char *name,
PCDIMMDevice *dimm = PC_DIMM(obj);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
-mr = ddc->get_memory_region(dimm);
+mr = ddc->get_memory_region(dimm, errp);
+if (!mr) {
+return;
+}
value = memory_region_size(mr);
visit_type_uint64(v, name, &value, errp);
@@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Error
**errp)
host_memory_backend_set_mapped(dimm->hostmem, false);
}
-static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
+static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error
**errp)
{
-return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+if (!dimm->hostmem) {
+error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
+return NULL;
+}
+
+return host_memory_backend_get_memory(dimm->hostmem, errp);
}
static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f7a19720dc..cec441cbf4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-MemoryRegion *mr = ddc->get_memory_region(dimm);
-uint64_t align = memory_region_get_alignment(mr);
-uint64_t size = memory_region_size(mr);
-uint64_t addr;
+MemoryRegion *mr;
+uint64_t align, size, addr;
+
+mr = ddc->get_memory_region(dimm, &local_err);
+if (local_err) {
+goto out;
+}
+align = memory_region_get_alignment(mr);
+size = memory_region_size(mr);
pc_dimm
Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
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
> >
> > 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
Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
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
>
> 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
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
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
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'
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
---
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
