diff mbox

qemu-char: Fixed the bug lead to dead lock

Message ID 1405054738-18522-1-git-send-email-liangx.z.li@intel.com
State New
Headers show

Commit Message

Liliang July 11, 2014, 4:58 a.m. UTC 
From: Li Liang <liangx.z.li@intel.com>

This bug was introduced in the commit 9005b2a7589540a3733b3abdcfbccfe7746cd1a1,
it will cause deadlock when create a vm with the parameter "-monitor pty" and
then try to read from /dev/pts/x.

Signed-off-by: Li Liang <liangx.z.li@intel.com>
---
 qemu-char.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Markus Armbruster July 11, 2014, 9:07 a.m. UTC | #1 
Cc'ing folks involved in the flawed commit.

Liliang <liangx.z.li@intel.com> writes:

> From: Li Liang <liangx.z.li@intel.com>
>
> This bug was introduced in the commit 9005b2a7589540a3733b3abdcfbccfe7746cd1a1,
> it will cause deadlock when create a vm with the parameter "-monitor pty" and
> then try to read from /dev/pts/x.
>
> Signed-off-by: Li Liang <liangx.z.li@intel.com>

Long lines in commit message, and the subject isn't as informative as it
could be.  Suggest something like:

    qemu-char: Fix deadlock in pty character device

    To reproduce, run with "-monitor pty", then try to read from the
    slave /dev/pts/FOO created for it.

    Broken in commit 9005b2a.

> ---
>  qemu-char.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 55e372c..55cdded 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1103,9 +1103,11 @@ static gboolean pty_chr_timer(gpointer opaque)
>      s->timer_tag = 0;
>      if (!s->connected) {
>          /* Next poll ... */
> +        qemu_mutex_unlock(&chr->chr_write_lock);
>          pty_chr_update_read_handler_locked(chr);
> +    } else {
> +        qemu_mutex_unlock(&chr->chr_write_lock);
>      }
> -    qemu_mutex_unlock(&chr->chr_write_lock);
>      return FALSE;
>  }
Peter Maydell July 11, 2014, 9:07 a.m. UTC | #2 
On 11 July 2014 05:58, Liliang <liangx.z.li@intel.com> wrote:
> From: Li Liang <liangx.z.li@intel.com>
>
> This bug was introduced in the commit 9005b2a7589540a3733b3abdcfbccfe7746cd1a1,
> it will cause deadlock when create a vm with the parameter "-monitor pty" and
> then try to read from /dev/pts/x.
>
> Signed-off-by: Li Liang <liangx.z.li@intel.com>
> ---
>  qemu-char.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 55e372c..55cdded 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1103,9 +1103,11 @@ static gboolean pty_chr_timer(gpointer opaque)
>      s->timer_tag = 0;
>      if (!s->connected) {
>          /* Next poll ... */
> +        qemu_mutex_unlock(&chr->chr_write_lock);
>          pty_chr_update_read_handler_locked(chr);
> +    } else {
> +        qemu_mutex_unlock(&chr->chr_write_lock);
>      }
> -    qemu_mutex_unlock(&chr->chr_write_lock);
>      return FALSE;
>  }

This is clearly not the correct fix -- it is now calling
pty_chr_update_read_handler_locked() without holding
the chr_write_lock mutex, which is not permitted.

thanks
-- PMM
Liliang July 11, 2014, 9:30 a.m. UTC | #3 
Yes, my patch will break the thread safe implementation, just avoid the deadlock.
This is the call trace when the dead lock happened.

#0  0x00007ffff27dbf79 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff27df388 in __GI_abort () at abort.c:89
#2  0x00005555555ef489 in error_exit (err=<optimized out>, msg=msg@entry=0x5555559796d0 <__func__.5959> "qemu_mutex_lock") at util/qemu-thread-posix.c:47
#3  0x00005555558f9080 in qemu_mutex_lock (mutex=mutex@entry=0x555556248a30) at util/qemu-thread-posix.c:78
#4  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) at qemu-char.c:127
#5  0x00005555556217fd in monitor_flush_locked (mon=mon@entry=0x555556251fd0) at /home/liliang/vt-sync/qemu/monitor.c:310
#6  0x0000555555621a12 in monitor_flush_locked (mon=0x555556251fd0) at /home/liliang/vt-sync/qemu/monitor.c:302
#7  monitor_puts (mon=mon@entry=0x555556251fd0, str=0x55555634bfa7 "", str@entry=0x55555634bf70 "QEMU 2.0.90 monitor - type 'help' for more information\n") at /home/liliang/vt-sync/qemu/monitor.c:352
#8  0x0000555555624359 in monitor_vprintf (mon=0x555556251fd0, fmt=<optimized out>, ap=<optimized out>) at /home/liliang/vt-sync/qemu/monitor.c:370
#9  0x0000555555624414 in monitor_printf (mon=<optimized out>, fmt=fmt@entry=0x5555559105a0 "QEMU %s monitor - type 'help' for more information\n") at /home/liliang/vt-sync/qemu/monitor.c:378
#10 0x0000555555629806 in monitor_event (opaque=0x555556251fd0, event=<optimized out>) at /home/liliang/vt-sync/qemu/monitor.c:5243
#11 0x000055555571343c in qemu_chr_be_generic_open (s=0x555556248a30) at qemu-char.c:120
#12 pty_chr_state (chr=chr@entry=0x555556248a30, connected=connected@entry=1) at qemu-char.c:1216
#13 0x00005555557134ee in pty_chr_update_read_handler_locked (chr=chr@entry=0x555556248a30) at qemu-char.c:1133
#14 0x00005555557135ab in pty_chr_timer (opaque=0x555556248a30) at qemu-char.c:1097
#15 0x00007ffff629d703 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#16 0x00007ffff629cce5 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#17 0x00005555558ac888 in glib_pollfds_poll () at main-loop.c:190
#18 os_host_main_loop_wait (timeout=<optimized out>) at main-loop.c:235
#19 main_loop_wait (nonblocking=<optimized out>) at main-loop.c:484
#20 0x00005555555f153e in main_loop () at vl.c:2010
#21 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4530


