[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] net: don't poke at chardev internal QemuOpts
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v3] net: don't poke at chardev internal QemuOpts |
Date: |
Fri, 07 Oct 2016 13:37:23 +0000 |
Hi
On Fri, Oct 7, 2016 at 4:18 PM Daniel P. Berrange <address@hidden>
wrote:
> The vhost-user & colo 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"
> }
>
> Code using chardevs should not be poking at the internals of the
> CharDriverState struct. What vhost-user wants is a chardev that is
> operating as reconnectable network service, along with the ability
> to do FD passing over the connection. The colo code simply wants
> a network service. Add a feature concept to the char drivers so
> that chardev users can query the actual features they wish to have
> supported. The QemuOpts member is removed to prevent future mistakes
> in this area.
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>
> Changed in v3:
>
> - Fix colo code too
> - Remove QemuOpts from CharDriverState
> - Generalize to allow both client & server mode chardevs
> for vhost-user
>
> Changed in v2:
>
> - Switch to use a bitmap to track features in chardev
> - Check for FD passing support in chardev
>
> hmp.c | 1 +
> include/sysemu/char.h | 21 ++++++++++++++++++++-
> net/colo-compare.c | 30 ++----------------------------
> net/vhost-user.c | 41 +++++++----------------------------------
> qemu-char.c | 22 +++++++++++++++++++---
> 5 files changed, 49 insertions(+), 66 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 336e7bf..3c42182 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1909,6 +1909,7 @@ void hmp_chardev_add(Monitor *mon, const QDict
> *qdict)
> error_setg(&err, "Parsing chardev args failed");
> } else {
> qemu_chr_new_from_opts(opts, NULL, &err);
> + qemu_opts_del(opts);
>
The function no longer takes ownership of opts. Important enough to mention
in commit log perhaps. I guess it's okay to keep opts around after vl.c
chardev_init_func
> }
> hmp_handle_error(mon, &err);
> }
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 0d0465a..19dad3f 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -9,6 +9,7 @@
> #include "qapi/qmp/qobject.h"
> #include "qapi/qmp/qstring.h"
> #include "qemu/main-loop.h"
> +#include "qemu/bitmap.h"
>
> /* character device */
>
> @@ -58,6 +59,20 @@ struct ParallelIOArg {
>
> typedef void IOEventHandler(void *opaque, int event);
>
> +typedef enum {
> + /* Whether the chardev peer is able to close and
> + * reopen the data channel, thus requiring support
> + * for qemu_chr_wait_connected() to wait for a
> + * valid connection */
> + QEMU_CHAR_FEATURE_RECONNECTABLE,
> + /* Whether it is possible to send/recv file descriptors
> + * over the data channel */
> + QEMU_CHAR_FEATURE_FD_PASS,
> +
> + QEMU_CHAR_FEATURE_LAST,
> +} CharDriverFeature;
> +
> +
> struct CharDriverState {
> QemuMutex chr_write_lock;
> void (*init)(struct CharDriverState *s);
> @@ -93,8 +108,8 @@ struct CharDriverState {
> int avail_connections;
> int is_mux;
> guint fd_in_tag;
> - QemuOpts *opts;
> bool replay;
> + DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
> QTAILQ_ENTRY(CharDriverState) next;
> };
>
> @@ -437,6 +452,10 @@ 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_has_feature(CharDriverState *chr,
> + CharDriverFeature feature);
> +void qemu_chr_set_feature(CharDriverState *chr,
> + CharDriverFeature feature);
> QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
>
> void register_char_driver(const char *name, ChardevBackendKind kind,
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 22b1da1..47703c5 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -564,29 +564,6 @@ static void compare_sec_rs_finalize(SocketReadState
> *sec_rs)
> }
> }
>
> -static int compare_chardev_opts(void *opaque,
> - const char *name, const char *value,
> - Error **errp)
> -{
> - CompareChardevProps *props = opaque;
> -
> - if (strcmp(name, "backend") == 0 &&
> - strcmp(value, "socket") == 0) {
> - props->is_socket = true;
> - return 0;
> - } else if (strcmp(name, "host") == 0 ||
> - (strcmp(name, "port") == 0) ||
> - (strcmp(name, "server") == 0) ||
> - (strcmp(name, "wait") == 0) ||
> - (strcmp(name, "path") == 0)) {
> - return 0;
> - } else {
> - error_setg(errp,
> - "COLO-compare does not support a chardev with option
> %s=%s",
> - name, value);
> - return -1;
> - }
> -}
>
> /*
> * Return 0 is success.
> @@ -606,12 +583,9 @@ static int find_and_check_chardev(CharDriverState
> **chr,
> }
>
> memset(&props, 0, sizeof(props));
> - if (qemu_opt_foreach((*chr)->opts, compare_chardev_opts, &props,
> errp)) {
> - return 1;
> - }
>
> - if (!props.is_socket) {
> - error_setg(errp, "chardev \"%s\" is not a tcp socket",
> + if (!qemu_chr_has_feature(*chr, QEMU_CHAR_FEATURE_RECONNECTABLE)) {
> + error_setg(errp, "chardev \"%s\" is not reconnectable",
> chr_name);
> return 1;
> }
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index b0595f8..5b94c84 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,23 @@ 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)) {
> + if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE)) {
> + error_setg(errp, "chardev \"%s\" is not reconnectable",
> + opts->chardev);
> return NULL;
> }
> -
> - if (!props.is_socket || !props.is_unix) {
> - error_setg(errp, "chardev \"%s\" is not a unix socket",
> + if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_FD_PASS)) {
> + error_setg(errp, "chardev \"%s\" does not support FD passing",
> opts->chardev);
> return NULL;
> }
> @@ -357,7 +330,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..768150d 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3996,7 +3996,6 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts
> *opts,
> }
>
> chr = qemu_chr_find(id);
> - chr->opts = opts;
>
> qapi_out:
> qapi_free_ChardevBackend(backend);
> @@ -4005,7 +4004,6 @@ qapi_out:
> return chr;
>
> err:
> - qemu_opts_del(opts);
> return NULL;
> }
>
> @@ -4033,6 +4031,7 @@ CharDriverState *qemu_chr_new_noreplay(const char
> *label, const char *filename,
> qemu_chr_fe_claim_no_fail(chr);
> monitor_init(chr, MONITOR_USE_READLINE);
> }
> + qemu_opts_del(opts);
> return chr;
> }
>
> @@ -4132,7 +4131,6 @@ static void qemu_chr_free_common(CharDriverState
> *chr)
> {
> g_free(chr->filename);
> g_free(chr->label);
> - qemu_opts_del(chr->opts);
> if (chr->logfd != -1) {
> close(chr->logfd);
> }
> @@ -4513,6 +4511,11 @@ static CharDriverState
> *qmp_chardev_open_socket(const char *id,
>
> s->addr = QAPI_CLONE(SocketAddress, sock->addr);
>
> + qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
> + if (s->is_unix) {
> + qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
> + }
> +
> chr->opaque = s;
> chr->chr_wait_connected = tcp_chr_wait_connected;
> chr->chr_write = tcp_chr_write;
> @@ -4596,6 +4599,19 @@ static CharDriverState *qmp_chardev_open_udp(const
> char *id,
> return qemu_chr_open_udp(sioc, common, errp);
> }
>
> +
> +bool qemu_chr_has_feature(CharDriverState *chr,
> + CharDriverFeature feature)
> +{
> + return test_bit(feature, chr->features);
> +}
> +
> +void qemu_chr_set_feature(CharDriverState *chr,
> + CharDriverFeature feature)
> +{
> + return set_bit(feature, chr->features);
> +}
> +
> ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> Error **errp)
> {
> --
> 2.7.4
>
Reviewed-by: Marc-André Lureau <address@hidden>
--
Marc-André Lureau