[Qemu-devel] [PATCH v2] monitor: fix dangling CPU pointer

[Qemu-devel] [PATCH v2] monitor: fix dangling CPU pointer

[Greg Kurz]

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 store the QOM path instead of a
pointer to the current CPU. The path is then resolved when needed.
If the resolution fails, we assume that the CPU was removed and the
path is resetted to the default (ie, path of first_cpu).

Note that the resolution should really return a CPU object, otherwise
we have a bug. This is achieved by relying on object_resolve_path()
and CPU() instead of calling object_resolve_path_type(path, TYPE_CPU).

Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - use object_resolve_path_type()
    - add Reported-by tag
---
 monitor.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/monitor.c b/monitor.c
index fe0d1bdbb461..ce577e46e568 100644
--- a/monitor.c
+++ b/monitor.c
@@ -200,7 +200,7 @@ struct Monitor {
 
     ReadLineState *rs;
     MonitorQMP qmp;
-    CPUState *mon_cpu;
+    gchar *mon_cpu_path;
     BlockCompletionFunc *password_completion_cb;
     void *password_opaque;
     mon_cmd_t *cmd_table;
@@ -579,6 +579,7 @@ static void monitor_data_init(Monitor *mon)
 
 static void monitor_data_destroy(Monitor *mon)
 {
+    g_free(mon->mon_cpu_path);
     qemu_chr_fe_deinit(&mon->chr, false);
     if (monitor_is_qmp(mon)) {
         json_message_parser_destroy(&mon->qmp.parser);
@@ -1047,20 +1048,32 @@ int monitor_set_cpu(int cpu_index)
     if (cpu == NULL) {
         return -1;
     }
-    cur_mon->mon_cpu = cpu;
+    g_free(cur_mon->mon_cpu_path);
+    cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
     return 0;
 }
 
 CPUState *mon_get_cpu(void)
 {
-    if (!cur_mon->mon_cpu) {
+    CPUState *cpu;
+
+    if (cur_mon->mon_cpu_path) {
+        cpu = (CPUState *) object_resolve_path_type(cur_mon->mon_cpu_path,
+                                                    TYPE_CPU, NULL);
+        if (!cpu) {
+            g_free(cur_mon->mon_cpu_path);
+            cur_mon->mon_cpu_path = NULL;
+        }
+    }
+    if (!cur_mon->mon_cpu_path) {
         if (!first_cpu) {
             return NULL;
         }
         monitor_set_cpu(first_cpu->cpu_index);
+        cpu = first_cpu;
     }
-    cpu_synchronize_state(cur_mon->mon_cpu);
-    return cur_mon->mon_cpu;
+    cpu_synchronize_state(cpu);
+    return cpu;
 }
 
 CPUArchState *mon_get_cpu_env(void)

Re: [Qemu-devel] [PATCH v2] monitor: fix dangling CPU pointer

[Igor Mammedov]

On Mon, 16 Oct 2017 20:47:56 +0200
Greg Kurz <groug@kaod.org> wrote:

...
> Note that the resolution should really return a CPU object, otherwise
> we have a bug. This is achieved by relying on object_resolve_path()
> and CPU() instead of calling object_resolve_path_type(path, TYPE_CPU).
Did you forget to remove this part?

> 
> Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - use object_resolve_path_type()
>     - add Reported-by tag
> ---
>  monitor.c |   23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index fe0d1bdbb461..ce577e46e568 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -200,7 +200,7 @@ struct Monitor {
>  
>      ReadLineState *rs;
>      MonitorQMP qmp;
> -    CPUState *mon_cpu;
> +    gchar *mon_cpu_path;
>      BlockCompletionFunc *password_completion_cb;
>      void *password_opaque;
>      mon_cmd_t *cmd_table;
> @@ -579,6 +579,7 @@ static void monitor_data_init(Monitor *mon)
>  
>  static void monitor_data_destroy(Monitor *mon)
>  {
> +    g_free(mon->mon_cpu_path);
>      qemu_chr_fe_deinit(&mon->chr, false);
>      if (monitor_is_qmp(mon)) {
>          json_message_parser_destroy(&mon->qmp.parser);
> @@ -1047,20 +1048,32 @@ int monitor_set_cpu(int cpu_index)
>      if (cpu == NULL) {
>          return -1;
>      }
> -    cur_mon->mon_cpu = cpu;
> +    g_free(cur_mon->mon_cpu_path);
> +    cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
>      return 0;
>  }
>  
>  CPUState *mon_get_cpu(void)
>  {
> -    if (!cur_mon->mon_cpu) {
> +    CPUState *cpu;
> +
> +    if (cur_mon->mon_cpu_path) {
> +        cpu = (CPUState *) object_resolve_path_type(cur_mon->mon_cpu_path,
> +                                                    TYPE_CPU, NULL);
> +        if (!cpu) {
> +            g_free(cur_mon->mon_cpu_path);
> +            cur_mon->mon_cpu_path = NULL;
> +        }
> +    }
> +    if (!cur_mon->mon_cpu_path) {
>          if (!first_cpu) {
>              return NULL;
>          }
>          monitor_set_cpu(first_cpu->cpu_index);
> +        cpu = first_cpu;
>      }
> -    cpu_synchronize_state(cur_mon->mon_cpu);
> -    return cur_mon->mon_cpu;
> +    cpu_synchronize_state(cpu);
> +    return cpu;
>  }
>  
>  CPUArchState *mon_get_cpu_env(void)
> 

Re: [Qemu-devel] [PATCH v2] monitor: fix dangling CPU pointer

[Greg Kurz]

On Tue, 17 Oct 2017 09:52:02 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 16 Oct 2017 20:47:56 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> ...
> > Note that the resolution should really return a CPU object, otherwise
> > we have a bug. This is achieved by relying on object_resolve_path()
> > and CPU() instead of calling object_resolve_path_type(path, TYPE_CPU).  
> Did you forget to remove this part?
> 

Argh yes I did... :-\ I'll send a v3 right away.

> > 
> > Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > v2: - use object_resolve_path_type()
> >     - add Reported-by tag
> > ---
> >  monitor.c |   23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index fe0d1bdbb461..ce577e46e568 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -200,7 +200,7 @@ struct Monitor {
> >  
> >      ReadLineState *rs;
> >      MonitorQMP qmp;
> > -    CPUState *mon_cpu;
> > +    gchar *mon_cpu_path;
> >      BlockCompletionFunc *password_completion_cb;
> >      void *password_opaque;
> >      mon_cmd_t *cmd_table;
> > @@ -579,6 +579,7 @@ static void monitor_data_init(Monitor *mon)
> >  
> >  static void monitor_data_destroy(Monitor *mon)
> >  {
> > +    g_free(mon->mon_cpu_path);
> >      qemu_chr_fe_deinit(&mon->chr, false);
> >      if (monitor_is_qmp(mon)) {
> >          json_message_parser_destroy(&mon->qmp.parser);
> > @@ -1047,20 +1048,32 @@ int monitor_set_cpu(int cpu_index)
> >      if (cpu == NULL) {
> >          return -1;
> >      }
> > -    cur_mon->mon_cpu = cpu;
> > +    g_free(cur_mon->mon_cpu_path);
> > +    cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> >      return 0;
> >  }
> >  
> >  CPUState *mon_get_cpu(void)
> >  {
> > -    if (!cur_mon->mon_cpu) {
> > +    CPUState *cpu;
> > +
> > +    if (cur_mon->mon_cpu_path) {
> > +        cpu = (CPUState *) object_resolve_path_type(cur_mon->mon_cpu_path,
> > +                                                    TYPE_CPU, NULL);
> > +        if (!cpu) {
> > +            g_free(cur_mon->mon_cpu_path);
> > +            cur_mon->mon_cpu_path = NULL;
> > +        }
> > +    }
> > +    if (!cur_mon->mon_cpu_path) {
> >          if (!first_cpu) {
> >              return NULL;
> >          }
> >          monitor_set_cpu(first_cpu->cpu_index);
> > +        cpu = first_cpu;
> >      }
> > -    cpu_synchronize_state(cur_mon->mon_cpu);
> > -    return cur_mon->mon_cpu;
> > +    cpu_synchronize_state(cpu);
> > +    return cpu;
> >  }
> >  
> >  CPUArchState *mon_get_cpu_env(void)
> >   
>