| Message ID | 1405073498-14645-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 (sig=sig@entry=6) > 0x00007ffff27df388 in __GI_abort () > 0x00005555555ef489 in error_exit (err=<optimized out>, msg=msg@entry=0x5555559796d0 <__func__.5959> "qemu_mutex_lock") > 0x00005555558f9080 in qemu_mutex_lock (mutex=mutex@entry=0x555556248a30) > 0x0000555555713936 in qemu_chr_fe_write (s=0x555556248a30, buf=buf@entry=0x5555563d8870 "QEMU 2.0.90 monitor - type 'help' for more information\r\n", len=56) > 0x00005555556217fd in monitor_flush_locked (mon=mon@entry=0x555556251fd0) > 0x0000555555621a12 in monitor_flush_locked (mon=0x555556251fd0) > monitor_puts (mon=mon@entry=0x555556251fd0, str=0x55555634bfa7 "", str@entry=0x55555634bf70 "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>, fmt=fmt@entry=0x5555559105a0 "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 <liangx.z.li@intel.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > 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 <famz@redhat.com>
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); }
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 (sig=sig@entry=6) 0x00007ffff27df388 in __GI_abort () 0x00005555555ef489 in error_exit (err=<optimized out>, msg=msg@entry=0x5555559796d0 <__func__.5959> "qemu_mutex_lock") 0x00005555558f9080 in qemu_mutex_lock (mutex=mutex@entry=0x555556248a30) 0x0000555555713936 in qemu_chr_fe_write (s=0x555556248a30, buf=buf@entry=0x5555563d8870 "QEMU 2.0.90 monitor - type 'help' for more information\r\n", len=56) 0x00005555556217fd in monitor_flush_locked (mon=mon@entry=0x555556251fd0) 0x0000555555621a12 in monitor_flush_locked (mon=0x555556251fd0) monitor_puts (mon=mon@entry=0x555556251fd0, str=0x55555634bfa7 "", str@entry=0x55555634bf70 "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>, fmt=fmt@entry=0x5555559105a0 "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 <liangx.z.li@intel.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- qemu-char.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)