| Message ID | 146798352770.17402.11063109294574588761.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 08 Jul 2016 15:12:07 +0200 Greg Kurz <groug@kaod.org> wrote: > If the host has 8 threads/core and the guest is started with: > > -smp cores=1,threads=4,maxcpus=12 > > It is possible to crash QEMU by doing: > > (qemu) device_add host-spapr-cpu-core,core-id=16,id=foo > (qemu) device_del foo > Segmentation fault > > This is caused because spapr_core_unplug() assumes cpu_dt_id == core_id. > Even if it happens to be the case when the host and guest have the same > number of threads per core, it is conceptually wrong and we may pass a > bogus id to spapr_dr_connector_by_id() and spapr_core_release() crashes. > > Let's use cc->core_id, which is the id that was used to create th DR > connector. My bad, I got excited and pointed out the wrong culprit... it is cpu_index again of course ! Please find an updated explanation to be put in the changelog after "Segmentation fault": ======================================================================== This happens because spapr_core_unplug() assumes cpu_dt_id == core_id. As long as cpu_dt_id is derived from the non-table cpu_index, this is only true when you plug cores with contiguous ids. It is safer to be consistent: the DR connector was created with an index that is immediately written to cc->core_id, and spapr_core_plug() also relies on cc->core_id. Let's use it also in spapr_core_unplug(). ======================================================================== > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr_cpu_core.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index 70b6b0b5ee17..106eaf45b399 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -126,11 +126,9 @@ static void spapr_core_release(DeviceState *dev, void *opaque) > void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > - sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev)); > - PowerPCCPU *cpu = POWERPC_CPU(core->threads); > - int id = ppc_get_vcpu_dt_id(cpu); > + CPUCore *cc = CPU_CORE(dev); > sPAPRDRConnector *drc = > - spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id); > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id); > sPAPRDRConnectorClass *drck; > Error *local_err = NULL; > > >
On Fri, Jul 08, 2016 at 05:47:01PM +0200, Greg Kurz wrote: > On Fri, 08 Jul 2016 15:12:07 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > If the host has 8 threads/core and the guest is started with: > > > > -smp cores=1,threads=4,maxcpus=12 > > > > It is possible to crash QEMU by doing: > > > > (qemu) device_add host-spapr-cpu-core,core-id=16,id=foo > > (qemu) device_del foo > > Segmentation fault > > > > This is caused because spapr_core_unplug() assumes cpu_dt_id == core_id. > > Even if it happens to be the case when the host and guest have the same > > number of threads per core, it is conceptually wrong and we may pass a > > bogus id to spapr_dr_connector_by_id() and spapr_core_release() crashes. > > > > Let's use cc->core_id, which is the id that was used to create th DR > > connector. > > My bad, I got excited and pointed out the wrong culprit... it is cpu_index > again of course ! Please find an updated explanation to be put in the > changelog after "Segmentation fault": > > ======================================================================== > This happens because spapr_core_unplug() assumes cpu_dt_id == core_id. > As long as cpu_dt_id is derived from the non-table cpu_index, this is > only true when you plug cores with contiguous ids. > > It is safer to be consistent: the DR connector was created with an > index that is immediately written to cc->core_id, and spapr_core_plug() > also relies on cc->core_id. > > Let's use it also in spapr_core_unplug(). > ======================================================================== > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com> This prevents the crash, but unplug still fails and that will be fixed only by having your patchset where device tree id is derived from core index. Regards, Bharata.
On Fri, Jul 08, 2016 at 03:12:07PM +0200, Greg Kurz wrote: > If the host has 8 threads/core and the guest is started with: > > -smp cores=1,threads=4,maxcpus=12 > > It is possible to crash QEMU by doing: > > (qemu) device_add host-spapr-cpu-core,core-id=16,id=foo > (qemu) device_del foo > Segmentation fault > > This is caused because spapr_core_unplug() assumes cpu_dt_id == core_id. > Even if it happens to be the case when the host and guest have the same > number of threads per core, it is conceptually wrong and we may pass a > bogus id to spapr_dr_connector_by_id() and spapr_core_release() crashes. > > Let's use cc->core_id, which is the id that was used to create th DR > connector. > > Signed-off-by: Greg Kurz <groug@kaod.org> Thanks, applied to ppc-for-2.7. > --- > hw/ppc/spapr_cpu_core.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index 70b6b0b5ee17..106eaf45b399 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -126,11 +126,9 @@ static void spapr_core_release(DeviceState *dev, void *opaque) > void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > - sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev)); > - PowerPCCPU *cpu = POWERPC_CPU(core->threads); > - int id = ppc_get_vcpu_dt_id(cpu); > + CPUCore *cc = CPU_CORE(dev); > sPAPRDRConnector *drc = > - spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id); > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id); > sPAPRDRConnectorClass *drck; > Error *local_err = NULL; > >
On Fri, Jul 08, 2016 at 05:47:01PM +0200, Greg Kurz wrote: > On Fri, 08 Jul 2016 15:12:07 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > If the host has 8 threads/core and the guest is started with: > > > > -smp cores=1,threads=4,maxcpus=12 > > > > It is possible to crash QEMU by doing: > > > > (qemu) device_add host-spapr-cpu-core,core-id=16,id=foo > > (qemu) device_del foo > > Segmentation fault > > > > This is caused because spapr_core_unplug() assumes cpu_dt_id == core_id. > > Even if it happens to be the case when the host and guest have the same > > number of threads per core, it is conceptually wrong and we may pass a > > bogus id to spapr_dr_connector_by_id() and spapr_core_release() crashes. > > > > Let's use cc->core_id, which is the id that was used to create th DR > > connector. > > My bad, I got excited and pointed out the wrong culprit... it is cpu_index > again of course ! Please find an updated explanation to be put in the > changelog after "Segmentation fault": > > ======================================================================== > This happens because spapr_core_unplug() assumes cpu_dt_id == core_id. > As long as cpu_dt_id is derived from the non-table cpu_index, this is > only true when you plug cores with contiguous ids. > > It is safer to be consistent: the DR connector was created with an > index that is immediately written to cc->core_id, and spapr_core_plug() > also relies on cc->core_id. > > Let's use it also in spapr_core_unplug(). > ======================================================================== Reworded in place, thanks. > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/ppc/spapr_cpu_core.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index 70b6b0b5ee17..106eaf45b399 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -126,11 +126,9 @@ static void spapr_core_release(DeviceState *dev, void *opaque) > > void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > > Error **errp) > > { > > - sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev)); > > - PowerPCCPU *cpu = POWERPC_CPU(core->threads); > > - int id = ppc_get_vcpu_dt_id(cpu); > > + CPUCore *cc = CPU_CORE(dev); > > sPAPRDRConnector *drc = > > - spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id); > > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id); > > sPAPRDRConnectorClass *drck; > > Error *local_err = NULL; > > > > > > >
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index 70b6b0b5ee17..106eaf45b399 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -126,11 +126,9 @@ static void spapr_core_release(DeviceState *dev, void *opaque) void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev)); - PowerPCCPU *cpu = POWERPC_CPU(core->threads); - int id = ppc_get_vcpu_dt_id(cpu); + CPUCore *cc = CPU_CORE(dev); sPAPRDRConnector *drc = - spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id); + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id); sPAPRDRConnectorClass *drck; Error *local_err = NULL;
If the host has 8 threads/core and the guest is started with: -smp cores=1,threads=4,maxcpus=12 It is possible to crash QEMU by doing: (qemu) device_add host-spapr-cpu-core,core-id=16,id=foo (qemu) device_del foo Segmentation fault This is caused because spapr_core_unplug() assumes cpu_dt_id == core_id. Even if it happens to be the case when the host and guest have the same number of threads per core, it is conceptually wrong and we may pass a bogus id to spapr_dr_connector_by_id() and spapr_core_release() crashes. Let's use cc->core_id, which is the id that was used to create th DR connector. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr_cpu_core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)