Re: [Qemu-devel] [PATCH v3] net: don't poke at chardev internal QemuOpts

2016-10-07 Thread Marc-André Lureau 
Hi

On Fri, Oct 7, 2016 at 4:18 PM Daniel P. Berrange 
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  0x55baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650
> , opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at
> util/qemu-option.c:617
>  617 QTAILQ_FOREACH(opt, >head, next) {
>  [Current thread is 1 (Thread 0x7f1d4970bb40 (LWP 6603))]
>  (gdb) bt
>  #0  0x55baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650
> , opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at
> util/qemu-option.c:617
>  #1  0x55baf696b7da in net_vhost_parse_chardev (opts=0x55baf8ff9260,
> errp=0x7ffc51368e48) at net/vhost-user.c:314
>  #2  0x55baf696b985 in net_init_vhost_user (netdev=0x55baf8ff9250,
> name=0x55baf879d270 "hostnet2", peer=0x0, errp=0x7ffc51368e48) at
> net/vhost-user.c:360
>  #3  0x55baf6960216 in net_client_init1 (object=0x55baf8ff9250,
> is_netdev=true, errp=0x7ffc51368e48) at net/net.c:1051
>  #4  0x55baf6960518 in net_client_init (opts=0x55baf776e7e0,
> is_netdev=true, errp=0x7ffc51368f00) at net/net.c:1108
>  #5  0x55baf696083f in netdev_add (opts=0x55baf776e7e0,
> errp=0x7ffc51368f00) at net/net.c:1186
>  #6  0x55baf69608c7 in qmp_netdev_add (qdict=0x55baf7afaf60,
> ret=0x7ffc51368f50, errp=0x7ffc51368f48) at net/net.c:1205
>  #7  0x55baf6622135 in handle_qmp_command (parser=0x55baf77fb590,
> tokens=0x7f1d24011960) at /path/to/qemu.git/monitor.c:3978
>  #8  0x55baf6a9d099 in json_message_process_token
> (lexer=0x55baf77fb598, input=0x55baf75acd20, type=JSON_RCURLY, x=113, y=19)
> at qobject/json-streamer.c:105
>  #9  0x55baf6abf7aa in json_lexer_feed_char (lexer=0x55baf77fb598,
> ch=125 '}', flush=false) at qobject/json-lexer.c:319
>  #10 0x55baf6abf8f2 in json_lexer_feed (lexer=0x55baf77fb598,
> buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-lexer.c:369
>  #11 0x55baf6a9d13c in json_message_parser_feed
> (parser=0x55baf77fb590, buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at
> qobject/json-streamer.c:124
>  #12 0x55baf66221f7 in monitor_qmp_read (opaque=0x55baf77fb530,
> buf=0x7ffc51369170 "}R\204\367\272U", size=1) at
> /path/to/qemu.git/monitor.c:3994
>  #13 0x55baf6757014 in qemu_chr_be_write_impl (s=0x55baf7610a40,
> buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:387
>  #14 0x55baf6757076 in qemu_chr_be_write (s=0x55baf7610a40,
> buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:399
>  #15 0x55baf675b3b0 in tcp_chr_read (chan=0x55baf90244b0,
> cond=G_IO_IN, opaque=0x55baf7610a40) at qemu-char.c:2927
>  #16 0x55baf6a5d655 in qio_channel_fd_source_dispatch
> (source=0x55baf7610df0, callback=0x55baf675b25a ,
> user_data=0x55baf7610a40) at io/channel-watch.c:84
>  #17 0x7f1d3e80cbbd in g_main_context_dispatch () from
> /usr/lib64/libglib-2.0.so.0
>  #18 0x55baf69d3720 in glib_pollfds_poll () at main-loop.c:213
>  #19 0x55baf69d37fd in os_host_main_loop_wait (timeout=12600) at
> main-loop.c:258
>  #20 0x55baf69d38ad in main_loop_wait (nonblocking=0) at
> main-loop.c:506
>  #21 0x55baf676587b in main_loop () at vl.c:1908
>  #22 0x55baf676d3bf 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 
> ---
>
> Changed in v3:
>
>  - Fix colo code too
>  - Remove QemuOpts from