It can be showed simply like bellow:

        ptr_char_timer ()
		{
        	qemu_mutex_lock(&chr->chr_write_lock);
            ...
            monitor_flush_locked()
			{        
       			...
       	  		qemu_chr_fe_write()
				{
         			qemu_mutex_lock(&chr->chr_write_lock);
        			...
         			qemu_mutex_unlock(&chr->chr_write_lock);
				}
				…
			}
            qemu_mutex_unlock(&chr->chr_write_lock);

		}

Acquire mutex in this way cause deadlock.


-----Original Message-----
From: Peter Maydell [mailto:peter.maydell@linaro.org] 

Sent: Friday, July 11, 2014 5:08 PM
To: Li, LiangX Z
Cc: QEMU Developers; Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH] qemu-char: Fixed the bug lead to dead lock

On 11 July 2014 05:58, Liliang <liangx.z.li@intel.com> wrote:
> From: Li Liang <liangx.z.li@intel.com>

>

> This bug was introduced in the commit 

> 9005b2a7589540a3733b3abdcfbccfe7746cd1a1,

> it will cause deadlock when create a vm with the parameter "-monitor 

> pty" and then try to read from /dev/pts/x.

>

> Signed-off-by: Li Liang <liangx.z.li@intel.com>

> ---

>  qemu-char.c | 4 +++-

>  1 file changed, 3 insertions(+), 1 deletion(-)

>

> diff --git a/qemu-char.c b/qemu-char.c index 55e372c..55cdded 100644

> --- a/qemu-char.c

> +++ b/qemu-char.c

> @@ -1103,9 +1103,11 @@ static gboolean pty_chr_timer(gpointer opaque)

>      s->timer_tag = 0;

>      if (!s->connected) {

>          /* Next poll ... */

> +        qemu_mutex_unlock(&chr->chr_write_lock);

>          pty_chr_update_read_handler_locked(chr);

> +    } else {

> +        qemu_mutex_unlock(&chr->chr_write_lock);

>      }

> -    qemu_mutex_unlock(&chr->chr_write_lock);

>      return FALSE;

>  }


This is clearly not the correct fix -- it is now calling
pty_chr_update_read_handler_locked() without holding the chr_write_lock mutex, which is not permitted.

thanks
-- PMM
Luiz Capitulino July 14, 2014, 5:56 p.m. UTC | #4 
On Fri, 11 Jul 2014 11:07:15 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Cc'ing folks involved in the flawed commit.
> 
> Liliang <liangx.z.li@intel.com> writes:
> 
> > From: Li Liang <liangx.z.li@intel.com>
> >
> > This bug was introduced in the commit 9005b2a7589540a3733b3abdcfbccfe7746cd1a1,
> > it will cause deadlock when create a vm with the parameter "-monitor pty" and
> > then try to read from /dev/pts/x.
> >
> > Signed-off-by: Li Liang <liangx.z.li@intel.com>
> 
> Long lines in commit message, and the subject isn't as informative as it
> could be.  Suggest something like:
> 
>     qemu-char: Fix deadlock in pty character device
> 
>     To reproduce, run with "-monitor pty", then try to read from the
>     slave /dev/pts/FOO created for it.
> 
>     Broken in commit 9005b2a.

Paolo posted a pull request today with a fix for this issue included:

[PULL 2/5] qemu-char: fix deadlock with "-monitor pty"

Thanks Paolo!

> 
> > ---
> >  qemu-char.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 55e372c..55cdded 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -1103,9 +1103,11 @@ static gboolean pty_chr_timer(gpointer opaque)
> >      s->timer_tag = 0;
> >      if (!s->connected) {
> >          /* Next poll ... */
> > +        qemu_mutex_unlock(&chr->chr_write_lock);
> >          pty_chr_update_read_handler_locked(chr);
> > +    } else {
> > +        qemu_mutex_unlock(&chr->chr_write_lock);
> >      }
> > -    qemu_mutex_unlock(&chr->chr_write_lock);
> >      return FALSE;
> >  }
>
diff mbox

Patch

diff --git a/qemu-char.c b/qemu-char.c
index 55e372c..55cdded 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1103,9 +1103,11 @@  static gboolean pty_chr_timer(gpointer opaque)
     s->timer_tag = 0;
     if (!s->connected) {
         /* Next poll ... */
+        qemu_mutex_unlock(&chr->chr_write_lock);
         pty_chr_update_read_handler_locked(chr);
+    } else {
+        qemu_mutex_unlock(&chr->chr_write_lock);
     }
-    qemu_mutex_unlock(&chr->chr_write_lock);
     return FALSE;
 }