[Qemu-devel] [PATCH 0/3] vhost-user-test fix

[Qemu-devel] [PATCH 0/3] vhost-user-test fix

[Li Qiang]

Currently, the vhost-user-test is not correct.
When in qtest mode, the accel is qtest, not kvm.
So when the client side of vhost-user-test send
'VHOST_USER_SET_VRING_CALL' msg, the 'fd' will
no be added in 'fds' in 'vhost_set_vring_file'.
In 'chr_read' of the server side in the 
vhost-user-test, it calls 'qemu_chr_fe_get_msgfds'
to get the fd in 'VHOST_USER_SET_VRING_CALL'. Though
there is no fd returned, but as the 'fd' is not initialized
so 'fd' maybe valid, and 'qemu_set_nonblock' will be success.
Even worse, 'qemu_set_nonblock' doesn't check the return value
of fcntl.

So this cause the interesting bug here: there are three issues,
but they combined and will bypass the qtest.

This patchset tries to address these issue.

Li Qiang (3):
  tests: vhost-user-test: initialize 'fd' in chr_read
  vhost-user: add fds inf 'vhost_set_vring_file' in qtest
  util: check the return value of fcntl in qemu_set_{block, nonblock}

 hw/virtio/vhost-user.c  | 3 ++-
 tests/vhost-user-test.c | 2 +-
 util/oslib-posix.c      | 8 ++++++--
 3 files changed, 9 insertions(+), 4 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/3] tests: vhost-user-test: initialize 'fd' in chr_read

[Li Qiang]

Currentyly when processing VHOST_USER_SET_VRING_CALL
if 'qemu_chr_fe_get_msgfds' get no fd, the 'fd' will
be a stack uninitialized value.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 tests/vhost-user-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 45d58d8ea2..86039e61e0 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -309,7 +309,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
     CharBackend *chr = &s->chr;
     VhostUserMsg msg;
     uint8_t *p = (uint8_t *) &msg;
-    int fd;
+    int fd = -1;
 
     if (s->test_fail) {
         qemu_chr_fe_disconnect(chr);
-- 
2.17.1

[Qemu-devel] [PATCH 2/3] vhost-user: add fds inf 'vhost_set_vring_file' in qtest

[Li Qiang]

Currently, the vhost-user-test assumes the eventfd is available.
However it's not true because the accel is qtest. So the
'vhost_set_vring_file' will not add fds to the msg and the server
side of vhost-user-test will be broken. This patch avoid this.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/virtio/vhost-user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e09bed0e4a..3b666f093c 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -23,6 +23,7 @@
 #include "migration/migration.h"
 #include "migration/postcopy-ram.h"
 #include "trace.h"
+#include "sysemu/qtest.h"
 
 #include <sys/ioctl.h>
 #include <sys/socket.h>
@@ -742,7 +743,7 @@ static int vhost_set_vring_file(struct vhost_dev *dev,
         .hdr.size = sizeof(msg.payload.u64),
     };
 
-    if (ioeventfd_enabled() && file->fd > 0) {
+    if ((qtest_enabled() || ioeventfd_enabled()) && file->fd > 0) {
         fds[fd_num++] = file->fd;
     } else {
         msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
-- 
2.17.1

[Qemu-devel] [PATCH 3/3] util: check the return value of fcntl in qemu_set_{block, nonblock}

[Li Qiang]

Assert that the return value is not an error. This is like commit
7e6478e7d4f for qemu_set_cloexec.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 util/oslib-posix.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index c1bee2a581..4ce1ba9ca4 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -233,14 +233,18 @@ void qemu_set_block(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    assert(f != -1);
+    f = fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    assert(f != -1);
 }
 
 void qemu_set_nonblock(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f | O_NONBLOCK);
+    assert(f != -1);
+    f = fcntl(fd, F_SETFL, f | O_NONBLOCK);
+    assert(f != -1);
 }
 
 int socket_set_fast_reuse(int fd)
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/3] vhost-user: add fds inf 'vhost_set_vring_file' in qtest

[Paolo Bonzini]

On 15/12/18 02:26, Li Qiang wrote:
> Currently, the vhost-user-test assumes the eventfd is available.
> However it's not true because the accel is qtest. So the
> 'vhost_set_vring_file' will not add fds to the msg and the server
> side of vhost-user-test will be broken. This patch avoid this.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/virtio/vhost-user.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index e09bed0e4a..3b666f093c 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -23,6 +23,7 @@
>  #include "migration/migration.h"
>  #include "migration/postcopy-ram.h"
>  #include "trace.h"
> +#include "sysemu/qtest.h"
>  
>  #include <sys/ioctl.h>
>  #include <sys/socket.h>
> @@ -742,7 +743,7 @@ static int vhost_set_vring_file(struct vhost_dev *dev,
>          .hdr.size = sizeof(msg.payload.u64),
>      };
>  
> -    if (ioeventfd_enabled() && file->fd > 0) {

The bug is in ioeventfd_enabled.  It should be !kvm_enabled() ||
kvm_eventfds_enabled().

Paolo

> +    if ((qtest_enabled() || ioeventfd_enabled()) && file->fd > 0) {
>          fds[fd_num++] = file->fd;
>      } else {
>          msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
>