[Qemu-devel] [PATCH 0/3] char-socket: Fix race condition

[Qemu-devel] [PATCH 0/3] char-socket: Fix race condition

[Alberto Garcia]

Hi,

this series fixes the crash I reported yesterday in char-socket and
adds a new qemu_idle_add() function, fixing a couple of memory leaks
along the way.

Regards,

Berto

Alberto Garcia (3):
  main-loop: Don't leak GSources attached to a GMainContext
  main-loop: Add qemu_idle_add()
  char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

 chardev/char-pty.c       |  8 ++------
 chardev/char-socket.c    | 19 +++++++++++++++++--
 include/qemu/main-loop.h |  8 ++++++++
 io/task.c                |  6 ++----
 util/main-loop.c         |  9 +++++++++
 5 files changed, 38 insertions(+), 12 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext

[Alberto Garcia]

After g_source_attach() the GMainContext holds a reference to the
GSource, so the caller does not need to keep it.

pty_chr_state() and qio_task_thread_worker() are not doing this, so
the GSource is being leaked in both cases (pty_chr_open_src_cancel()
is the exception here because it does remove the extra reference
correctly).

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 chardev/char-pty.c | 2 +-
 io/task.c          | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index f681d637c1..f16a5e8d59 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -60,7 +60,6 @@ static void pty_chr_open_src_cancel(PtyChardev *s)
 {
     if (s->open_source) {
         g_source_destroy(s->open_source);
-        g_source_unref(s->open_source);
         s->open_source = NULL;
     }
 }
@@ -216,6 +215,7 @@ static void pty_chr_state(Chardev *chr, int connected)
                                   qemu_chr_be_generic_open_func,
                                   chr, NULL);
             g_source_attach(s->open_source, chr->gcontext);
+            g_source_unref(s->open_source);
         }
         if (!chr->gsource) {
             chr->gsource = io_add_watch_poll(chr, s->ioc,
diff --git a/io/task.c b/io/task.c
index 2886a2c1bc..c8489fb790 100644
--- a/io/task.c
+++ b/io/task.c
@@ -120,6 +120,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
     idle = g_idle_source_new();
     g_source_set_callback(idle, qio_task_thread_result, data, NULL);
     g_source_attach(idle, data->context);
+    g_source_unref(idle);
 
     return NULL;
 }
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add()

[Alberto Garcia]

This works like g_idle_add() but allows specifying a different
GMainContext. It also returns the GSource directly instead of its ID
number for convenience.

pty_chr_state() and qio_task_thread_worker() are modified to make use
of this new function.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 chardev/char-pty.c       | 8 ++------
 include/qemu/main-loop.h | 8 ++++++++
 io/task.c                | 7 ++-----
 util/main-loop.c         | 9 +++++++++
 4 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index f16a5e8d59..6562c38f39 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -209,13 +209,9 @@ static void pty_chr_state(Chardev *chr, int connected)
         pty_chr_timer_cancel(s);
         if (!s->connected) {
             g_assert(s->open_source == NULL);
-            s->open_source = g_idle_source_new();
+            s->open_source = qemu_idle_add(qemu_chr_be_generic_open_func,
+                                           chr, chr->gcontext);
             s->connected = 1;
-            g_source_set_callback(s->open_source,
-                                  qemu_chr_be_generic_open_func,
-                                  chr, NULL);
-            g_source_attach(s->open_source, chr->gcontext);
-            g_source_unref(s->open_source);
         }
         if (!chr->gsource) {
             chr->gsource = io_add_watch_poll(chr, s->ioc,
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index e59f9ae1e9..1865afd993 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -295,6 +295,14 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line);
  */
 void qemu_mutex_unlock_iothread(void);
 
+/**
+ * qemu_idle_add: Add an idle function to a GMainContext
+ *
+ * This function is similar to GLib's g_idle_add() but it allows
+ * specifying a GMainContext instead of using the global one.
+ */
+GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx);
+
 /* internal interfaces */
 
 void qemu_fd_register(int fd);
diff --git a/io/task.c b/io/task.c
index c8489fb790..bdd395b0ad 100644
--- a/io/task.c
+++ b/io/task.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "io/task.h"
 #include "qapi/error.h"
 #include "qemu/thread.h"
@@ -105,7 +106,6 @@ static gboolean qio_task_thread_result(gpointer opaque)
 static gpointer qio_task_thread_worker(gpointer opaque)
 {
     struct QIOTaskThreadData *data = opaque;
-    GSource *idle;
 
     trace_qio_task_thread_run(data->task);
     data->worker(data->task, data->opaque);
@@ -117,10 +117,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
      */
     trace_qio_task_thread_exit(data->task);
 
-    idle = g_idle_source_new();
-    g_source_set_callback(idle, qio_task_thread_result, data, NULL);
-    g_source_attach(idle, data->context);
-    g_source_unref(idle);
+    qemu_idle_add(qio_task_thread_result, data, data->context);
 
     return NULL;
 }
diff --git a/util/main-loop.c b/util/main-loop.c
index 443cb4cfe8..1c68e3bf5f 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -171,6 +171,15 @@ int qemu_init_main_loop(Error **errp)
     return 0;
 }
 
+GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx)
+{
+    GSource *idle = g_idle_source_new();
+    g_source_set_callback(idle, func, data, NULL);
+    g_source_attach(idle, ctx);
+    g_source_unref(idle);
+    return idle;
+}
+
 static int max_priority;
 
 #ifndef _WIN32
-- 
2.11.0

[Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Alberto Garcia]

There's a race condition in which the tcp_chr_read() ioc handler can
close a connection that is being written to from another thread.

Running iotest 136 in a loop triggers this problem and crashes QEMU.

 (gdb) bt
 #0  0x00005558b842902d in object_get_class (obj=0x0) at qom/object.c:860
 #1  0x00005558b84f92db in qio_channel_writev_full (ioc=0x0, iov=0x7ffc355decf0, niov=1, fds=0x0, nfds=0, errp=0x0) at io/channel.c:76
 #2  0x00005558b84e0e9e in io_channel_send_full (ioc=0x0, buf=0x5558baf5beb0, len=138, fds=0x0, nfds=0) at chardev/char-io.c:123
 #3  0x00005558b84e4a69 in tcp_chr_write (chr=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138) at chardev/char-socket.c:135
 #4  0x00005558b84dca55 in qemu_chr_write_buffer (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, offset=0x7ffc355dedd0, write_all=false) at chardev/char.c:112
 #5  0x00005558b84dcbc2 in qemu_chr_write (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, write_all=false) at chardev/char.c:147
 #6  0x00005558b84dfb26 in qemu_chr_fe_write (be=0x5558ba476610, buf=0x5558baf5beb0 "...", len=138) at chardev/char-fe.c:42
 #7  0x00005558b8088c86 in monitor_flush_locked (mon=0x5558ba476610) at monitor.c:406
 #8  0x00005558b8088e8c in monitor_puts (mon=0x5558ba476610, str=0x5558ba921e49 "") at monitor.c:449
 #9  0x00005558b8089178 in qmp_send_response (mon=0x5558ba476610, rsp=0x5558bb161600) at monitor.c:498
 #10 0x00005558b808920c in monitor_qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:526
 #11 0x00005558b8089307 in monitor_qapi_event_queue_no_reenter (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:551
 #12 0x00005558b80896c0 in qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:626
 #13 0x00005558b855f23b in qapi_event_send_shutdown (guest=false, reason=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at qapi/qapi-events-run-state.c:43
 #14 0x00005558b81911ef in qemu_system_shutdown (cause=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at vl.c:1837
 #15 0x00005558b8191308 in main_loop_should_exit () at vl.c:1885
 #16 0x00005558b819140d in main_loop () at vl.c:1924
 #17 0x00005558b8198c84 in main (argc=18, argv=0x7ffc355df3f8, envp=0x7ffc355df490) at vl.c:4665

This patch adds a lock to protect tcp_chr_disconnect() and
socket_reconnect_timeout()

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 chardev/char-socket.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index eaa8e8b68f..5f421bdc58 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -148,7 +148,9 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
 
         if (ret < 0 && errno != EAGAIN) {
             if (tcp_chr_read_poll(chr) <= 0) {
+                qemu_mutex_unlock(&chr->chr_write_lock);
                 tcp_chr_disconnect(chr);
+                qemu_mutex_lock(&chr->chr_write_lock);
                 return len;
             } /* else let the read handler finish it properly */
         }
@@ -440,6 +442,13 @@ static void update_disconnected_filename(SocketChardev *s)
     }
 }
 
