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

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

[Alberto Garcia]

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

Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.

Berto

RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html

v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
- Fixes memory leaks and adds a qemu_idle_add() function

v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
- Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
- Patches 1 and 2: Remove the changes in char-pty.c, they're not
                   needed after the rebase.
- Patch 3: Fix conflicts after the rebase.

v3:
- Patch 3: Add tcp_chr_disconnect_locked() [Daniel]

Alberto Garcia (3):
  main-loop: Fix GSource leak in qio_task_thread_worker()
  main-loop: Add qemu_idle_add()
  char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

 chardev/char-socket.c    | 25 +++++++++++++++++++++----
 include/qemu/main-loop.h | 12 ++++++++++++
 io/task.c                |  9 +++------
 util/main-loop.c         |  9 +++++++++
 4 files changed, 45 insertions(+), 10 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v3 1/3] main-loop: Fix GSource leak in qio_task_thread_worker()

[Alberto Garcia]

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

qio_task_thread_worker() is not releasing its reference so the GSource
is being leaked since a17536c594bfed94d05667b419f747b692f5fc7f.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/task.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/io/task.c b/io/task.c
index 64c4c7126a..1ae7b86488 100644
--- a/io/task.c
+++ b/io/task.c
@@ -136,6 +136,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
                           qio_task_thread_result, task, NULL);
     g_source_attach(task->thread->completion,
                     task->thread->context);
+    g_source_unref(task->thread->completion);
     trace_qio_task_thread_source_attach(task, task->thread->completion);
 
     qemu_cond_signal(&task->thread_cond);
-- 
2.11.0

[Qemu-devel] [PATCH v3 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.

qio_task_thread_worker() is modified to make use of this new function.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/main-loop.h | 12 ++++++++++++
 io/task.c                | 10 +++-------
 util/main-loop.c         |  9 +++++++++
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index f6ba78ea73..f966c015d0 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -295,6 +295,18 @@ 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.
+ *
+ * The returned GSource is owned by the GMainContext and is deleted
+ * automatically after @func returns false. It can also be deleted by
+ * removing it from the GMainContext using g_source_destroy().
+ */
+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 1ae7b86488..16754e22ba 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"
@@ -130,13 +131,8 @@ static gpointer qio_task_thread_worker(gpointer opaque)
     trace_qio_task_thread_exit(task);
 
     qemu_mutex_lock(&task->thread_lock);
-
-    task->thread->completion = g_idle_source_new();
-    g_source_set_callback(task->thread->completion,
-                          qio_task_thread_result, task, NULL);
-    g_source_attach(task->thread->completion,
-                    task->thread->context);
-    g_source_unref(task->thread->completion);
+    task->thread->completion = qemu_idle_add(qio_task_thread_result, task,
+                                             task->thread->context);
     trace_qio_task_thread_source_attach(task, task->thread->completion);
 
     qemu_cond_signal(&task->thread_cond);
diff --git a/util/main-loop.c b/util/main-loop.c
index d4a521caeb..e23eda68cc 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 v3 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 | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 4fcdd8aedd..94522777dc 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -148,7 +148,7 @@ static void tcp_chr_accept(QIONetListener *listener,
                            void *opaque);
 
 static int tcp_chr_read_poll(void *opaque);
-static void tcp_chr_disconnect(Chardev *chr);
+static void tcp_chr_disconnect_locked(Chardev *chr);
 
 /* Called with chr_write_lock held.  */
 static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
@@ -172,7 +172,7 @@ 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) {
-                tcp_chr_disconnect(chr);
+                tcp_chr_disconnect_locked(chr);
                 return len;
             } /* else let the read handler finish it properly */
         }
@@ -464,11 +464,19 @@ 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->state == TCP_CHARDEV_STATE_CONNECTED
+ * This must be called with chr->chr_write_lock held.
  */
-static void tcp_chr_disconnect(Chardev *chr)
+static void tcp_chr_disconnect_locked(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
     bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
@@ -481,13 +489,20 @@ 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);
     }
 }
 
+static void tcp_chr_disconnect(Chardev *chr)
+{
+    qemu_mutex_lock(&chr->chr_write_lock);
+    tcp_chr_disconnect_locked(chr);
+    qemu_mutex_unlock(&chr->chr_write_lock);
+}
+
 static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
@@ -1128,8 +1143,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 v3 3/3] char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

[Daniel P. Berrangé]

On Fri, Feb 22, 2019 at 03:46:26PM +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 | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 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 v3 0/3] char-socket: Fix race condition

[Alberto Garcia]

ping

On Fri 22 Feb 2019 02:46:23 PM CET, Alberto Garcia wrote:
> This fixes a race condition in which the tcp_chr_read() ioc handler
> can close a connection that is being written to from another thread.
>
> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
>
> Berto

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

[Marc-André Lureau]

