| Message ID | 150788373123.25736.7359515819699182906.stgit@bahia |
|---|---|
| State | New |
| Headers | show |
| Series | monitor: increase the refcount of the current CPU | expand |
On Fri, 13 Oct 2017 10:35:31 +0200 Greg Kurz <groug@kaod.org> wrote: > If a CPU selected with the "cpu" command is hot-unplugged then "info cpus" > causes QEMU to exit: > > (qemu) device_del cpu1 > (qemu) info cpus > qemu:qemu_cpu_kick_thread: No such process > > This happens because "cpu" stores the pointer to the selected CPU into > the monitor structure. When the CPU is hot-unplugged, we end up with a > dangling pointer. The "info cpus" command then does: > > hmp_info_cpus() > monitor_get_cpu_index() > mon_get_cpu() > cpu_synchronize_state() <--- called with dangling pointer > > This could cause a QEMU crash as well. > > This patch switches the monitor to use object_ref() to ensure the > CPU object doesn't vanish unexpectedly. The reference is dropped > either when "cpu" is used to switch to another CPU, or when the > selected CPU is unrealized and cpu_list_remove() sets its cpu_index > back to UNASSIGNED_CPU_INDEX. I don't really like an idea of leaving dangling cpu around. Is it possible to store QOM path on set_cpu in monitor and resolving it each to instance each time it's needed? > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > monitor.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/monitor.c b/monitor.c > index fe0d1bdbb461..1c0b9a2c3ad3 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -579,6 +579,9 @@ static void monitor_data_init(Monitor *mon) > > static void monitor_data_destroy(Monitor *mon) > { > + if (mon->mon_cpu) { > + object_unref((Object *) mon->mon_cpu); > + } > qemu_chr_fe_deinit(&mon->chr, false); > if (monitor_is_qmp(mon)) { > json_message_parser_destroy(&mon->qmp.parser); > @@ -1047,12 +1050,21 @@ int monitor_set_cpu(int cpu_index) > if (cpu == NULL) { > return -1; > } > + if (cur_mon->mon_cpu) { > + object_unref((Object *) cur_mon->mon_cpu); > + } > cur_mon->mon_cpu = cpu; > + object_ref((Object *) cpu); > return 0; > } > > CPUState *mon_get_cpu(void) > { > + if (cur_mon->mon_cpu && > + cur_mon->mon_cpu->cpu_index == UNASSIGNED_CPU_INDEX) { > + object_unref((Object *) cur_mon->mon_cpu); > + cur_mon->mon_cpu = NULL; > + } > if (!cur_mon->mon_cpu) { > if (!first_cpu) { > return NULL; > >
On Fri, 13 Oct 2017 11:24:59 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Fri, 13 Oct 2017 10:35:31 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > If a CPU selected with the "cpu" command is hot-unplugged then "info cpus" > > causes QEMU to exit: > > > > (qemu) device_del cpu1 > > (qemu) info cpus > > qemu:qemu_cpu_kick_thread: No such process > > > > This happens because "cpu" stores the pointer to the selected CPU into > > the monitor structure. When the CPU is hot-unplugged, we end up with a > > dangling pointer. The "info cpus" command then does: > > > > hmp_info_cpus() > > monitor_get_cpu_index() > > mon_get_cpu() > > cpu_synchronize_state() <--- called with dangling pointer > > > > This could cause a QEMU crash as well. > > > > This patch switches the monitor to use object_ref() to ensure the > > CPU object doesn't vanish unexpectedly. The reference is dropped > > either when "cpu" is used to switch to another CPU, or when the > > selected CPU is unrealized and cpu_list_remove() sets its cpu_index > > back to UNASSIGNED_CPU_INDEX. > I don't really like an idea of leaving dangling cpu around. > Is it possible to store QOM path on set_cpu in monitor and > resolving it each to instance each time it's needed? > It sounds workable. Also it would allow the fix to not depend on patch 1 for ppc. Thanks for the suggestion! > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > monitor.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/monitor.c b/monitor.c > > index fe0d1bdbb461..1c0b9a2c3ad3 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -579,6 +579,9 @@ static void monitor_data_init(Monitor *mon) > > > > static void monitor_data_destroy(Monitor *mon) > > { > > + if (mon->mon_cpu) { > > + object_unref((Object *) mon->mon_cpu); > > + } > > qemu_chr_fe_deinit(&mon->chr, false); > > if (monitor_is_qmp(mon)) { > > json_message_parser_destroy(&mon->qmp.parser); > > @@ -1047,12 +1050,21 @@ int monitor_set_cpu(int cpu_index) > > if (cpu == NULL) { > > return -1; > > } > > + if (cur_mon->mon_cpu) { > > + object_unref((Object *) cur_mon->mon_cpu); > > + } > > cur_mon->mon_cpu = cpu; > > + object_ref((Object *) cpu); > > return 0; > > } > > > > CPUState *mon_get_cpu(void) > > { > > + if (cur_mon->mon_cpu && > > + cur_mon->mon_cpu->cpu_index == UNASSIGNED_CPU_INDEX) { > > + object_unref((Object *) cur_mon->mon_cpu); > > + cur_mon->mon_cpu = NULL; > > + } > > if (!cur_mon->mon_cpu) { > > if (!first_cpu) { > > return NULL; > > > > >
On Fri, 13 Oct 2017 12:05:46 +0200 Greg Kurz <groug@kaod.org> wrote: > On Fri, 13 Oct 2017 11:24:59 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > > On Fri, 13 Oct 2017 10:35:31 +0200 > > Greg Kurz <groug@kaod.org> wrote: > > > > > If a CPU selected with the "cpu" command is hot-unplugged then "info cpus" > > > causes QEMU to exit: > > > > > > (qemu) device_del cpu1 > > > (qemu) info cpus > > > qemu:qemu_cpu_kick_thread: No such process > > > > > > This happens because "cpu" stores the pointer to the selected CPU into > > > the monitor structure. When the CPU is hot-unplugged, we end up with a > > > dangling pointer. The "info cpus" command then does: > > > > > > hmp_info_cpus() > > > monitor_get_cpu_index() > > > mon_get_cpu() > > > cpu_synchronize_state() <--- called with dangling pointer > > > > > > This could cause a QEMU crash as well. > > > > > > This patch switches the monitor to use object_ref() to ensure the > > > CPU object doesn't vanish unexpectedly. The reference is dropped > > > either when "cpu" is used to switch to another CPU, or when the > > > selected CPU is unrealized and cpu_list_remove() sets its cpu_index > > > back to UNASSIGNED_CPU_INDEX. > > I don't really like an idea of leaving dangling cpu around. > > Is it possible to store QOM path on set_cpu in monitor and > > resolving it each to instance each time it's needed? > > > > It sounds workable. Also it would allow the fix to not depend on patch 1 > for ppc. yep, also if you'd touch these cpu_index based monitor commands, it might make sense to convert them to use qmo path directly instead if cpu_index (with command completion it would be easy to use) > > Thanks for the suggestion! > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > monitor.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/monitor.c b/monitor.c > > > index fe0d1bdbb461..1c0b9a2c3ad3 100644 > > > --- a/monitor.c > > > +++ b/monitor.c > > > @@ -579,6 +579,9 @@ static void monitor_data_init(Monitor *mon) > > > > > > static void monitor_data_destroy(Monitor *mon) > > > { > > > + if (mon->mon_cpu) { > > > + object_unref((Object *) mon->mon_cpu); > > > + } > > > qemu_chr_fe_deinit(&mon->chr, false); > > > if (monitor_is_qmp(mon)) { > > > json_message_parser_destroy(&mon->qmp.parser); > > > @@ -1047,12 +1050,21 @@ int monitor_set_cpu(int cpu_index) > > > if (cpu == NULL) { > > > return -1; > > > } > > > + if (cur_mon->mon_cpu) { > > > + object_unref((Object *) cur_mon->mon_cpu); > > > + } > > > cur_mon->mon_cpu = cpu; > > > + object_ref((Object *) cpu); > > > return 0; > > > } > > > > > > CPUState *mon_get_cpu(void) > > > { > > > + if (cur_mon->mon_cpu && > > > + cur_mon->mon_cpu->cpu_index == UNASSIGNED_CPU_INDEX) { > > > + object_unref((Object *) cur_mon->mon_cpu); > > > + cur_mon->mon_cpu = NULL; > > > + } > > > if (!cur_mon->mon_cpu) { > > > if (!first_cpu) { > > > return NULL; > > > > > > > > >
On Mon, 16 Oct 2017 10:07:18 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Fri, 13 Oct 2017 12:05:46 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > On Fri, 13 Oct 2017 11:24:59 +0200 > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > On Fri, 13 Oct 2017 10:35:31 +0200 > > > Greg Kurz <groug@kaod.org> wrote: > > > > > > > If a CPU selected with the "cpu" command is hot-unplugged then "info cpus" > > > > causes QEMU to exit: > > > > > > > > (qemu) device_del cpu1 > > > > (qemu) info cpus > > > > qemu:qemu_cpu_kick_thread: No such process > > > > > > > > This happens because "cpu" stores the pointer to the selected CPU into > > > > the monitor structure. When the CPU is hot-unplugged, we end up with a > > > > dangling pointer. The "info cpus" command then does: > > > > > > > > hmp_info_cpus() > > > > monitor_get_cpu_index() > > > > mon_get_cpu() > > > > cpu_synchronize_state() <--- called with dangling pointer > > > > > > > > This could cause a QEMU crash as well. > > > > > > > > This patch switches the monitor to use object_ref() to ensure the > > > > CPU object doesn't vanish unexpectedly. The reference is dropped > > > > either when "cpu" is used to switch to another CPU, or when the > > > > selected CPU is unrealized and cpu_list_remove() sets its cpu_index > > > > back to UNASSIGNED_CPU_INDEX. > > > I don't really like an idea of leaving dangling cpu around. > > > Is it possible to store QOM path on set_cpu in monitor and > > > resolving it each to instance each time it's needed? > > > > > > > It sounds workable. Also it would allow the fix to not depend on patch 1 > > for ppc. > yep, > also if you'd touch these cpu_index based monitor commands, > it might make sense to convert them to use qmo path directly > instead if cpu_index (with command completion it would be > easy to use) > Ok, I'll look into that but I'm not sure I can work something out before soft freeze (2017-10-31 AFAIK). > > > > > Thanks for the suggestion! > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > monitor.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/monitor.c b/monitor.c > > > > index fe0d1bdbb461..1c0b9a2c3ad3 100644 > > > > --- a/monitor.c > > > > +++ b/monitor.c > > > > @@ -579,6 +579,9 @@ static void monitor_data_init(Monitor *mon) > > > > > > > > static void monitor_data_destroy(Monitor *mon) > > > > { > > > > + if (mon->mon_cpu) { > > > > + object_unref((Object *) mon->mon_cpu); > > > > + } > > > > qemu_chr_fe_deinit(&mon->chr, false); > > > > if (monitor_is_qmp(mon)) { > > > > json_message_parser_destroy(&mon->qmp.parser); > > > > @@ -1047,12 +1050,21 @@ int monitor_set_cpu(int cpu_index) > > > > if (cpu == NULL) { > > > > return -1; > > > > } > > > > + if (cur_mon->mon_cpu) { > > > > + object_unref((Object *) cur_mon->mon_cpu); > > > > + } > > > > cur_mon->mon_cpu = cpu; > > > > + object_ref((Object *) cpu); > > > > return 0; > > > > } > > > > > > > > CPUState *mon_get_cpu(void) > > > > { > > > > + if (cur_mon->mon_cpu && > > > > + cur_mon->mon_cpu->cpu_index == UNASSIGNED_CPU_INDEX) { > > > > + object_unref((Object *) cur_mon->mon_cpu); > > > > + cur_mon->mon_cpu = NULL; > > > > + } > > > > if (!cur_mon->mon_cpu) { > > > > if (!first_cpu) { > > > > return NULL; > > > > > > > > > > > > > >
diff --git a/monitor.c b/monitor.c index fe0d1bdbb461..1c0b9a2c3ad3 100644 --- a/monitor.c +++ b/monitor.c @@ -579,6 +579,9 @@ static void monitor_data_init(Monitor *mon) static void monitor_data_destroy(Monitor *mon) { + if (mon->mon_cpu) { + object_unref((Object *) mon->mon_cpu); + } qemu_chr_fe_deinit(&mon->chr, false); if (monitor_is_qmp(mon)) { json_message_parser_destroy(&mon->qmp.parser); @@ -1047,12 +1050,21 @@ int monitor_set_cpu(int cpu_index) if (cpu == NULL) { return -1; } + if (cur_mon->mon_cpu) { + object_unref((Object *) cur_mon->mon_cpu); + } cur_mon->mon_cpu = cpu; + object_ref((Object *) cpu); return 0; } CPUState *mon_get_cpu(void) { + if (cur_mon->mon_cpu && + cur_mon->mon_cpu->cpu_index == UNASSIGNED_CPU_INDEX) { + object_unref((Object *) cur_mon->mon_cpu); + cur_mon->mon_cpu = NULL; + } if (!cur_mon->mon_cpu) { if (!first_cpu) { return NULL;
If a CPU selected with the "cpu" command is hot-unplugged then "info cpus" causes QEMU to exit: (qemu) device_del cpu1 (qemu) info cpus qemu:qemu_cpu_kick_thread: No such process This happens because "cpu" stores the pointer to the selected CPU into the monitor structure. When the CPU is hot-unplugged, we end up with a dangling pointer. The "info cpus" command then does: hmp_info_cpus() monitor_get_cpu_index() mon_get_cpu() cpu_synchronize_state() <--- called with dangling pointer This could cause a QEMU crash as well. This patch switches the monitor to use object_ref() to ensure the CPU object doesn't vanish unexpectedly. The reference is dropped either when "cpu" is used to switch to another CPU, or when the selected CPU is unrealized and cpu_list_remove() sets its cpu_index back to UNASSIGNED_CPU_INDEX. Signed-off-by: Greg Kurz <groug@kaod.org> --- monitor.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)