Re: [Qemu-devel] [PATCH] qemu-char: fix deadlock with "-monitor pty"

[Fam Zheng]

On Fri, 07/11 12:11, Paolo Bonzini wrote:
> qemu_chr_be_generic_open cannot be called with the write lock taken,
> because it calls client code that may call qemu_chr_fe_write.  This
> actually happens for the monitor:
> 
>     0x00007ffff27dbf79 in __GI_raise (address@hidden)
>     0x00007ffff27df388 in __GI_abort ()
>     0x00005555555ef489 in error_exit (err=<optimized out>, address@hidden 
> <__func__.5959> "qemu_mutex_lock")
>     0x00005555558f9080 in qemu_mutex_lock (address@hidden)
>     0x0000555555713936 in qemu_chr_fe_write (s=0x555556248a30, address@hidden 
> "QEMU 2.0.90 monitor - type 'help' for more information\r\n", len=56)
>     0x00005555556217fd in monitor_flush_locked (address@hidden)
>     0x0000555555621a12 in monitor_flush_locked (mon=0x555556251fd0)
>     monitor_puts (address@hidden, str=0x55555634bfa7 "", address@hidden "QEMU 
> 2.0.90 monitor - type 'help' for more information\n")
>     0x0000555555624359 in monitor_vprintf (mon=0x555556251fd0, fmt=<optimized 
> out>, ap=<optimized out>)
>     0x0000555555624414 in monitor_printf (mon=<optimized out>, address@hidden 
> "QEMU %s monitor - type 'help' for more information\n")
>     0x0000555555629806 in monitor_event (opaque=0x555556251fd0, 
> event=<optimized out>)
>     0x000055555571343c in qemu_chr_be_generic_open (s=0x555556248a30)
> 
> To avoid this, defer the call to an idle callback, which will be
> called as soon as the main loop is re-entered.  In order to simplify
> the cleanup and do it in one place only, change pty_chr_close to
> call pty_chr_state.
> 
> To reproduce, run with "-monitor pty", then try to read from the
> slave /dev/pts/FOO that it creates.
> 
> Fixes: 9005b2a7589540a3733b3abdcfbccfe7746cd1a1
> Reported-by: Li Liang <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  qemu-char.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 51917de..5bf99d1 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1080,6 +1080,7 @@ typedef struct {
>      /* Protected by the CharDriverState chr_write_lock.  */
>      int connected;
>      guint timer_tag;
> +    guint open_tag;
>  } PtyCharDriver;
>  
>  static void pty_chr_update_read_handler_locked(CharDriverState *chr);
> @@ -1092,6 +1093,7 @@ static gboolean pty_chr_timer(gpointer opaque)
>  
>      qemu_mutex_lock(&chr->chr_write_lock);
>      s->timer_tag = 0;
> +    s->open_tag = 0;
>      if (!s->connected) {
>          /* Next poll ... */
>          pty_chr_update_read_handler_locked(chr);
> @@ -1194,12 +1196,26 @@ static gboolean pty_chr_read(GIOChannel *chan, 
> GIOCondition cond, void *opaque)
>      return TRUE;
>  }
>  
> +static gboolean qemu_chr_be_generic_open_func(gpointer opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    PtyCharDriver *s = chr->opaque;
> +
> +    s->open_tag = 0;
> +    qemu_chr_be_generic_open(chr);
> +    return FALSE;
> +}
> +
>  /* Called with chr_write_lock held.  */
>  static void pty_chr_state(CharDriverState *chr, int connected)
>  {
>      PtyCharDriver *s = chr->opaque;
>  
>      if (!connected) {
> +        if (s->open_tag) {
> +            g_source_remove(s->open_tag);
> +            s->open_tag = 0;
> +        }
>          remove_fd_in_watch(chr);
>          s->connected = 0;
>          /* (re-)connect poll interval for idle guests: once per second.
> @@ -1212,8 +1228,9 @@ static void pty_chr_state(CharDriverState *chr, int 
> connected)
>              s->timer_tag = 0;
>          }
>          if (!s->connected) {
> +            g_assert(s->open_tag == 0);
>              s->connected = 1;
> -            qemu_chr_be_generic_open(chr);
> +            s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr);
>          }
>          if (!chr->fd_in_tag) {
>              chr->fd_in_tag = io_add_watch_poll(s->fd, pty_chr_read_poll,
> @@ -1227,7 +1244,8 @@ static void pty_chr_close(struct CharDriverState *chr)
>      PtyCharDriver *s = chr->opaque;
>      int fd;
>  
> -    remove_fd_in_watch(chr);
> +    qemu_mutex_lock(&chr->chr_write_lock);
> +    pty_chr_state(chr, 0);
>      fd = g_io_channel_unix_get_fd(s->fd);
>      g_io_channel_unref(s->fd);
>      close(fd);
> @@ -1235,6 +1253,7 @@ static void pty_chr_close(struct CharDriverState *chr)
>          g_source_remove(s->timer_tag);
>          s->timer_tag = 0;
>      }
> +    qemu_mutex_unlock(&chr->chr_write_lock);
>      g_free(s);
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>  }
> -- 
> 1.8.3.1
> 
> 

Looks good and fixes the deadlock.

Reviewed-by: Fam Zheng <address@hidden>