[Qemu-devel] [PATCH 0/5] vhost-user: fixes from reconnect RFC

[Qemu-devel] [PATCH 0/5] vhost-user: fixes from reconnect RFC

[marcandre.lureau]

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

During rebase of "vhost-user: shutdown and reconnection" RFC series, I grew a
set of small fixes that are prerequired and worthy to include now. They are
gathered here.

thanks

Marc-André Lureau (5):
  vhost-user: fix use after free
  vhost-user: remove useless is_server field
  qemu-char: avoid potential double-free
  qemu-char: remove all msgfds on disconnect
  qemu-char: make tcp_chr_disconnect() reentrant-safe

 net/vhost-user.c | 9 ++++++---
 qemu-char.c      | 6 ++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/5] vhost-user: fix use after free

[marcandre.lureau]

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

"name" is freed after visiting options, instead use the first NetClientState
name. Adds a few assert() for clarifying and checking some impossible states.

READ of size 1 at 0x602000000990 thread T0
    #0 0x7f6b251c570c  (/lib64/libasan.so.2+0x4770c)
    #1 0x5566dc380600 in qemu_find_net_clients_except net/net.c:824
    #2 0x5566dc39bac7 in net_vhost_user_event net/vhost-user.c:193
    #3 0x5566dbee862a in qemu_chr_be_event /home/elmarco/src/qemu/qemu-char.c:201
    #4 0x5566dbef2890 in tcp_chr_disconnect /home/elmarco/src/qemu/qemu-char.c:2790
    #5 0x5566dbef2d0b in tcp_chr_sync_read /home/elmarco/src/qemu/qemu-char.c:2835
    #6 0x5566dbee8a99 in qemu_chr_fe_read_all /home/elmarco/src/qemu/qemu-char.c:295
    #7 0x5566dc39b964 in net_vhost_user_watch net/vhost-user.c:180
    #8 0x5566dc5a06c7 in qio_channel_fd_source_dispatch io/channel-watch.c:70
    #9 0x7f6b1aa2ab87 in g_main_dispatch /home/elmarco/src/gnome/glib/glib/gmain.c:3154
    #10 0x7f6b1aa2b9cb in g_main_context_dispatch /home/elmarco/src/gnome/glib/glib/gmain.c:3769
    #11 0x5566dc475ed4 in glib_pollfds_poll /home/elmarco/src/qemu/main-loop.c:212
    #12 0x5566dc476029 in os_host_main_loop_wait /home/elmarco/src/qemu/main-loop.c:257
    #13 0x5566dc476165 in main_loop_wait /home/elmarco/src/qemu/main-loop.c:505
    #14 0x5566dbf08d31 in main_loop /home/elmarco/src/qemu/vl.c:1932
    #15 0x5566dbf16783 in main /home/elmarco/src/qemu/vl.c:4646
    #16 0x7f6b180bb57f in __libc_start_main (/lib64/libc.so.6+0x2057f)
    #17 0x5566dbbf5348 in _start (/home/elmarco/src/qemu/x86_64-softmmu/qemu-system-x86_64+0x3f9348)