Hi

On Tue, Mar 5, 2019 at 4:52 PM Alberto Garcia <berto@igalia.com> wrote:
>
> ping

I can queue this in the next chardev PR. This can probably wait until
the softfreeze though. I think Paolo had some plans to look at chardev
issues.

>
> On Fri 22 Feb 2019 02:46:23 PM CET, Alberto Garcia wrote:
> > This fixes a race condition in which the tcp_chr_read() ioc handler
> > can close a connection that is being written to from another thread.
> >
> > Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
> >
> > Berto

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

[Paolo Bonzini]

On 05/03/19 16:55, Marc-André Lureau wrote:
>> ping
> I can queue this in the next chardev PR. This can probably wait until
> the softfreeze though. I think Paolo had some plans to look at chardev
> issues.
> 

Yes, I still do.  This week I'm a bit busy with Linux and babysitting...

Paolo

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

[Daniel P. Berrangé]

ping - paolo/marc-andré - unless I'm missing something, it looks like
this chardev series slipped through the cracks and missed 4.0

On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
> This fixes a race condition in which the tcp_chr_read() ioc handler
> can close a connection that is being written to from another thread.
> 
> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
> 
> Berto
> 
> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
> 
> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
> - Fixes memory leaks and adds a qemu_idle_add() function
> 
> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>                    needed after the rebase.
> - Patch 3: Fix conflicts after the rebase.
> 
> v3:
> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
> 
> Alberto Garcia (3):
>   main-loop: Fix GSource leak in qio_task_thread_worker()
>   main-loop: Add qemu_idle_add()
>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
> 
>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>  include/qemu/main-loop.h | 12 ++++++++++++
>  io/task.c                |  9 +++------
>  util/main-loop.c         |  9 +++++++++
>  4 files changed, 45 insertions(+), 10 deletions(-)
> 
> -- 
> 2.11.0
> 

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 v3 0/3] char-socket: Fix race condition

[Daniel P. Berrangé]

ping - paolo/marc-andré - unless I'm missing something, it looks like
this chardev series slipped through the cracks and missed 4.0

On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
> This fixes a race condition in which the tcp_chr_read() ioc handler
> can close a connection that is being written to from another thread.
> 
> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
> 
> Berto
> 
> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
> 
> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
> - Fixes memory leaks and adds a qemu_idle_add() function
> 
> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>                    needed after the rebase.
> - Patch 3: Fix conflicts after the rebase.
> 
> v3:
> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
> 
> Alberto Garcia (3):
>   main-loop: Fix GSource leak in qio_task_thread_worker()
>   main-loop: Add qemu_idle_add()
>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
> 
>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>  include/qemu/main-loop.h | 12 ++++++++++++
>  io/task.c                |  9 +++------
>  util/main-loop.c         |  9 +++++++++
>  4 files changed, 45 insertions(+), 10 deletions(-)
> 
> -- 
> 2.11.0
> 

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 v3 0/3] char-socket: Fix race condition

[Paolo Bonzini]

On 23/04/19 18:55, Daniel P. Berrangé wrote:
> ping - paolo/marc-andré - unless I'm missing something, it looks like
> this chardev series slipped through the cracks and missed 4.0

Yeah, it had a bug unfortunately.  I'm looking at it RSN.

Paolo

> 
> On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
>> This fixes a race condition in which the tcp_chr_read() ioc handler
>> can close a connection that is being written to from another thread.
>>
>> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
>>
>> Berto
>>
>> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
>>
>> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
>> - Fixes memory leaks and adds a qemu_idle_add() function
>>
>> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
>> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
>> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>>                    needed after the rebase.
>> - Patch 3: Fix conflicts after the rebase.
>>
>> v3:
>> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
>>
>> Alberto Garcia (3):
>>   main-loop: Fix GSource leak in qio_task_thread_worker()
>>   main-loop: Add qemu_idle_add()
>>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
>>
>>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>>  include/qemu/main-loop.h | 12 ++++++++++++
>>  io/task.c                |  9 +++------
>>  util/main-loop.c         |  9 +++++++++
>>  4 files changed, 45 insertions(+), 10 deletions(-)
>>
>> -- 
>> 2.11.0
>>
> 
> Regards,
> Daniel
> 

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

[Paolo Bonzini]

On 23/04/19 18:55, Daniel P. Berrangé wrote:
> ping - paolo/marc-andré - unless I'm missing something, it looks like
> this chardev series slipped through the cracks and missed 4.0

Yeah, it had a bug unfortunately.  I'm looking at it RSN.

Paolo