+static gboolean tcp_chr_be_event_closed(gpointer opaque)
+{
+    Chardev *chr = opaque;
+    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+    return FALSE;
+}
+
 /* NB may be called even if tcp_chr_connect has not been
  * reached, due to TLS or telnet initialization failure,
  * so can *not* assume s->connected == true
@@ -447,7 +456,10 @@ static void update_disconnected_filename(SocketChardev *s)
 static void tcp_chr_disconnect(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
-    bool emit_close = s->connected;
+    bool emit_close;
+
+    qemu_mutex_lock(&chr->chr_write_lock);
+    emit_close = s->connected;
 
     tcp_chr_free_connection(chr);
 
@@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
     }
     update_disconnected_filename(s);
     if (emit_close) {
-        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+        qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);
     }
     if (s->reconnect_time) {
         qemu_chr_socket_restart_timer(chr);
     }
+    qemu_mutex_unlock(&chr->chr_write_lock);
 }
 
 static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
@@ -975,8 +988,10 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
     Chardev *chr = CHARDEV(opaque);
     SocketChardev *s = SOCKET_CHARDEV(opaque);
 
+    qemu_mutex_lock(&chr->chr_write_lock);
     g_source_unref(s->reconnect_timer);
     s->reconnect_timer = NULL;
+    qemu_mutex_unlock(&chr->chr_write_lock);
 
     if (chr->be_open) {
         return false;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext

[Alberto Garcia]

On Thu 07 Feb 2019 02:23:21 PM CET, Alberto Garcia wrote:
> After g_source_attach() the GMainContext holds a reference to the
> GSource, so the caller does not need to keep it.
>
> pty_chr_state() and qio_task_thread_worker() are not doing this, so

s/are not doing this/are not releasing their references/

I'll correct this if I need to send a new version of the series,
otherwise please fix when committing this patch.

Berto

Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext

[Daniel P. Berrangé]

On Thu, Feb 07, 2019 at 03:23:21PM +0200, Alberto Garcia wrote:
> After g_source_attach() the GMainContext holds a reference to the
> GSource, so the caller does not need to keep it.
> 
> pty_chr_state() and qio_task_thread_worker() are not doing this, so
> the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> is the exception here because it does remove the extra reference
> correctly).
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  chardev/char-pty.c | 2 +-
>  io/task.c          | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext

[Marc-André Lureau]

On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
>
> After g_source_attach() the GMainContext holds a reference to the
> GSource, so the caller does not need to keep it.
>
> pty_chr_state() and qio_task_thread_worker() are not doing this, so
> the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> is the exception here because it does remove the extra reference
> correctly).
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>

You could mention this is a fix for regression from
a17536c594bfed94d05667b419f747b692f5fc7f


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  chardev/char-pty.c | 2 +-
>  io/task.c          | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index f681d637c1..f16a5e8d59 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -60,7 +60,6 @@ static void pty_chr_open_src_cancel(PtyChardev *s)
>  {
>      if (s->open_source) {
>          g_source_destroy(s->open_source);
> -        g_source_unref(s->open_source);
>          s->open_source = NULL;
>      }
>  }
> @@ -216,6 +215,7 @@ static void pty_chr_state(Chardev *chr, int connected)
>                                    qemu_chr_be_generic_open_func,
>                                    chr, NULL);
>              g_source_attach(s->open_source, chr->gcontext);
> +            g_source_unref(s->open_source);
>          }
>          if (!chr->gsource) {
>              chr->gsource = io_add_watch_poll(chr, s->ioc,
> diff --git a/io/task.c b/io/task.c
> index 2886a2c1bc..c8489fb790 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -120,6 +120,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
>      idle = g_idle_source_new();
>      g_source_set_callback(idle, qio_task_thread_result, data, NULL);
>      g_source_attach(idle, data->context);
> +    g_source_unref(idle);
>
>      return NULL;
>  }
> --
> 2.11.0
>

Re: [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add()

[Daniel P. Berrangé]

On Thu, Feb 07, 2019 at 03:23:22PM +0200, Alberto Garcia wrote:
> This works like g_idle_add() but allows specifying a different
> GMainContext. It also returns the GSource directly instead of its ID
> number for convenience.
> 
> pty_chr_state() and qio_task_thread_worker() are modified to make use
> of this new function.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  chardev/char-pty.c       | 8 ++------
>  include/qemu/main-loop.h | 8 ++++++++
>  io/task.c                | 7 ++-----
>  util/main-loop.c         | 9 +++++++++
>  4 files changed, 21 insertions(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext

[Marc-André Lureau]

On Thu, Feb 7, 2019 at 3:23 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
> >
> > After g_source_attach() the GMainContext holds a reference to the
> > GSource, so the caller does not need to keep it.
> >
> > pty_chr_state() and qio_task_thread_worker() are not doing this, so
> > the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> > is the exception here because it does remove the extra reference
> > correctly).
> >
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> You could mention this is a fix for regression from
> a17536c594bfed94d05667b419f747b692f5fc7f
>

and 938eb9e9c83d34d90cac6ec5c5388e7998840e4e


>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
> > ---
> >  chardev/char-pty.c | 2 +-
> >  io/task.c          | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > index f681d637c1..f16a5e8d59 100644
> > --- a/chardev/char-pty.c
> > +++ b/chardev/char-pty.c
> > @@ -60,7 +60,6 @@ static void pty_chr_open_src_cancel(PtyChardev *s)
> >  {
> >      if (s->open_source) {
> >          g_source_destroy(s->open_source);
> > -        g_source_unref(s->open_source);
> >          s->open_source = NULL;
> >      }
> >  }
> > @@ -216,6 +215,7 @@ static void pty_chr_state(Chardev *chr, int connected)
> >                                    qemu_chr_be_generic_open_func,
> >                                    chr, NULL);
> >              g_source_attach(s->open_source, chr->gcontext);
> > +            g_source_unref(s->open_source);
> >          }
> >          if (!chr->gsource) {
> >              chr->gsource = io_add_watch_poll(chr, s->ioc,
> > diff --git a/io/task.c b/io/task.c
> > index 2886a2c1bc..c8489fb790 100644
> > --- a/io/task.c
> > +++ b/io/task.c
> > @@ -120,6 +120,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
> >      idle = g_idle_source_new();
> >      g_source_set_callback(idle, qio_task_thread_result, data, NULL);
> >      g_source_attach(idle, data->context);
> > +    g_source_unref(idle);
> >
> >      return NULL;
> >  }
> > --
> > 2.11.0
> >

Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Daniel P. Berrangé]

On Thu, Feb 07, 2019 at 03:23:23PM +0200, Alberto Garcia wrote:
> There's a race condition in which the tcp_chr_read() ioc handler can
> close a connection that is being written to from another thread.
> 
> Running iotest 136 in a loop triggers this problem and crashes QEMU.
> 
>  (gdb) bt
>  #0  0x00005558b842902d in object_get_class (obj=0x0) at qom/object.c:860
>  #1  0x00005558b84f92db in qio_channel_writev_full (ioc=0x0, iov=0x7ffc355decf0, niov=1, fds=0x0, nfds=0, errp=0x0) at io/channel.c:76
>  #2  0x00005558b84e0e9e in io_channel_send_full (ioc=0x0, buf=0x5558baf5beb0, len=138, fds=0x0, nfds=0) at chardev/char-io.c:123
>  #3  0x00005558b84e4a69 in tcp_chr_write (chr=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138) at chardev/char-socket.c:135
>  #4  0x00005558b84dca55 in qemu_chr_write_buffer (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, offset=0x7ffc355dedd0, write_all=false) at chardev/char.c:112
>  #5  0x00005558b84dcbc2 in qemu_chr_write (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, write_all=false) at chardev/char.c:147
>  #6  0x00005558b84dfb26 in qemu_chr_fe_write (be=0x5558ba476610, buf=0x5558baf5beb0 "...", len=138) at chardev/char-fe.c:42
>  #7  0x00005558b8088c86 in monitor_flush_locked (mon=0x5558ba476610) at monitor.c:406
>  #8  0x00005558b8088e8c in monitor_puts (mon=0x5558ba476610, str=0x5558ba921e49 "") at monitor.c:449
>  #9  0x00005558b8089178 in qmp_send_response (mon=0x5558ba476610, rsp=0x5558bb161600) at monitor.c:498
>  #10 0x00005558b808920c in monitor_qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:526
>  #11 0x00005558b8089307 in monitor_qapi_event_queue_no_reenter (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:551
>  #12 0x00005558b80896c0 in qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:626
>  #13 0x00005558b855f23b in qapi_event_send_shutdown (guest=false, reason=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at qapi/qapi-events-run-state.c:43
>  #14 0x00005558b81911ef in qemu_system_shutdown (cause=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at vl.c:1837
>  #15 0x00005558b8191308 in main_loop_should_exit () at vl.c:1885
>  #16 0x00005558b819140d in main_loop () at vl.c:1924
>  #17 0x00005558b8198c84 in main (argc=18, argv=0x7ffc355df3f8, envp=0x7ffc355df490) at vl.c:4665
> 
> This patch adds a lock to protect tcp_chr_disconnect() and
> socket_reconnect_timeout()
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  chardev/char-socket.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index eaa8e8b68f..5f421bdc58 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -148,7 +148,9 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>  
>          if (ret < 0 && errno != EAGAIN) {
>              if (tcp_chr_read_poll(chr) <= 0) {
> +                qemu_mutex_unlock(&chr->chr_write_lock);
>                  tcp_chr_disconnect(chr);
> +                qemu_mutex_lock(&chr->chr_write_lock);
>                  return len;
>              } /* else let the read handler finish it properly */
>          }
> @@ -440,6 +442,13 @@ static void update_disconnected_filename(SocketChardev *s)
>      }
>  }
>  
> +static gboolean tcp_chr_be_event_closed(gpointer opaque)
> +{
> +    Chardev *chr = opaque;
> +    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +    return FALSE;
> +}
> +
>  /* NB may be called even if tcp_chr_connect has not been
>   * reached, due to TLS or telnet initialization failure,
>   * so can *not* assume s->connected == true
> @@ -447,7 +456,10 @@ static void update_disconnected_filename(SocketChardev *s)
>  static void tcp_chr_disconnect(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> -    bool emit_close = s->connected;
> +    bool emit_close;
> +
> +    qemu_mutex_lock(&chr->chr_write_lock);
> +    emit_close = s->connected;
>  
>      tcp_chr_free_connection(chr);
>  
> @@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
>      }
>      update_disconnected_filename(s);
>      if (emit_close) {
> -        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +        qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);

I'm a little concerned that this change might trigger some unexpected
interactions with the qemu_chr_wait_connected() function. I added some
more testing around that code, so if it passes the new tests I have in:

  https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05962.html

and also works with the vhostuser reconnect device, then we are probably
ok.

Assuming it passes the test above:

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


>      }
>      if (s->reconnect_time) {
>          qemu_chr_socket_restart_timer(chr);
>      }
> +    qemu_mutex_unlock(&chr->chr_write_lock);
>  }
>  
>  static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
> @@ -975,8 +988,10 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
>      Chardev *chr = CHARDEV(opaque);
>      SocketChardev *s = SOCKET_CHARDEV(opaque);
>  
> +    qemu_mutex_lock(&chr->chr_write_lock);
>      g_source_unref(s->reconnect_timer);
>      s->reconnect_timer = NULL;
> +    qemu_mutex_unlock(&chr->chr_write_lock);
>  
>      if (chr->be_open) {
>          return false;

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext

[Alberto Garcia]

On Thu 07 Feb 2019 03:24:40 PM CET, Marc-André Lureau wrote:
> On Thu, Feb 7, 2019 at 3:23 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
>> >
>> > After g_source_attach() the GMainContext holds a reference to the
>> > GSource, so the caller does not need to keep it.
>> >
>> > pty_chr_state() and qio_task_thread_worker() are not doing this, so
>> > the GSource is being leaked in both cases (pty_chr_open_src_cancel()
>> > is the exception here because it does remove the extra reference
>> > correctly).
>> >
>> > Signed-off-by: Alberto Garcia <berto@igalia.com>
>>
>> You could mention this is a fix for regression from
>> a17536c594bfed94d05667b419f747b692f5fc7f
>
> and 938eb9e9c83d34d90cac6ec5c5388e7998840e4e

Ok, feel free to add this to the commit message if I don't need to
resend the series.

Berto

Re: [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add()

[Marc-André Lureau]

Hi

On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
>
> This works like g_idle_add() but allows specifying a different
> GMainContext. It also returns the GSource directly instead of its ID
> number for convenience.
>
> pty_chr_state() and qio_task_thread_worker() are modified to make use
> of this new function.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  chardev/char-pty.c       | 8 ++------
>  include/qemu/main-loop.h | 8 ++++++++
>  io/task.c                | 7 ++-----
>  util/main-loop.c         | 9 +++++++++
>  4 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index f16a5e8d59..6562c38f39 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -209,13 +209,9 @@ static void pty_chr_state(Chardev *chr, int connected)
>          pty_chr_timer_cancel(s);
>          if (!s->connected) {
>              g_assert(s->open_source == NULL);
> -            s->open_source = g_idle_source_new();
> +            s->open_source = qemu_idle_add(qemu_chr_be_generic_open_func,
> +                                           chr, chr->gcontext);
>              s->connected = 1;
> -            g_source_set_callback(s->open_source,
> -                                  qemu_chr_be_generic_open_func,
> -                                  chr, NULL);
> -            g_source_attach(s->open_source, chr->gcontext);
> -            g_source_unref(s->open_source);
>          }
>          if (!chr->gsource) {
>              chr->gsource = io_add_watch_poll(chr, s->ioc,
> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> index e59f9ae1e9..1865afd993 100644
> --- a/include/qemu/main-loop.h
> +++ b/include/qemu/main-loop.h
> @@ -295,6 +295,14 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line);
>   */
>  void qemu_mutex_unlock_iothread(void);
>
> +/**
> + * qemu_idle_add: Add an idle function to a GMainContext
> + *
> + * This function is similar to GLib's g_idle_add() but it allows
> + * specifying a GMainContext instead of using the global one.
> + */
> +GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx);

