[Qemu-devel] [PATCH] vhost-user: don't poke at chardev internal QemuOpts

[Daniel P. Berrange]

The vhost-user code is poking at the QemuOpts instance
in the CharDriverState struct, not realizing that it is
valid for this to be NULL. e.g. the following crash
shows a codepath where it will be NULL:

 Program terminated with signal SIGSEGV, Segmentation fault.
 #0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650 
<net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at 
util/qemu-option.c:617
 617         QTAILQ_FOREACH(opt, &opts->head, next) {
 [Current thread is 1 (Thread 0x7f1d4970bb40 (LWP 6603))]
 (gdb) bt
 #0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650 
<net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at 
util/qemu-option.c:617
 #1  0x000055baf696b7da in net_vhost_parse_chardev (opts=0x55baf8ff9260, 
errp=0x7ffc51368e48) at net/vhost-user.c:314
 #2  0x000055baf696b985 in net_init_vhost_user (netdev=0x55baf8ff9250, 
name=0x55baf879d270 "hostnet2", peer=0x0, errp=0x7ffc51368e48) at 
net/vhost-user.c:360
 #3  0x000055baf6960216 in net_client_init1 (object=0x55baf8ff9250, 
is_netdev=true, errp=0x7ffc51368e48) at net/net.c:1051
 #4  0x000055baf6960518 in net_client_init (opts=0x55baf776e7e0, 
is_netdev=true, errp=0x7ffc51368f00) at net/net.c:1108
 #5  0x000055baf696083f in netdev_add (opts=0x55baf776e7e0, 
errp=0x7ffc51368f00) at net/net.c:1186
 #6  0x000055baf69608c7 in qmp_netdev_add (qdict=0x55baf7afaf60, 
ret=0x7ffc51368f50, errp=0x7ffc51368f48) at net/net.c:1205
 #7  0x000055baf6622135 in handle_qmp_command (parser=0x55baf77fb590, 
tokens=0x7f1d24011960) at /path/to/qemu.git/monitor.c:3978
 #8  0x000055baf6a9d099 in json_message_process_token (lexer=0x55baf77fb598, 
input=0x55baf75acd20, type=JSON_RCURLY, x=113, y=19) at 
qobject/json-streamer.c:105
 #9  0x000055baf6abf7aa in json_lexer_feed_char (lexer=0x55baf77fb598, ch=125 
'}', flush=false) at qobject/json-lexer.c:319
 #10 0x000055baf6abf8f2 in json_lexer_feed (lexer=0x55baf77fb598, 
buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-lexer.c:369
 #11 0x000055baf6a9d13c in json_message_parser_feed (parser=0x55baf77fb590, 
buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-streamer.c:124
 #12 0x000055baf66221f7 in monitor_qmp_read (opaque=0x55baf77fb530, 
buf=0x7ffc51369170 "}R\204\367\272U", size=1) at 
/path/to/qemu.git/monitor.c:3994
 #13 0x000055baf6757014 in qemu_chr_be_write_impl (s=0x55baf7610a40, 
buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:387
 #14 0x000055baf6757076 in qemu_chr_be_write (s=0x55baf7610a40, 
buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:399
 #15 0x000055baf675b3b0 in tcp_chr_read (chan=0x55baf90244b0, cond=G_IO_IN, 
opaque=0x55baf7610a40) at qemu-char.c:2927
 #16 0x000055baf6a5d655 in qio_channel_fd_source_dispatch 
(source=0x55baf7610df0, callback=0x55baf675b25a <tcp_chr_read>, 
user_data=0x55baf7610a40) at io/channel-watch.c:84
 #17 0x00007f1d3e80cbbd in g_main_context_dispatch () from 
/usr/lib64/libglib-2.0.so.0
 #18 0x000055baf69d3720 in glib_pollfds_poll () at main-loop.c:213
 #19 0x000055baf69d37fd in os_host_main_loop_wait (timeout=126000000) at 
main-loop.c:258
 #20 0x000055baf69d38ad in main_loop_wait (nonblocking=0) at main-loop.c:506
 #21 0x000055baf676587b in main_loop () at vl.c:1908
 #22 0x000055baf676d3bf in main (argc=101, argv=0x7ffc5136a6c8, 
envp=0x7ffc5136a9f8) at vl.c:4604
 (gdb) p opts
 $1 = (QemuOpts *) 0x0

The crash occurred when attaching vhost-user net via QMP:

{
    "execute": "chardev-add",
    "arguments": {
        "id": "charnet2",
        "backend": {
            "type": "socket",
            "data": {
                "addr": {
                    "type": "unix",
                    "data": {
                        "path": "/var/run/openvswitch/vhost-user1"
                    }
                },
                "wait": false,
                "server": false
            }
        }
    },
    "id": "libvirt-19"
}
{
    "return": {

    },
    "id": "libvirt-19"
}
{
    "execute": "netdev_add",
    "arguments": {
        "type": "vhost-user",
        "chardev": "charnet2",
        "id": "hostnet2"
    },
    "id": "libvirt-20"
}

The vhost-user code should not be poking at the internals of the
CharDriverState struct. What it wants is a chardev that is operating
as a network server, so add an explicit API to allow it to validate
this feature condition without looking at chardev internals.

Signed-off-by: Daniel P. Berrange <address@hidden>
---

NB, I've not functionally tested this - just compiled and
unit tested.

 include/sysemu/char.h |  1 +
 net/vhost-user.c      | 40 ++++------------------------------------
 qemu-char.c           |  7 +++++++
 3 files changed, 12 insertions(+), 36 deletions(-)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 0d0465a..50a6603 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -437,6 +437,7 @@ int qemu_chr_add_client(CharDriverState *s, int fd);
 CharDriverState *qemu_chr_find(const char *name);
 bool chr_is_ringbuf(const CharDriverState *chr);
 
+bool qemu_chr_is_network_server(CharDriverState *chr);
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
 
 void register_char_driver(const char *name, ChardevBackendKind kind,
diff --git a/net/vhost-user.c b/net/vhost-user.c
index b0595f8..2e2cf24 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -27,11 +27,6 @@ typedef struct VhostUserState {
     bool started;
 } VhostUserState;
 
-typedef struct VhostUserChardevProps {
-    bool is_socket;
-    bool is_unix;
-} VhostUserChardevProps;
-
 VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
 {
     VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
@@ -278,45 +273,18 @@ static int net_vhost_user_init(NetClientState *peer, 
const char *device,
     return 0;
 }
 
-static int net_vhost_chardev_opts(void *opaque,
-                                  const char *name, const char *value,
-                                  Error **errp)
-{
-    VhostUserChardevProps *props = opaque;
-
-    if (strcmp(name, "backend") == 0 && strcmp(value, "socket") == 0) {
-        props->is_socket = true;
-    } else if (strcmp(name, "path") == 0) {
-        props->is_unix = true;
-    } else if (strcmp(name, "server") == 0) {
-    } else {
-        error_setg(errp,
-                   "vhost-user does not support a chardev with option %s=%s",
-                   name, value);
-        return -1;
-    }
-    return 0;
-}
-
-static CharDriverState *net_vhost_parse_chardev(
+static CharDriverState *net_vhost_claim_chardev(
     const NetdevVhostUserOptions *opts, Error **errp)
 {
     CharDriverState *chr = qemu_chr_find(opts->chardev);
-    VhostUserChardevProps props;
 
     if (chr == NULL) {
         error_setg(errp, "chardev \"%s\" not found", opts->chardev);
         return NULL;
     }
 
-    /* inspect chardev opts */
-    memset(&props, 0, sizeof(props));
-    if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props, errp)) {
-        return NULL;
-    }
-
-    if (!props.is_socket || !props.is_unix) {
-        error_setg(errp, "chardev \"%s\" is not a unix socket",
+    if (!qemu_chr_is_network_server(chr)) {
+        error_setg(errp, "chardev \"%s\" does not provide a network server",
                    opts->chardev);
         return NULL;
     }
@@ -357,7 +325,7 @@ int net_init_vhost_user(const Netdev *netdev, const char 
*name,
     assert(netdev->type == NET_CLIENT_DRIVER_VHOST_USER);
     vhost_user_opts = &netdev->u.vhost_user;
 
-    chr = net_vhost_parse_chardev(vhost_user_opts, errp);
+    chr = net_vhost_claim_chardev(vhost_user_opts, errp);
     if (!chr) {
         return -1;
     }
diff --git a/qemu-char.c b/qemu-char.c
index fb456ce..977fb1a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4596,6 +4596,13 @@ static CharDriverState *qmp_chardev_open_udp(const char 
*id,
     return qemu_chr_open_udp(sioc, common, errp);
 }
 
+
+bool qemu_chr_is_network_server(CharDriverState *chr)
+{
+    return chr->chr_wait_connected != NULL;
+}
+
+
 ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
                                Error **errp)
 {
-- 
2.7.4