[Qemu-devel] [PATCH v3] net: don't poke at chardev internal QemuOpts

2016-10-07 Thread Daniel P. Berrange 
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  0x55baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650 
, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at 
util/qemu-option.c:617
 617 QTAILQ_FOREACH(opt, >head, next) {
 [Current thread is 1 (Thread 0x7f1d4970bb40 (LWP 6603))]
 (gdb) bt
 #0  0x55baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650 
, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at 
util/qemu-option.c:617
 #1  0x55baf696b7da in net_vhost_parse_chardev (opts=0x55baf8ff9260, 
errp=0x7ffc51368e48) at net/vhost-user.c:314
 #2  0x55baf696b985 in net_init_vhost_user (netdev=0x55baf8ff9250, 
name=0x55baf879d270 "hostnet2", peer=0x0, errp=0x7ffc51368e48) at 
net/vhost-user.c:360
 #3  0x55baf6960216 in net_client_init1 (object=0x55baf8ff9250, 
is_netdev=true, errp=0x7ffc51368e48) at net/net.c:1051
 #4  0x55baf6960518 in net_client_init (opts=0x55baf776e7e0, 
is_netdev=true, errp=0x7ffc51368f00) at net/net.c:1108
 #5  0x55baf696083f in netdev_add (opts=0x55baf776e7e0, 
errp=0x7ffc51368f00) at net/net.c:1186
 #6  0x55baf69608c7 in qmp_netdev_add (qdict=0x55baf7afaf60, 
ret=0x7ffc51368f50, errp=0x7ffc51368f48) at net/net.c:1205
 #7  0x55baf6622135 in handle_qmp_command (parser=0x55baf77fb590, 
tokens=0x7f1d24011960) at /path/to/qemu.git/monitor.c:3978
 #8  0x55baf6a9d099 in json_message_process_token (lexer=0x55baf77fb598, 
input=0x55baf75acd20, type=JSON_RCURLY, x=113, y=19) at 
qobject/json-streamer.c:105
 #9  0x55baf6abf7aa in json_lexer_feed_char (lexer=0x55baf77fb598, ch=125 
'}', flush=false) at qobject/json-lexer.c:319
 #10 0x55baf6abf8f2 in json_lexer_feed (lexer=0x55baf77fb598, 
buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-lexer.c:369
 #11 0x55baf6a9d13c in json_message_parser_feed (parser=0x55baf77fb590, 
buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-streamer.c:124
 #12 0x55baf66221f7 in monitor_qmp_read (opaque=0x55baf77fb530, 
buf=0x7ffc51369170 "}R\204\367\272U", size=1) at 
/path/to/qemu.git/monitor.c:3994
 #13 0x55baf6757014 in qemu_chr_be_write_impl (s=0x55baf7610a40, 
buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:387
 #14 0x55baf6757076 in qemu_chr_be_write (s=0x55baf7610a40, 
buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:399
 #15 0x55baf675b3b0 in tcp_chr_read (chan=0x55baf90244b0, cond=G_IO_IN, 
opaque=0x55baf7610a40) at qemu-char.c:2927
 #16 0x55baf6a5d655 in qio_channel_fd_source_dispatch 
(source=0x55baf7610df0, callback=0x55baf675b25a , 
user_data=0x55baf7610a40) at io/channel-watch.c:84
 #17 0x7f1d3e80cbbd in g_main_context_dispatch () from 
/usr/lib64/libglib-2.0.so.0
 #18 0x55baf69d3720 in glib_pollfds_poll () at main-loop.c:213
 #19 0x55baf69d37fd in os_host_main_loop_wait (timeout=12600) at 
main-loop.c:258
 #20 0x55baf69d38ad in main_loop_wait (nonblocking=0) at main-loop.c:506
 #21 0x55baf676587b in main_loop () at vl.c:1908
 #22 0x55baf676d3bf 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 
---

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 -