> 
> On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
>> This fixes a race condition in which the tcp_chr_read() ioc handler
>> can close a connection that is being written to from another thread.
>>
>> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
>>
>> Berto
>>
>> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
>>
>> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
>> - Fixes memory leaks and adds a qemu_idle_add() function
>>
>> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
>> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
>> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>>                    needed after the rebase.
>> - Patch 3: Fix conflicts after the rebase.
>>
>> v3:
>> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
>>
>> Alberto Garcia (3):
>>   main-loop: Fix GSource leak in qio_task_thread_worker()
>>   main-loop: Add qemu_idle_add()
>>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
>>
>>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>>  include/qemu/main-loop.h | 12 ++++++++++++
>>  io/task.c                |  9 +++------
>>  util/main-loop.c         |  9 +++++++++
>>  4 files changed, 45 insertions(+), 10 deletions(-)
>>
>> -- 
>> 2.11.0
>>
> 
> Regards,
> Daniel
> 

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1838 bytes --]

On 26.04.19 09:51, Paolo Bonzini wrote:
> On 23/04/19 18:55, Daniel P. Berrangé wrote:
>> ping - paolo/marc-andré - unless I'm missing something, it looks like
>> this chardev series slipped through the cracks and missed 4.0
> 
> Yeah, it had a bug unfortunately.  I'm looking at it RSN.

I’ll just leave another ping here

> Paolo
> 
>>
>> On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
>>> This fixes a race condition in which the tcp_chr_read() ioc handler
>>> can close a connection that is being written to from another thread.
>>>
>>> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
>>>
>>> Berto
>>>
>>> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
>>>
>>> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
>>> - Fixes memory leaks and adds a qemu_idle_add() function
>>>
>>> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
>>> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
>>> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>>>                    needed after the rebase.
>>> - Patch 3: Fix conflicts after the rebase.
>>>
>>> v3:
>>> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
>>>
>>> Alberto Garcia (3):
>>>   main-loop: Fix GSource leak in qio_task_thread_worker()
>>>   main-loop: Add qemu_idle_add()
>>>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
>>>
>>>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>>>  include/qemu/main-loop.h | 12 ++++++++++++
>>>  io/task.c                |  9 +++------
>>>  util/main-loop.c         |  9 +++++++++
>>>  4 files changed, 45 insertions(+), 10 deletions(-)
>>>
>>> -- 
>>> 2.11.0
>>>
>>
>> Regards,
>> Daniel
>>
> 
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1962 bytes --]

On 05.06.19 21:36, Max Reitz wrote:
> On 26.04.19 09:51, Paolo Bonzini wrote:
>> On 23/04/19 18:55, Daniel P. Berrangé wrote:
>>> ping - paolo/marc-andré - unless I'm missing something, it looks like
>>> this chardev series slipped through the cracks and missed 4.0
>>
>> Yeah, it had a bug unfortunately.  I'm looking at it RSN.
> 
> I’ll just leave another ping here

And another one.

Max

>> Paolo
>>
>>>
>>> On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
>>>> This fixes a race condition in which the tcp_chr_read() ioc handler
>>>> can close a connection that is being written to from another thread.
>>>>
>>>> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
>>>>
>>>> Berto
>>>>
>>>> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
>>>>
>>>> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
>>>> - Fixes memory leaks and adds a qemu_idle_add() function
>>>>
>>>> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
>>>> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
>>>> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>>>>                    needed after the rebase.
>>>> - Patch 3: Fix conflicts after the rebase.
>>>>
>>>> v3:
>>>> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
>>>>
>>>> Alberto Garcia (3):
>>>>   main-loop: Fix GSource leak in qio_task_thread_worker()
>>>>   main-loop: Add qemu_idle_add()
>>>>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
>>>>
>>>>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>>>>  include/qemu/main-loop.h | 12 ++++++++++++
>>>>  io/task.c                |  9 +++------
>>>>  util/main-loop.c         |  9 +++++++++
>>>>  4 files changed, 45 insertions(+), 10 deletions(-)
>>>>
>>>> -- 
>>>> 2.11.0
>>>>
>>>
>>> Regards,
>>> Daniel
>>>
>>
>>
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Paolo Bonzini]

On 03/07/19 19:51, Max Reitz wrote:
> On 05.06.19 21:36, Max Reitz wrote:
>> On 26.04.19 09:51, Paolo Bonzini wrote:
>>> On 23/04/19 18:55, Daniel P. Berrangé wrote:
>>>> ping - paolo/marc-andré - unless I'm missing something, it looks like
>>>> this chardev series slipped through the cracks and missed 4.0
>>>
>>> Yeah, it had a bug unfortunately.  I'm looking at it RSN.
>>
>> I’ll just leave another ping here

I haven't forgotten. :(

Paolo

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 7702 bytes --]

On 22.02.19 14:46, 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 | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 4fcdd8aedd..94522777dc 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c

[...]