I think you should document that returned GSource* isn't ref/owned.

> +
>  /* internal interfaces */
>
>  void qemu_fd_register(int fd);
> diff --git a/io/task.c b/io/task.c
> index c8489fb790..bdd395b0ad 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -19,6 +19,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "io/task.h"
>  #include "qapi/error.h"
>  #include "qemu/thread.h"
> @@ -105,7 +106,6 @@ static gboolean qio_task_thread_result(gpointer opaque)
>  static gpointer qio_task_thread_worker(gpointer opaque)
>  {
>      struct QIOTaskThreadData *data = opaque;
> -    GSource *idle;
>
>      trace_qio_task_thread_run(data->task);
>      data->worker(data->task, data->opaque);
> @@ -117,10 +117,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
>       */
>      trace_qio_task_thread_exit(data->task);
>
> -    idle = g_idle_source_new();
> -    g_source_set_callback(idle, qio_task_thread_result, data, NULL);
> -    g_source_attach(idle, data->context);
> -    g_source_unref(idle);
> +    qemu_idle_add(qio_task_thread_result, data, data->context);
>
>      return NULL;
>  }
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 443cb4cfe8..1c68e3bf5f 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -171,6 +171,15 @@ int qemu_init_main_loop(Error **errp)
>      return 0;
>  }
>
> +GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx)
> +{
> +    GSource *idle = g_idle_source_new();
> +    g_source_set_callback(idle, func, data, NULL);
> +    g_source_attach(idle, ctx);
> +    g_source_unref(idle);
> +    return idle;
> +}
> +
>  static int max_priority;
>
>  #ifndef _WIN32
> --
> 2.11.0
>

