[v2,2/3] monitor: take mon_lock where proper

Went through all the montior.h APIs to make sure existing Monitor
related APIs will always take the new monitor lock when necessary.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

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!

On Mon, Apr 30, 2018 at 11:10:50AM +0100, Stefan Hajnoczi wrote:
> 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.

You are right.

After a second thought I think readline is not needed to be protected.
IMHO it's only used in parsing phase, so actually we don't have
multi-threading issue with that (parsing is either happening in main
thread only, or monitor iothread only).

So I'll drop all the readline_* protections, and add a comment for
monitor_set_cpu() on BQL.

> 
> >  {
> >      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.

Yes.  Will do.

> 
> > @@ -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!

Indeed!  I think I'll drop most of this patch, only add protection for
mon->fds, and add those comments that you suggested.  They make sense
to me.  Thanks,