> @@ -464,11 +464,19 @@ 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->state == TCP_CHARDEV_STATE_CONNECTED
> + * This must be called with chr->chr_write_lock held.
>   */
> -static void tcp_chr_disconnect(Chardev *chr)
> +static void tcp_chr_disconnect_locked(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>      bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
> @@ -481,13 +489,20 @@ 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);

How does this guarantee that *chr is still valid when the callback is
invoked?

If I run tests/char-test, it fails.  With a bit of debugging output, it
looks like the the callback is scheduled with this function, then the
chardev is finalized, and only then is the callback invoked, accessing a
dangling pointer.

Or just with valgrind:

> /char/socket/server/mainloop/tcp: ==2114== Invalid write of size 4
> ==2114==    at 0x139F86: qemu_chr_be_event (char.c:70)
> ==2114==    by 0x13FE3D: tcp_chr_be_event_closed (char-socket.c:472)
> ==2114==    by 0x48ED7DA: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x48F0EDC: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x158DC7: glib_pollfds_poll (main-loop.c:222)
> ==2114==    by 0x158DC7: os_host_main_loop_wait (main-loop.c:245)
> ==2114==    by 0x158DC7: main_loop_wait (main-loop.c:521)
> ==2114==    by 0x11EA06: char_socket_server_test (test-char.c:808)
> ==2114==    by 0x4918F9D: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x4918D43: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x4918D43: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x4918D43: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x4918D43: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x4919489: g_test_run_suite (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==  Address 0x5694434 is 116 bytes inside a block of size 312 free'd
> ==2114==    at 0x4839A0C: free (vg_replace_malloc.c:540)
> ==2114==    by 0x48F6D8C: g_free (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x1360E9: object_finalize (object.c:620)
> ==2114==    by 0x1360E9: object_unref (object.c:1070)
> ==2114==    by 0x134EC4: object_property_del_child.isra.0 (object.c:576)
> ==2114==    by 0x11F1EF: char_websock_test (test-char.c:461)
> ==2114==    by 0x4918F9D: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x4918D43: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x4919489: g_test_run_suite (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x49194A4: g_test_run (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x11A249: main (test-char.c:1459)
> ==2114==  Block was alloc'd at
> ==2114==    at 0x483880B: malloc (vg_replace_malloc.c:309)
> ==2114==    by 0x48F6C98: g_malloc (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x1355D0: object_new_with_type (object.c:631)
> ==2114==    by 0x13AEEA: qemu_chardev_new (char.c:959)
> ==2114==    by 0x13B227: qemu_chr_new_from_opts (char.c:680)
> ==2114==    by 0x13B3B3: qemu_chr_new_noreplay (char.c:726)
> ==2114==    by 0x13B469: qemu_chr_new_permit_mux_mon (char.c:748)
> ==2114==    by 0x11EF17: char_websock_test (test-char.c:415)
> ==2114==    by 0x4918F9D: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x4918D43: ??? (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x4919489: g_test_run_suite (in /usr/lib64/libglib-2.0.so.0.6000.4)
> ==2114==    by 0x49194A4: g_test_run (in /usr/lib64/libglib-2.0.so.0.6000.4)

(plus more of the same.)

Now I don’t know whether the test is just wrong and in normal use, the
callback would always be invoked before the object is destroyed (i.e.
the test just needs to poll or something).  But in any case, it is fishy
enough that there should be an explanatory comment at least.

Max

>      }
>      if (s->reconnect_time) {
>          qemu_chr_socket_restart_timer(chr);
>      }
>  }
>  
> +static void tcp_chr_disconnect(Chardev *chr)
> +{
> +    qemu_mutex_lock(&chr->chr_write_lock);
> +    tcp_chr_disconnect_locked(chr);
> +    qemu_mutex_unlock(&chr->chr_write_lock);
> +}
> +
>  static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>  {
>      Chardev *chr = CHARDEV(opaque);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andrey Shinkevich]

On 22/02/2019 16:46, Alberto Garcia wrote:
> This fixes a race condition in which the tcp_chr_read() ioc handler
> can close a connection that is being written to from another thread.
> 
> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
> 
> Berto
> 
> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
> 
> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
> - Fixes memory leaks and adds a qemu_idle_add() function
> 
> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>                     needed after the rebase.
> - Patch 3: Fix conflicts after the rebase.
> 
> v3:
> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
> 
> Alberto Garcia (3):
>    main-loop: Fix GSource leak in qio_task_thread_worker()
>    main-loop: Add qemu_idle_add()
>    char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
> 
>   chardev/char-socket.c    | 25 +++++++++++++++++++++----
>   include/qemu/main-loop.h | 12 ++++++++++++
>   io/task.c                |  9 +++------
>   util/main-loop.c         |  9 +++++++++
>   4 files changed, 45 insertions(+), 10 deletions(-)
> 

Please add me to cc.
I confirm that check-unit: tests/test-char does not pass.

Andrey
-- 
With the best regards,
Andrey Shinkevich