0x602000000990 is located 0 bytes inside of 5-byte region [0x602000000990,0x602000000995)
freed by thread T0 here:
    #0 0x7f6b2521666a in __interceptor_free (/lib64/libasan.so.2+0x9866a)
    #1 0x7f6b1aa332a4 in g_free /home/elmarco/src/gnome/glib/glib/gmem.c:189
    #2 0x5566dc5f416f in qapi_dealloc_type_str qapi/qapi-dealloc-visitor.c:134
    #3 0x5566dc5f3268 in visit_type_str qapi/qapi-visit-core.c:196
    #4 0x5566dc5ced58 in visit_type_Netdev_fields /home/elmarco/src/qemu/qapi-visit.c:5936
    #5 0x5566dc5cef71 in visit_type_Netdev /home/elmarco/src/qemu/qapi-visit.c:5960
    #6 0x5566dc381a8d in net_visit net/net.c:1049
    #7 0x5566dc381c37 in net_client_init net/net.c:1076
    #8 0x5566dc3839e2 in net_init_netdev net/net.c:1473
    #9 0x5566dc63cc0a in qemu_opts_foreach util/qemu-option.c:1112
    #10 0x5566dc383b36 in net_init_clients net/net.c:1499
    #11 0x5566dbf15d86 in main /home/elmarco/src/qemu/vl.c:4397
    #12 0x7f6b180bb57f in __libc_start_main (/lib64/libc.so.6+0x2057f)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/vhost-user.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 451dbbf..9d2a6fa 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -179,6 +179,8 @@ static void net_vhost_user_event(void *opaque, int event)
     queues = qemu_find_net_clients_except(name, ncs,
                                           NET_CLIENT_OPTIONS_KIND_NIC,
                                           MAX_QUEUE_NUM);
+    assert(queues < MAX_QUEUE_NUM);
+
     s = DO_UPCAST(VhostUserState, nc, ncs[0]);
     trace_vhost_user_event(s->chr->label, event);
     switch (event) {
@@ -207,6 +209,9 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
     VhostUserState *s;
     int i;
 
+    assert(name);
+    assert(queues > 0);
+
     for (i = 0; i < queues; i++) {
         nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
 
@@ -219,7 +224,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
         s->chr = chr;
     }
 
-    qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, (void*)name);
+    qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, nc[0].name);
 
     return 0;
 }
-- 
2.5.0

[Qemu-devel] [PATCH 2/5] vhost-user: remove useless is_server field

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/vhost-user.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 9d2a6fa..eb40387 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -27,7 +27,6 @@ typedef struct VhostUserState {
 typedef struct VhostUserChardevProps {
     bool is_socket;
     bool is_unix;
-    bool is_server;
 } VhostUserChardevProps;
 
 VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
@@ -240,7 +239,6 @@ static int net_vhost_chardev_opts(void *opaque,
     } else if (strcmp(name, "path") == 0) {
         props->is_unix = true;
     } else if (strcmp(name, "server") == 0) {
-        props->is_server = true;
     } else {
         error_setg(errp,
                    "vhost-user does not support a chardev with option %s=%s",
-- 
2.5.0

[Qemu-devel] [PATCH 3/5] qemu-char: avoid potential double-free

[marcandre.lureau]

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

If tcp_set_msgfds() is called several time with NULL fds, this
could lead to double-free.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-char.c b/qemu-char.c
index ad11b75..24e326a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2697,6 +2697,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num)
     }
     /* clear old pending fd array */
     g_free(s->write_msgfds);
+    s->write_msgfds = NULL;
 
     if (num) {
         s->write_msgfds = g_new(int, num);
-- 
2.5.0

[Qemu-devel] [PATCH 4/5] qemu-char: remove all msgfds on disconnect

[marcandre.lureau]

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

Disconnect should reset context.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-char.c b/qemu-char.c
index 24e326a..dc433a6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2774,6 +2774,7 @@ static void tcp_chr_disconnect(CharDriverState *chr)
         s->listen_tag = qio_channel_add_watch(
             QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
     }
+    tcp_set_msgfds(chr, NULL, 0);
     remove_fd_in_watch(chr);
     object_unref(OBJECT(s->sioc));
     s->sioc = NULL;
-- 
2.5.0

[Qemu-devel] [PATCH 5/5] qemu-char: make tcp_chr_disconnect() reentrant-safe

[marcandre.lureau]

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

During CHR_EVENT_CLOSED, the function could be reentered, make this
case safe.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index dc433a6..9bd568b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2769,6 +2769,10 @@ static void tcp_chr_disconnect(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
 
+    if (!s->connected) {
+        return;
+    }
+
     s->connected = 0;
     if (s->listen_ioc) {
         s->listen_tag = qio_channel_add_watch(
-- 
2.5.0