looks ok otherwise
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext

[Marc-André Lureau]

Hi

On Thu, Feb 7, 2019 at 3:37 PM Alberto Garcia <berto@igalia.com> wrote:
>
> On Thu 07 Feb 2019 03:24:40 PM CET, Marc-André Lureau wrote:
> > On Thu, Feb 7, 2019 at 3:23 PM Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> >>
> >> On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
> >> >
> >> > After g_source_attach() the GMainContext holds a reference to the
> >> > GSource, so the caller does not need to keep it.
> >> >
> >> > pty_chr_state() and qio_task_thread_worker() are not doing this, so
> >> > the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> >> > is the exception here because it does remove the extra reference
> >> > correctly).
> >> >
> >> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> >>
> >> You could mention this is a fix for regression from
> >> a17536c594bfed94d05667b419f747b692f5fc7f
> >
> > and 938eb9e9c83d34d90cac6ec5c5388e7998840e4e
>
> Ok, feel free to add this to the commit message if I don't need to
> resend the series.

Well, the series conflicts with the currently queued chardev series. I
think I will send the pullreq and let you rebase.

(I also have a related series pending review "[PATCH v2 0/6] chardev:
various cleanups and improvements")


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Marc-André Lureau]

Hi

On Thu, Feb 7, 2019 at 2:25 PM Alberto Garcia <berto@igalia.com> wrote:
>
> There's a race condition in which the tcp_chr_read() ioc handler can
> close a connection that is being written to from another thread.
>
> Running iotest 136 in a loop triggers this problem and crashes QEMU.
>
>  (gdb) bt
>  #0  0x00005558b842902d in object_get_class (obj=0x0) at qom/object.c:860
>  #1  0x00005558b84f92db in qio_channel_writev_full (ioc=0x0, iov=0x7ffc355decf0, niov=1, fds=0x0, nfds=0, errp=0x0) at io/channel.c:76
>  #2  0x00005558b84e0e9e in io_channel_send_full (ioc=0x0, buf=0x5558baf5beb0, len=138, fds=0x0, nfds=0) at chardev/char-io.c:123
>  #3  0x00005558b84e4a69 in tcp_chr_write (chr=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138) at chardev/char-socket.c:135
>  #4  0x00005558b84dca55 in qemu_chr_write_buffer (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, offset=0x7ffc355dedd0, write_all=false) at chardev/char.c:112
>  #5  0x00005558b84dcbc2 in qemu_chr_write (s=0x5558ba460380, buf=0x5558baf5beb0 "...", len=138, write_all=false) at chardev/char.c:147
>  #6  0x00005558b84dfb26 in qemu_chr_fe_write (be=0x5558ba476610, buf=0x5558baf5beb0 "...", len=138) at chardev/char-fe.c:42
>  #7  0x00005558b8088c86 in monitor_flush_locked (mon=0x5558ba476610) at monitor.c:406
>  #8  0x00005558b8088e8c in monitor_puts (mon=0x5558ba476610, str=0x5558ba921e49 "") at monitor.c:449
>  #9  0x00005558b8089178 in qmp_send_response (mon=0x5558ba476610, rsp=0x5558bb161600) at monitor.c:498
>  #10 0x00005558b808920c in monitor_qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:526
>  #11 0x00005558b8089307 in monitor_qapi_event_queue_no_reenter (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:551
>  #12 0x00005558b80896c0 in qapi_event_emit (event=QAPI_EVENT_SHUTDOWN, qdict=0x5558bb161600) at monitor.c:626
>  #13 0x00005558b855f23b in qapi_event_send_shutdown (guest=false, reason=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at qapi/qapi-events-run-state.c:43
>  #14 0x00005558b81911ef in qemu_system_shutdown (cause=SHUTDOWN_CAUSE_HOST_QMP_QUIT) at vl.c:1837
>  #15 0x00005558b8191308 in main_loop_should_exit () at vl.c:1885
>  #16 0x00005558b819140d in main_loop () at vl.c:1924
>  #17 0x00005558b8198c84 in main (argc=18, argv=0x7ffc355df3f8, envp=0x7ffc355df490) at vl.c:4665
>
> This patch adds a lock to protect tcp_chr_disconnect() and
> socket_reconnect_timeout()
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>

