| Message ID | 20170616013753.GA9658@in.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 16, 2017 at 07:07:53AM +0530, Bharata B Rao wrote: > On Thu, Jun 15, 2017 at 09:32:38AM +0200, Greg Kurz wrote: > > On Thu, 15 Jun 2017 08:22:44 +0530 > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > ICPState objects were being allocated before CPU thread realization. > > > However commit 9ed656631d73 (xics: setup cpu at realize time) reversed it > > > by allocating ICPState objects after CPU thread is realized. But it > > > didn't take care to fix the error path because of which we observe > > > a SIGSEGV when CPU thread realization fails during cold/hotplug. > > > > > > Fix this by ensuring that we do object_unparent() of ICPState object > > > only in case when is was created earlier. > > > > > > > Oops, my bad... my initial intent was to conditionally call object_unparent() > > and I simply forgot to put the "if (obj) { }". But your patch is valid as well > > of course. Maybe you can drop the initialization of obj to NULL on the way, > > since it really doesn't make sense anymore. > > Here is the version w/o initializing obj to NULL. > > >From cb9cc946df0d1c430cccb1463d78fa4b41e9f0ee Mon Sep 17 00:00:00 2001 > From: Bharata B Rao <bharata@linux.vnet.ibm.com> > Date: Wed, 14 Jun 2017 19:24:43 +0530 > Subject: [FIX PATCH v1] spapr: prevent QEMU crash when CPU realization > fails > > ICPState objects were being allocated before CPU thread realization. > However commit 9ed656631d73 (xics: setup cpu at realize time) reversed it > by allocating ICPState objects after CPU thread is realized. But it > didn't take care to fix the error path because of which we observe > a SIGSEGV when CPU thread realization fails during cold/hotplug. > > Fix this by ensuring that we do object_unparent() of ICPState object > only in case when is was created earlier. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > Reviewed-by: Greg Kurz <groug@kaod.org> I've replaced the version in my tree with this newer one. > --- > hw/ppc/spapr_cpu_core.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index d6719d5..ea278ce 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -178,7 +178,7 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp) > sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > CPUState *cs = CPU(child); > PowerPCCPU *cpu = POWERPC_CPU(cs); > - Object *obj = NULL; > + Object *obj; > > object_property_set_bool(child, true, "realized", &local_err); > if (local_err) { > @@ -198,13 +198,14 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp) > object_property_add_const_link(obj, ICP_PROP_CPU, child, &error_abort); > object_property_set_bool(obj, true, "realized", &local_err); > if (local_err) { > - goto error; > + goto free_icp; > } > > return; > > -error: > +free_icp: > object_unparent(obj); > +error: > error_propagate(errp, local_err); > } >
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index d6719d5..ea278ce 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -178,7 +178,7 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp) sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); CPUState *cs = CPU(child); PowerPCCPU *cpu = POWERPC_CPU(cs); - Object *obj = NULL; + Object *obj; object_property_set_bool(child, true, "realized", &local_err); if (local_err) { @@ -198,13 +198,14 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp) object_property_add_const_link(obj, ICP_PROP_CPU, child, &error_abort); object_property_set_bool(obj, true, "realized", &local_err); if (local_err) { - goto error; + goto free_icp; } return; -error: +free_icp: object_unparent(obj); +error: error_propagate(errp, local_err); }