* [Qemu-devel] [PATCH] qemu-char: fix deadlock with "-monitor pty"
@ 2014-07-11 10:11 Paolo Bonzini
2014-07-14 13:52 ` Fam Zheng
0 siblings, 1 reply; 2+ messages in thread
From: Paolo Bonzini @ 2014-07-11 10:11 UTC (permalink / raw)
To: qemu-devel; +Cc: liangx.z.li, peter.maydell, armbru
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-char: fix deadlock with "-monitor pty"
2014-07-11 10:11 [Qemu-devel] [PATCH] qemu-char: fix deadlock with "-monitor pty" Paolo Bonzini
@ 2014-07-14 13:52 ` Fam Zheng
0 siblings, 0 replies; 2+ messages in thread
From: Fam Zheng @ 2014-07-14 13:52 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: liangx.z.li, peter.maydell, qemu-devel, armbru
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>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-07-14 13:52 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-11 10:11 [Qemu-devel] [PATCH] qemu-char: fix deadlock with "-monitor pty" Paolo Bonzini
2014-07-14 13:52 ` Fam Zheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).