That looks like a subset of "[Qemu-devel] Segfaults in chardev due to
races". The problem is likely to occur with other chardevs I am
afraid.


> ---
>  chardev/char-socket.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index eaa8e8b68f..5f421bdc58 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -148,7 +148,9 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>
>          if (ret < 0 && errno != EAGAIN) {
>              if (tcp_chr_read_poll(chr) <= 0) {
> +                qemu_mutex_unlock(&chr->chr_write_lock);
>                  tcp_chr_disconnect(chr);
> +                qemu_mutex_lock(&chr->chr_write_lock);
>                  return len;
>              } /* else let the read handler finish it properly */
>          }
> @@ -440,6 +442,13 @@ static void update_disconnected_filename(SocketChardev *s)
>      }
>  }
>
> +static gboolean tcp_chr_be_event_closed(gpointer opaque)
> +{
> +    Chardev *chr = opaque;
> +    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +    return FALSE;
> +}
> +
>  /* NB may be called even if tcp_chr_connect has not been
>   * reached, due to TLS or telnet initialization failure,
>   * so can *not* assume s->connected == true
> @@ -447,7 +456,10 @@ static void update_disconnected_filename(SocketChardev *s)
>  static void tcp_chr_disconnect(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> -    bool emit_close = s->connected;
> +    bool emit_close;
> +
> +    qemu_mutex_lock(&chr->chr_write_lock);
> +    emit_close = s->connected;
>
>      tcp_chr_free_connection(chr);
>
> @@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
>      }
>      update_disconnected_filename(s);
>      if (emit_close) {
> -        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +        qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);
>      }
>      if (s->reconnect_time) {
>          qemu_chr_socket_restart_timer(chr);
>      }
> +    qemu_mutex_unlock(&chr->chr_write_lock);
>  }
>
>  static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
> @@ -975,8 +988,10 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
>      Chardev *chr = CHARDEV(opaque);
>      SocketChardev *s = SOCKET_CHARDEV(opaque);
>
> +    qemu_mutex_lock(&chr->chr_write_lock);
>      g_source_unref(s->reconnect_timer);
>      s->reconnect_timer = NULL;
> +    qemu_mutex_unlock(&chr->chr_write_lock);
>
>      if (chr->be_open) {
>          return false;
> --
> 2.11.0
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/3] main-loop: Don't leak GSources attached to a GMainContext

[Alberto Garcia]

On Thu 07 Feb 2019 03:43:59 PM CET, Marc-André Lureau wrote:

> Well, the series conflicts with the currently queued chardev series. I
> think I will send the pullreq and let you rebase.

I'll wait for it then

Berto

Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Paolo Bonzini]

On 07/02/19 15:46, Marc-André Lureau wrote:
> That looks like a subset of "[Qemu-devel] Segfaults in chardev due to
> races". The problem is likely to occur with other chardevs I am
> afraid.

Yes, it could possibly happen with pty at least.  However, Alberto's
patch is a start.

Paolo

Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Alberto Garcia]

On Thu 07 Feb 2019 03:26:49 PM CET, Daniel P. Berrangé wrote:
>> @@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
>>      }
>>      update_disconnected_filename(s);
>>      if (emit_close) {
>> -        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>> +        qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);
>
> I'm a little concerned that this change might trigger some unexpected
> interactions with the qemu_chr_wait_connected() function. I added some
> more testing around that code, so if it passes the new tests I have
> in:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05962.html
>
> and also works with the vhostuser reconnect device, then we are
> probably ok.

Seems that it doesn't ...

/char/null: OK
/char/invalid: OK
/char/ringbuf: OK
/char/mux: OK
/char/stdio: OK
/char/pipe: OK
/char/file: OK
/char/file-fifo: OK
/char/udp: OK
/char/serial: OK
/char/hotswap: OK
/char/websocket: OK
/char/socket/server/mainloop/tcp: OK
/char/socket/server/mainloop/unix: OK
/char/socket/server/wait-conn/tcp: OK
/char/socket/server/wait-conn/unix: OK
/char/socket/server/mainloop-fdpass/tcp: OK
/char/socket/server/mainloop-fdpass/unix: OK
/char/socket/server/wait-conn-fdpass/tcp: OK
/char/socket/server/wait-conn-fdpass/unix: OK
/char/socket/client/mainloop/tcp: OK
/char/socket/client/mainloop/unix: OK
/char/socket/client/wait-conn/tcp: OK
/char/socket/client/wait-conn/unix: OK
/char/socket/client/mainloop-reconnect/tcp: OK
/char/socket/client/mainloop-reconnect/unix: OK
/char/socket/client/wait-conn-reconnect/tcp: Unexpected error in qio_channel_readv_all() at /home/berto/work/qemu/qemu/io/channel.c:147:
Unexpected end-of-file before all bytes were read

Berto

Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Alberto Garcia]

On Thu 07 Feb 2019 03:26:49 PM CET, Daniel P. Berrangé wrote:
>> @@ -457,11 +469,12 @@ static void tcp_chr_disconnect(Chardev *chr)
>>      }
>>      update_disconnected_filename(s);
>>      if (emit_close) {
>> -        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>> +        qemu_idle_add(tcp_chr_be_event_closed, chr, chr->gcontext);
>
> I'm a little concerned that this change might trigger some unexpected
> interactions with the qemu_chr_wait_connected() function. I added some
> more testing around that code, so if it passes the new tests I have in:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05962.html

Forget my previous e-mail, there was a mistake on my side. The test does
pass after rebasing my series on top of that one.

> and also works with the vhostuser reconnect device, then we are
> probably ok.

How do I test this?

Berto

Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Paolo Bonzini]

On 08/02/19 10:34, Alberto Garcia wrote:
>> and also works with the vhostuser reconnect device, then we are
>> probably ok.
> How do I test this?

QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 \
  QTEST_VHOST_USER_FIXME=1 \
  tests/vhost-user-test

Paolo

Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Alberto Garcia]

On Fri 08 Feb 2019 02:46:27 PM CET, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 08/02/19 10:34, Alberto Garcia wrote:
>>> and also works with the vhostuser reconnect device, then we are
>>> probably ok.
>> How do I test this?
>
> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 \
>   QTEST_VHOST_USER_FIXME=1 \
>   tests/vhost-user-test

I see. The ones enabled by QTEST_VHOST_USER_FIXME never finish with this
patch applied.

Berto

Re: [Qemu-devel] [PATCH 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Paolo Bonzini]

On 08/02/19 16:01, Alberto Garcia wrote:
> On Fri 08 Feb 2019 02:46:27 PM CET, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 08/02/19 10:34, Alberto Garcia wrote:
>>>> and also works with the vhostuser reconnect device, then we are
>>>> probably ok.
>>> How do I test this?
>>
>> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 \
>>   QTEST_VHOST_USER_FIXME=1 \
>>   tests/vhost-user-test
> 
> I see. The ones enabled by QTEST_VHOST_USER_FIXME never finish with this
> patch applied.

I can look at it next week.

Paolo