Re: [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3788 bytes --]

On Wed, Apr 18, 2018 at 05:02:38PM +0800, Peter Xu wrote:
> diff --git a/monitor.c b/monitor.c
> index c93aa4e22b..f4951cafbc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
>      if (!mon->rs)
>          return;
>  
> +    qemu_mutex_lock(&mon->mon_lock);
>      readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
>      if (show_prompt)
>          readline_show_prompt(mon->rs);
> +    qemu_mutex_unlock(&mon->mon_lock);
>  }
>  
>  int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>                            void *opaque)
>  {
>      if (mon->rs) {
> +        qemu_mutex_lock(&mon->mon_lock);
>          readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
> +        qemu_mutex_unlock(&mon->mon_lock);
>          /* prompt is printed on return from the command handler */
>          return 0;
>      } else {

I'm not sure why the lock is being used around readline_start() and
readline_show_prompt().  There are other readline_*() callers who do not
take the lock, which is suspicious.

Can you explain the purpose of this?

> @@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
>      cur_mon->qmp.commands = &qmp_commands;
>  }
>  
> -/* set the current CPU defined by the user */
> -int monitor_set_cpu(int cpu_index)
> +static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)

This function requires the BQL since qemu_get_cpu() accesses the cpus
list without acquiring qemu_cpu_list_lock.

Two options:
1. Document that monitor_set_cpu() must be called with the BQL held.
2. Audit qemu_cpu_list_lock to check that it meets the out-of-band
   monitor code requirements, document that qemu_cpu_list_lock code must
   follow out-of-band monitor code requirements, and then take the lock.

#1 is more practical since we will probably never need to call
monitor_set_cpu() from out-of-band monitor code.  Anyway, in that case
mon_lock is not needed unless there is a mon field that needs to be
protected.

>  {
>      CPUState *cpu;
>  
> @@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
>      if (cpu == NULL) {
>          return -1;
>      }
> -    g_free(cur_mon->mon_cpu_path);
> -    cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> +    g_free(mon->mon_cpu_path);
> +    mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
>      return 0;
>  }
>  
> +/* set the current CPU defined by the user */
> +int monitor_set_cpu(int cpu_index)
> +{
> +    int ret;
> +
> +    qemu_mutex_lock(&cur_mon->mon_lock);
> +    ret = monitor_set_cpu_locked(cur_mon, cpu_index);
> +    qemu_mutex_unlock(&cur_mon->mon_lock);
> +
> +    return ret;
> +}
> +
>  static CPUState *mon_get_cpu_sync(bool synchronize)
>  {

This function calls monitor_set_cpu() so it must be called from the BQL.
The locking changes are probably not needed.  This function just needs
to be documented as BQL-only.

> @@ -2239,6 +2258,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>  {
>      mon_fd_t *monfd;
>  
> +    qemu_mutex_lock(&mon->mon_lock);
>      QLIST_FOREACH(monfd, &mon->fds, next) {
>          int fd;
>  
> @@ -2252,9 +2272,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>          QLIST_REMOVE(monfd, next);
>          g_free(monfd->name);
>          g_free(monfd);
> -
> +        qemu_mutex_unlock(&mon->mon_lock);
>          return fd;
>      }
> +    qemu_mutex_unlock(&mon->mon_lock);

What about all the other mon->fds users?  They need to lock too,
otherwise we will QLIST_REMOVE() an fd while they are accessing the
list!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]