[Qemu-devel] [PATCH v6 0/4] Unit test+fix for problem with QEMU handling of multiple bind()s to the same port

[Qemu-devel] [PATCH v6 0/4] Unit test+fix for problem with QEMU handling of multiple bind()s to the same port

[Knut Omang]

This series contains:
* a unit test that exposes a race condition which causes QEMU to fail
  to find a port even when there is plenty of available ports.
* a refactor of the qemu-sockets inet_listen_saddr() function
  to better handle this situation.

Changes from v5:
* Also move setting of error from socket creation out
  into the main double for loop.
* Simplify and enhance reporting if socket creation fails:
  Only report failure to create sockets if none of the
  provided addrinfo list elements allows creation of a
  socket, otherwise report failure to find an available port.
* Further simplify if's within the for loops and make sure
  failure to recreate a socket gets distinctively reported.
* Rebased to current master.

Changes from v4:
* Move the complexity of recreating a socket and setting the error pointer
  into the main for loop, eliminating the try_bind_listen() function
  again. Cleaning up and improving error handling in the process.

Changes from v3:
* Test changes: Add missing license
  Add subtests for ipv4, ipv6 and both
  Various g_* usage improvements
* Split patch into 3 patches with two refactoring patches ahead
  of the actual fix.

Changes from v2:
* Non-trivial rebase + further abstraction
  on top of 7ad9af343c7f1c70c8015c7c519c312d8c5f9fa1
  'tests: add functional test validating ipv4/ipv6 address flag handling'

Changes from v1:
* Fix potential uninitialized variable only detected by optimize.
* Improve unexpected error detection in test-listen to give more
  details about why the test fails unexpectedly.
* Fix some line length style issues.

Thanks,
Knut

Knut Omang (4):
  tests: Add test-listen - a stress test for QEMU socket listen
  sockets: factor out a new try_bind() function
  sockets: factor out create_fast_reuse_socket
  sockets: Handle race condition between binds to the same port

 tests/Makefile.include |   2 +-
 tests/test-listen.c    | 253 ++++++++++++++++++++++++++++++++++++++++++-
 util/qemu-sockets.c    | 138 +++++++++++++++--------
 3 files changed, 345 insertions(+), 48 deletions(-)
 create mode 100644 tests/test-listen.c

base-commit: a588c4985eff363154d65aee8607d0a4601655f7
-- 
git-series 0.9.1

[Qemu-devel] [PATCH v6 1/4] tests: Add test-listen - a stress test for QEMU socket listen

[Knut Omang]

There's a potential race condition between multiple bind()'s
attempting to bind to the same port, which occasionally
allows more than one bind to succeed against the same port.

When a subsequent listen() call is made with the same socket
only one will succeed.

The current QEMU code does however not take this situation into account
and the listen will cause the code to break out and fail even
when there are actually available ports to use.

This test exposes two subtests:

/socket/listen-serial
/socket/listen-compete

The "compete" subtest creates a number of threads and have them all trying to bind
to the same port with a large enough offset input to
allow all threads to get it's own port.
The "serial" subtest just does the same, except in series in a
single thread.

The serial version passes, probably in most versions of QEMU.

The parallel version exposes the problem in a relatively reliable way,
eg. it fails a majority of times, but not with a 100% rate, occasional
passes can be seen. Nevertheless this is quite good given that
the bug was tricky to reproduce and has been left undetected for
a while.

The problem seems to be present in all versions of QEMU.

The original failure scenario occurred with VNC port allocation
in a traditional Xen based build, in different code
but with similar functionality.

Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com>
Signed-off-by: Knut Omang <knut.omang@oracle.com>
Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
---
 tests/Makefile.include |   2 +-
 tests/test-listen.c    | 253 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 255 insertions(+)
 create mode 100644 tests/test-listen.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 7af278d..b37c0c8 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -128,6 +128,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
 gcov-files-check-bufferiszero-y = util/bufferiszero.c
 check-unit-y += tests/test-uuid$(EXESUF)
 check-unit-y += tests/ptimer-test$(EXESUF)
+#check-unit-y += tests/test-listen$(EXESUF)
 gcov-files-ptimer-test-y = hw/core/ptimer.c
 check-unit-y += tests/test-qapi-util$(EXESUF)
 gcov-files-test-qapi-util-y = qapi/qapi-util.c
@@ -769,6 +770,7 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
 tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
 tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
+tests/test-listen$(EXESUF): tests/test-listen.o $(test-util-obj-y)
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
diff --git a/tests/test-listen.c b/tests/test-listen.c
new file mode 100644
index 0000000..5c07537
--- /dev/null
+++ b/tests/test-listen.c
@@ -0,0 +1,253 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ *    Author: Knut Omang <knut.omang@oracle.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation.
+ *
+ * Test parallel port listen configuration with
+ * dynamic port allocation
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qemu-common.h"
+#include "qemu/thread.h"
+#include "qemu/sockets.h"
+#include "qapi/error.h"
+
+#define NAME_LEN 1024
+#define PORT_LEN 16
+
+struct thr_info {
+    QemuThread thread;
+    int to_port;
+    bool ipv4;
+    bool ipv6;
+    int got_port;
+    int eno;
+    int fd;
+    const char *errstr;
+    char hostname[NAME_LEN + 1];
+    char port[PORT_LEN + 1];
+};
+
+
+/* These two functions taken from test-io-channel-socket.c */
+static int check_bind(const char *hostname, bool *has_proto)
+{
+    int fd = -1;
+    struct addrinfo ai, *res = NULL;
+    int rc;
+    int ret = -1;
+
+    memset(&ai, 0, sizeof(ai));
+    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
+    ai.ai_family = AF_UNSPEC;
+    ai.ai_socktype = SOCK_STREAM;
+
+    /* lookup */
+    rc = getaddrinfo(hostname, NULL, &ai, &res);
+    if (rc != 0) {
+        if (rc == EAI_ADDRFAMILY ||
+            rc == EAI_FAMILY) {
+            *has_proto = false;
+            goto done;
+        }
+        goto cleanup;
+    }
+
+    fd = qemu_socket(res->ai_family, res->ai_socktype, res->ai_protocol);
+    if (fd < 0) {
+        goto cleanup;
+    }
+
+    if (bind(fd, res->ai_addr, res->ai_addrlen) < 0) {
+        if (errno == EADDRNOTAVAIL) {
+            *has_proto = false;
+            goto done;
+        }
+        goto cleanup;
+    }
+
+    *has_proto = true;
+ done:
+    ret = 0;
+
+ cleanup:
+    if (fd != -1) {
+        close(fd);
+    }
+    if (res) {
+        freeaddrinfo(res);
+    }
+    return ret;
+}
+
+static int check_protocol_support(bool *has_ipv4, bool *has_ipv6)
+{
+    if (check_bind("127.0.0.1", has_ipv4) < 0) {
+        return -1;
+    }
+    if (check_bind("::1", has_ipv6) < 0) {
+        return -1;
+    }
+
+    return 0;
+}
+
+static void *listener_thread(void *arg)
+{
+    struct thr_info *thr = (struct thr_info *)arg;
+    SocketAddress addr = {
+        .type = SOCKET_ADDRESS_TYPE_INET,
+        .u = {
+            .inet = {
+                .host = thr->hostname,
+                .port = thr->port,
+                .has_ipv4 = thr->ipv4,
+                .ipv4 = thr->ipv4,
+                .has_ipv6 = thr->ipv6,
+                .ipv6 = thr->ipv6,
+                .has_to = true,
+                .to = thr->to_port,
+            },
+        },
+    };
+    Error *err = NULL;
+    int fd;
+
+    fd = socket_listen(&addr, &err);
+    if (fd < 0) {
+        thr->eno = errno;
+        thr->errstr = error_get_pretty(err);
+    } else {
+        struct sockaddr_in a;
+        socklen_t a_len = sizeof(a);
+        g_assert_cmpint(getsockname(fd, (struct sockaddr *)&a, &a_len), ==, 0);
+        thr->got_port = ntohs(a.sin_port);
+        thr->fd = fd;
+    }
+    return arg;
+}
+
+
+static void listen_compete_nthr(bool threaded, int nthreads,
+                                int start_port, int max_offset,
+                                bool ipv4, bool ipv6)
+{
+    int i;
+    int failed_listens = 0;
+    struct thr_info *thr = g_new0(struct thr_info, nthreads);
+    int used[max_offset + 1];
+
+    memset(used, 0, sizeof(used));
+    for (i = 0; i < nthreads; i++) {
+        snprintf(thr[i].port, PORT_LEN, "%d", start_port);
+        strcpy(thr[i].hostname, "localhost");
+        thr[i].to_port = start_port + max_offset;
+        thr[i].ipv4 = ipv4;
+        thr[i].ipv6 = ipv6;
+    }
+
+    for (i = 0; i < nthreads; i++) {
+        if (threaded) {
+            qemu_thread_create(&thr[i].thread, "listener",
+                               listener_thread, &thr[i],
+                               QEMU_THREAD_JOINABLE);
+        } else {
+            listener_thread(&thr[i]);
+        }
+    }
+
+    if (threaded) {
+        for (i = 0; i < nthreads; i++) {
+            qemu_thread_join(&thr[i].thread);
+        }
+    }
+    for (i = 0; i < nthreads; i++) {
+        if (thr[i].got_port) {
+            closesocket(thr[i].fd);
+        }
+    }
+
+    for (i = 0; i < nthreads; i++) {
+        if (thr[i].eno != 0) {
+            const char *m;
+            g_printerr("** Failed to assign a port to thread %d (errno = %d)\n",
+                   i, thr[i].eno);
+            /* This is what we are interested in capturing -
+             * catch and report details if something unexpected happens:
+             */
+            m = strstr(thr[i].errstr, "Failed to listen on socket");
+            if (m != NULL) {
+                g_assert_cmpstr(thr[i].errstr, ==,
+                    "Failed to listen on socket: Address already in use");
+            }
+            failed_listens++;
+        } else {
+            int assigned_port = thr[i].got_port;
+            g_assert_cmpint(assigned_port, <= , thr[i].to_port);
+            g_assert_cmpint(used[assigned_port - start_port], == , 0);
+        }
+    }
+    g_assert_cmpint(failed_listens, ==, 0);
+    g_free(thr);
+}
+
+
+static void listen_compete_ipv4(void)
+{
+    listen_compete_nthr(true, 200, 5920, 300, true, false);
+}
+
+static void listen_serial_ipv4(void)
+{
+    listen_compete_nthr(false, 200, 6300, 300, true, false);
+}
+
+static void listen_compete_ipv6(void)
+{
+    listen_compete_nthr(true, 200, 5920, 300, true, false);
+}
+
+static void listen_serial_ipv6(void)
+{
+    listen_compete_nthr(false, 200, 6300, 300, false, true);
+}
+
+static void listen_compete_gen(void)
+{
+    listen_compete_nthr(true, 200, 5920, 300, true, true);
+}
+
+static void listen_serial_gen(void)
+{
+    listen_compete_nthr(false, 200, 6300, 300, true, true);
+}
+
+
+int main(int argc, char **argv)
+{
+    bool has_ipv4, has_ipv6;
+    g_test_init(&argc, &argv, NULL);
+
+    if (check_protocol_support(&has_ipv4, &has_ipv6) < 0) {
+        return 1;
+    }
+
+    if (has_ipv4) {
+        g_test_add_func("/socket/listen-serial/ipv4", listen_serial_ipv4);
+        g_test_add_func("/socket/listen-compete/ipv4", listen_compete_ipv4);
+    }
+    if (has_ipv6) {
+        g_test_add_func("/socket/listen-serial/ipv6", listen_serial_ipv6);
+        g_test_add_func("/socket/listen-compete/ipv6", listen_compete_ipv6);
+    }
+    if (has_ipv4 && has_ipv6) {
+        g_test_add_func("/socket/listen-serial/generic", listen_serial_gen);
+        g_test_add_func("/socket/listen-compete/generic", listen_compete_gen);
+    }
+    return g_test_run();
+}
-- 
git-series 0.9.1

[Qemu-devel] [PATCH v6 2/4] sockets: factor out a new try_bind() function

[Knut Omang]

A refactoring step to prepare for the problem
exposed by the test-listen test in the previous commit.

Simplify and reorganize the IPv6 specific extra
measures and move it out of the for loop to increase
code readability. No semantic changes.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/qemu-sockets.c | 69 ++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 30 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 1358c81..b4a2f24 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -149,6 +149,44 @@ int inet_ai_family_from_address(InetSocketAddress *addr,
     return PF_UNSPEC;
 }
 
+static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e)
+{
+#ifndef IPV6_V6ONLY
+    return bind(socket, e->ai_addr, e->ai_addrlen);
+#else
+    /*
+     * Deals with first & last cases in matrix in comment
+     * for inet_ai_family_from_address().
+     */
+    int v6only =
+        ((!saddr->has_ipv4 && !saddr->has_ipv6) ||
+         (saddr->has_ipv4 && saddr->ipv4 &&
+          saddr->has_ipv6 && saddr->ipv6)) ? 0 : 1;
+    int stat;
+
+ rebind:
+    if (e->ai_family == PF_INET6) {
+        qemu_setsockopt(socket, IPPROTO_IPV6, IPV6_V6ONLY, &v6only,
+                        sizeof(v6only));
+    }
+
+    stat = bind(socket, e->ai_addr, e->ai_addrlen);
+    if (!stat) {
+        return 0;
+    }
+
+    /* If we got EADDRINUSE from an IPv6 bind & v6only is unset,
+     * it could be that the IPv4 port is already claimed, so retry
+     * with v6only set
+     */
+    if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) {
+        v6only = 1;
+        goto rebind;
+    }
+    return stat;
+#endif
+}
+
 static int inet_listen_saddr(InetSocketAddress *saddr,
                              int port_offset,
                              bool update_addr,
@@ -228,39 +266,10 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         port_min = inet_getport(e);
         port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
-#ifdef IPV6_V6ONLY
-            /*
-             * Deals with first & last cases in matrix in comment
-             * for inet_ai_family_from_address().
-             */
-            int v6only =
-                ((!saddr->has_ipv4 && !saddr->has_ipv6) ||
-                 (saddr->has_ipv4 && saddr->ipv4 &&
-                  saddr->has_ipv6 && saddr->ipv6)) ? 0 : 1;
-#endif
             inet_setport(e, p);
-#ifdef IPV6_V6ONLY
-        rebind:
-            if (e->ai_family == PF_INET6) {
-                qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &v6only,
-                                sizeof(v6only));
-            }
-#endif
-            if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) {
+            if (try_bind(slisten, saddr, e) >= 0) {
                 goto listen;
             }
-
-#ifdef IPV6_V6ONLY
-            /* If we got EADDRINUSE from an IPv6 bind & V6ONLY is unset,
-             * it could be that the IPv4 port is already claimed, so retry
-             * with V6ONLY set
-             */
-            if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) {
-                v6only = 1;
-                goto rebind;
-            }
-#endif
-
             if (p == port_max) {
                 if (!e->ai_next) {
                     error_setg_errno(errp, errno, "Failed to bind socket");
-- 
git-series 0.9.1

[Qemu-devel] [PATCH v6 3/4] sockets: factor out create_fast_reuse_socket

[Knut Omang]

Another refactoring step to prepare for fixing the problem
exposed with the test-listen test in the previous commit

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 util/qemu-sockets.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b4a2f24..d0d2047 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -149,6 +149,16 @@ int inet_ai_family_from_address(InetSocketAddress *addr,
     return PF_UNSPEC;
 }
 
+static int create_fast_reuse_socket(struct addrinfo *e)
+{
+    int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
+    if (slisten < 0) {
+        return -1;
+    }
+    socket_set_fast_reuse(slisten);
+    return slisten;
+}
+
 static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e)
 {
 #ifndef IPV6_V6ONLY
@@ -253,7 +263,8 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
 		        uaddr,INET6_ADDRSTRLEN,uport,32,
 		        NI_NUMERICHOST | NI_NUMERICSERV);
-        slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
+
+        slisten = create_fast_reuse_socket(e);
         if (slisten < 0) {
             if (!e->ai_next) {
                 error_setg_errno(errp, errno, "Failed to create socket");
@@ -261,8 +272,6 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
             continue;
         }
 
-        socket_set_fast_reuse(slisten);
-
         port_min = inet_getport(e);
         port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
-- 
git-series 0.9.1

[Qemu-devel] [PATCH v6 4/4] sockets: Handle race condition between binds to the same port

[Knut Omang]

If an offset of ports is specified to the inet_listen_saddr function(),
and two or more processes tries to bind from these ports at the same time,
occasionally more than one process may be able to bind to the same
port. The condition is detected by listen() but too late to avoid a failure.

This function is called by socket_listen() and used
by all socket listening code in QEMU, so all cases where any form of dynamic
port selection is used should be subject to this issue.

Add code to close and re-establish the socket when this
condition is observed, hiding the race condition from the user.

Also clean up some issues with error handling to allow more
accurate reporting of the cause of an error.

This has been developed and tested by means of the
test-listen unit test in the previous commit.
Enable the test for make check now that it passes.

Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 tests/Makefile.include |  2 +-
 util/qemu-sockets.c    | 58 ++++++++++++++++++++++++++++++-------------
 2 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index b37c0c8..9d2131d 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -128,7 +128,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
 gcov-files-check-bufferiszero-y = util/bufferiszero.c
 check-unit-y += tests/test-uuid$(EXESUF)
 check-unit-y += tests/ptimer-test$(EXESUF)
-#check-unit-y += tests/test-listen$(EXESUF)
+check-unit-y += tests/test-listen$(EXESUF)
 gcov-files-ptimer-test-y = hw/core/ptimer.c
 check-unit-y += tests/test-qapi-util$(EXESUF)
 gcov-files-test-qapi-util-y = qapi/qapi-util.c
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d0d2047..6a511fb 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -206,7 +206,10 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
     char port[33];
     char uaddr[INET6_ADDRSTRLEN+1];
     char uport[33];
-    int slisten, rc, port_min, port_max, p;
+    int rc, port_min, port_max, p;
+    int slisten = 0;
+    int saved_errno = 0;
+    bool socket_created = false;
     Error *err = NULL;
 
     memset(&ai,0, sizeof(ai));
@@ -258,7 +261,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         return -1;
     }
 
-    /* create socket + bind */
+    /* create socket + bind/listen */
     for (e = res; e != NULL; e = e->ai_next) {
         getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
 		        uaddr,INET6_ADDRSTRLEN,uport,32,
@@ -266,37 +269,58 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
 
         slisten = create_fast_reuse_socket(e);
         if (slisten < 0) {
-            if (!e->ai_next) {
-                error_setg_errno(errp, errno, "Failed to create socket");
-            }
             continue;
         }
 
+        socket_created = true;
         port_min = inet_getport(e);
         port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
             inet_setport(e, p);
-            if (try_bind(slisten, saddr, e) >= 0) {
-                goto listen;
-            }
-            if (p == port_max) {
-                if (!e->ai_next) {
+            rc = try_bind(slisten, saddr, e);
+            if (rc) {
+                if (errno == EADDRINUSE) {
+                    continue;
+                } else {
                     error_setg_errno(errp, errno, "Failed to bind socket");
+                    goto listen_failed;
                 }
             }
+            if (!listen(slisten, 1)) {
+                goto listen_ok;
+            }
+            if (errno != EADDRINUSE) {
+                error_setg_errno(errp, errno, "Failed to listen on socket");
+                goto listen_failed;
+            }
+            /* Someone else managed to bind to the same port and beat us
+             * to listen on it! Socket semantics does not allow us to
+             * recover from this situation, so we need to recreate the
+             * socket to allow bind attempts for subsequent ports:
+             */
+            closesocket(slisten);
+            slisten = create_fast_reuse_socket(e);
+            if (slisten < 0) {
+                error_setg_errno(errp, errno,
+                                 "Failed to recreate failed listening socket");
+                goto listen_failed;
+            }
         }
+    }
+    error_setg_errno(errp, errno,
+                     socket_created ?
+                     "Failed to find an available port" :
+                     "Failed to create a socket");
+listen_failed:
+    saved_errno = errno;
+    if (slisten >= 0) {
         closesocket(slisten);
     }
     freeaddrinfo(res);
+    errno = saved_errno;
     return -1;
 
-listen:
-    if (listen(slisten,1) != 0) {
-        error_setg_errno(errp, errno, "Failed to listen on socket");
-        closesocket(slisten);
-        freeaddrinfo(res);
-        return -1;
-    }
+listen_ok:
     if (update_addr) {
         g_free(saddr->host);
         saddr->host = g_strdup(uaddr);
-- 
git-series 0.9.1

Re: [Qemu-devel] [PATCH v6 3/4] sockets: factor out create_fast_reuse_socket

[Daniel P. Berrange]

On Sat, Jul 29, 2017 at 11:18:17PM +0200, Knut Omang wrote:
> Another refactoring step to prepare for fixing the problem
> exposed with the test-listen test in the previous commit
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  util/qemu-sockets.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v6 4/4] sockets: Handle race condition between binds to the same port

[Daniel P. Berrange]

On Sat, Jul 29, 2017 at 11:18:18PM +0200, Knut Omang wrote:
> If an offset of ports is specified to the inet_listen_saddr function(),
> and two or more processes tries to bind from these ports at the same time,
> occasionally more than one process may be able to bind to the same
> port. The condition is detected by listen() but too late to avoid a failure.
> 
> This function is called by socket_listen() and used
> by all socket listening code in QEMU, so all cases where any form of dynamic
> port selection is used should be subject to this issue.
> 
> Add code to close and re-establish the socket when this
> condition is observed, hiding the race condition from the user.
> 
> Also clean up some issues with error handling to allow more
> accurate reporting of the cause of an error.
> 
> This has been developed and tested by means of the
> test-listen unit test in the previous commit.
> Enable the test for make check now that it passes.
> 
> Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  tests/Makefile.include |  2 +-
>  util/qemu-sockets.c    | 58 ++++++++++++++++++++++++++++++-------------
>  2 files changed, 42 insertions(+), 18 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v6 1/4] tests: Add test-listen - a stress test for QEMU socket listen

[Daniel P. Berrange]

On Sat, Jul 29, 2017 at 11:18:15PM +0200, Knut Omang wrote:
> There's a potential race condition between multiple bind()'s
> attempting to bind to the same port, which occasionally
> allows more than one bind to succeed against the same port.
> 
> When a subsequent listen() call is made with the same socket
> only one will succeed.
> 
> The current QEMU code does however not take this situation into account
> and the listen will cause the code to break out and fail even
> when there are actually available ports to use.
> 
> This test exposes two subtests:
> 
> /socket/listen-serial
> /socket/listen-compete
> 
> The "compete" subtest creates a number of threads and have them all trying to bind
> to the same port with a large enough offset input to
> allow all threads to get it's own port.
> The "serial" subtest just does the same, except in series in a
> single thread.
> 
> The serial version passes, probably in most versions of QEMU.
> 
> The parallel version exposes the problem in a relatively reliable way,
> eg. it fails a majority of times, but not with a 100% rate, occasional
> passes can be seen. Nevertheless this is quite good given that
> the bug was tricky to reproduce and has been left undetected for
> a while.
> 
> The problem seems to be present in all versions of QEMU.
> 
> The original failure scenario occurred with VNC port allocation
> in a traditional Xen based build, in different code
> but with similar functionality.
> 
> Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
> ---
>  tests/Makefile.include |   2 +-
>  tests/test-listen.c    | 253 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 255 insertions(+)
>  create mode 100644 tests/test-listen.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 7af278d..b37c0c8 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -128,6 +128,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
>  gcov-files-check-bufferiszero-y = util/bufferiszero.c
>  check-unit-y += tests/test-uuid$(EXESUF)
>  check-unit-y += tests/ptimer-test$(EXESUF)
> +#check-unit-y += tests/test-listen$(EXESUF)
>  gcov-files-ptimer-test-y = hw/core/ptimer.c
>  check-unit-y += tests/test-qapi-util$(EXESUF)
>  gcov-files-test-qapi-util-y = qapi/qapi-util.c
> @@ -769,6 +770,7 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
>  tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
>  tests/numa-test$(EXESUF): tests/numa-test.o
>  tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
> +tests/test-listen$(EXESUF): tests/test-listen.o $(test-util-obj-y)
>  
>  tests/migration/stress$(EXESUF): tests/migration/stress.o
>  	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
> diff --git a/tests/test-listen.c b/tests/test-listen.c
> new file mode 100644
> index 0000000..5c07537
> --- /dev/null
> +++ b/tests/test-listen.c
> @@ -0,0 +1,253 @@
> +/*
> + * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
> + *    Author: Knut Omang <knut.omang@oracle.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.

Can you change that to "version 2 or later" - per the LICENSE file, we don't
accept contributions under "version 2 only" except for 4 specific subdirs:


  "As of July 2013, contributions under version 2 of the GNU General Public
   License (and no later version) are only accepted for the following files
   or directories: bsd-user/, linux-user/, hw/vfio/, hw/xen/xen_pt*."


> + *
> + * Test parallel port listen configuration with
> + * dynamic port allocation
> + */


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v6 1/4] tests: Add test-listen - a stress test for QEMU socket listen

[Knut Omang]

On Mon, 2017-08-07 at 09:45 +0100, Daniel P. Berrange wrote:
> On Sat, Jul 29, 2017 at 11:18:15PM +0200, Knut Omang wrote:
> > There's a potential race condition between multiple bind()'s
> > attempting to bind to the same port, which occasionally
> > allows more than one bind to succeed against the same port.
> > 
> > When a subsequent listen() call is made with the same socket
> > only one will succeed.
> > 
> > The current QEMU code does however not take this situation into account
> > and the listen will cause the code to break out and fail even
> > when there are actually available ports to use.
> > 
> > This test exposes two subtests:
> > 
> > /socket/listen-serial
> > /socket/listen-compete
> > 
> > The "compete" subtest creates a number of threads and have them all trying to bind
> > to the same port with a large enough offset input to
> > allow all threads to get it's own port.
> > The "serial" subtest just does the same, except in series in a
> > single thread.
> > 
> > The serial version passes, probably in most versions of QEMU.
> > 
> > The parallel version exposes the problem in a relatively reliable way,
> > eg. it fails a majority of times, but not with a 100% rate, occasional
> > passes can be seen. Nevertheless this is quite good given that
> > the bug was tricky to reproduce and has been left undetected for
> > a while.
> > 
> > The problem seems to be present in all versions of QEMU.
> > 
> > The original failure scenario occurred with VNC port allocation
> > in a traditional Xen based build, in different code
> > but with similar functionality.
> > 
> > Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> > Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> > Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
> > ---
> >  tests/Makefile.include |   2 +-
> >  tests/test-listen.c    | 253 ++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 255 insertions(+)
> >  create mode 100644 tests/test-listen.c
> > 
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index 7af278d..b37c0c8 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -128,6 +128,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
> >  gcov-files-check-bufferiszero-y = util/bufferiszero.c
> >  check-unit-y += tests/test-uuid$(EXESUF)
> >  check-unit-y += tests/ptimer-test$(EXESUF)
> > +#check-unit-y += tests/test-listen$(EXESUF)
> >  gcov-files-ptimer-test-y = hw/core/ptimer.c
> >  check-unit-y += tests/test-qapi-util$(EXESUF)
> >  gcov-files-test-qapi-util-y = qapi/qapi-util.c
> > @@ -769,6 +770,7 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
> >  tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
> >  tests/numa-test$(EXESUF): tests/numa-test.o
> >  tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
> > +tests/test-listen$(EXESUF): tests/test-listen.o $(test-util-obj-y)
> >  
> >  tests/migration/stress$(EXESUF): tests/migration/stress.o
> >  	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
> > diff --git a/tests/test-listen.c b/tests/test-listen.c
> > new file mode 100644
> > index 0000000..5c07537
> > --- /dev/null
> > +++ b/tests/test-listen.c
> > @@ -0,0 +1,253 @@
> > +/*
> > + * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
> > + *    Author: Knut Omang <knut.omang@oracle.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2
> > + * as published by the Free Software Foundation.
> 
> Can you change that to "version 2 or later" - per the LICENSE file, we don't
> accept contributions under "version 2 only" except for 4 specific subdirs:
> 
> 
>   "As of July 2013, contributions under version 2 of the GNU General Public
>    License (and no later version) are only accepted for the following files
>    or directories: bsd-user/, linux-user/, hw/vfio/, hw/xen/xen_pt*."

Oh, sorry - I wasn't aware of this, +"...or later" is fine with me.
Would you like me to send a v7 of the set with only that change, or can you amend 
it as part of the merge?

Thanks,
Knut

> 
> > + *
> > + * Test parallel port listen configuration with
> > + * dynamic port allocation
> > + */
> 
> 
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH v6 1/4] tests: Add test-listen - a stress test for QEMU socket listen

[Daniel P. Berrange]

On Mon, Aug 07, 2017 at 11:21:25AM +0200, Knut Omang wrote:
> On Mon, 2017-08-07 at 09:45 +0100, Daniel P. Berrange wrote:
> > On Sat, Jul 29, 2017 at 11:18:15PM +0200, Knut Omang wrote:
> > > There's a potential race condition between multiple bind()'s
> > > attempting to bind to the same port, which occasionally
> > > allows more than one bind to succeed against the same port.
> > > 
> > > When a subsequent listen() call is made with the same socket
> > > only one will succeed.
> > > 
> > > The current QEMU code does however not take this situation into account
> > > and the listen will cause the code to break out and fail even
> > > when there are actually available ports to use.
> > > 
> > > This test exposes two subtests:
> > > 
> > > /socket/listen-serial
> > > /socket/listen-compete
> > > 
> > > The "compete" subtest creates a number of threads and have them all trying to bind
> > > to the same port with a large enough offset input to
> > > allow all threads to get it's own port.
> > > The "serial" subtest just does the same, except in series in a
> > > single thread.
> > > 
> > > The serial version passes, probably in most versions of QEMU.
> > > 
> > > The parallel version exposes the problem in a relatively reliable way,
> > > eg. it fails a majority of times, but not with a 100% rate, occasional
> > > passes can be seen. Nevertheless this is quite good given that
> > > the bug was tricky to reproduce and has been left undetected for
> > > a while.
> > > 
> > > The problem seems to be present in all versions of QEMU.
> > > 
> > > The original failure scenario occurred with VNC port allocation
> > > in a traditional Xen based build, in different code
> > > but with similar functionality.
> > > 
> > > Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> > > Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> > > Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
> > > ---
> > >  tests/Makefile.include |   2 +-
> > >  tests/test-listen.c    | 253 ++++++++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 255 insertions(+)
> > >  create mode 100644 tests/test-listen.c
> > > 
> > > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > > index 7af278d..b37c0c8 100644
> > > --- a/tests/Makefile.include
> > > +++ b/tests/Makefile.include
> > > @@ -128,6 +128,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
> > >  gcov-files-check-bufferiszero-y = util/bufferiszero.c
> > >  check-unit-y += tests/test-uuid$(EXESUF)
> > >  check-unit-y += tests/ptimer-test$(EXESUF)
> > > +#check-unit-y += tests/test-listen$(EXESUF)
> > >  gcov-files-ptimer-test-y = hw/core/ptimer.c
> > >  check-unit-y += tests/test-qapi-util$(EXESUF)
> > >  gcov-files-test-qapi-util-y = qapi/qapi-util.c
> > > @@ -769,6 +770,7 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
> > >  tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
> > >  tests/numa-test$(EXESUF): tests/numa-test.o
> > >  tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
> > > +tests/test-listen$(EXESUF): tests/test-listen.o $(test-util-obj-y)
> > >  
> > >  tests/migration/stress$(EXESUF): tests/migration/stress.o
> > >  	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
> > > diff --git a/tests/test-listen.c b/tests/test-listen.c
> > > new file mode 100644
> > > index 0000000..5c07537
> > > --- /dev/null
> > > +++ b/tests/test-listen.c
> > > @@ -0,0 +1,253 @@
> > > +/*
> > > + * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
> > > + *    Author: Knut Omang <knut.omang@oracle.com>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2
> > > + * as published by the Free Software Foundation.
> > 
> > Can you change that to "version 2 or later" - per the LICENSE file, we don't
> > accept contributions under "version 2 only" except for 4 specific subdirs:
> > 
> > 
> >   "As of July 2013, contributions under version 2 of the GNU General Public
> >    License (and no later version) are only accepted for the following files
> >    or directories: bsd-user/, linux-user/, hw/vfio/, hw/xen/xen_pt*."
> 
> Oh, sorry - I wasn't aware of this, +"...or later" is fine with me.
> Would you like me to send a v7 of the set with only that change, or can you amend 
> it as part of the merge?

Since its a copyright statement, I'd prefer if you just sent a v7. I've no
other comments, so will queue it for merge once you resend, and send a pull
request once 2.11 opens up.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v6 1/4] tests: Add test-listen - a stress test for QEMU socket listen

[Knut Omang]

On Mon, 2017-08-07 at 11:27 +0100, Daniel P. Berrange wrote:
> On Mon, Aug 07, 2017 at 11:21:25AM +0200, Knut Omang wrote:
> > On Mon, 2017-08-07 at 09:45 +0100, Daniel P. Berrange wrote:
> > > On Sat, Jul 29, 2017 at 11:18:15PM +0200, Knut Omang wrote:
> > > > There's a potential race condition between multiple bind()'s
> > > > attempting to bind to the same port, which occasionally
> > > > allows more than one bind to succeed against the same port.
> > > > 
> > > > When a subsequent listen() call is made with the same socket
> > > > only one will succeed.
> > > > 
> > > > The current QEMU code does however not take this situation into account
> > > > and the listen will cause the code to break out and fail even
> > > > when there are actually available ports to use.
> > > > 
> > > > This test exposes two subtests:
> > > > 
> > > > /socket/listen-serial
> > > > /socket/listen-compete
> > > > 
> > > > The "compete" subtest creates a number of threads and have them all trying to bind
> > > > to the same port with a large enough offset input to
> > > > allow all threads to get it's own port.
> > > > The "serial" subtest just does the same, except in series in a
> > > > single thread.
> > > > 
> > > > The serial version passes, probably in most versions of QEMU.
> > > > 
> > > > The parallel version exposes the problem in a relatively reliable way,
> > > > eg. it fails a majority of times, but not with a 100% rate, occasional
> > > > passes can be seen. Nevertheless this is quite good given that
> > > > the bug was tricky to reproduce and has been left undetected for
> > > > a while.
> > > > 
> > > > The problem seems to be present in all versions of QEMU.
> > > > 
> > > > The original failure scenario occurred with VNC port allocation
> > > > in a traditional Xen based build, in different code
> > > > but with similar functionality.
> > > > 
> > > > Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> > > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> > > > Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> > > > Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
> > > > ---
> > > >  tests/Makefile.include |   2 +-
> > > >  tests/test-listen.c    | 253 ++++++++++++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 255 insertions(+)
> > > >  create mode 100644 tests/test-listen.c
> > > > 
> > > > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > > > index 7af278d..b37c0c8 100644
> > > > --- a/tests/Makefile.include
> > > > +++ b/tests/Makefile.include
> > > > @@ -128,6 +128,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
> > > >  gcov-files-check-bufferiszero-y = util/bufferiszero.c
> > > >  check-unit-y += tests/test-uuid$(EXESUF)
> > > >  check-unit-y += tests/ptimer-test$(EXESUF)
> > > > +#check-unit-y += tests/test-listen$(EXESUF)
> > > >  gcov-files-ptimer-test-y = hw/core/ptimer.c
> > > >  check-unit-y += tests/test-qapi-util$(EXESUF)
> > > >  gcov-files-test-qapi-util-y = qapi/qapi-util.c
> > > > @@ -769,6 +770,7 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
> > > >  tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
> > > >  tests/numa-test$(EXESUF): tests/numa-test.o
> > > >  tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
> > > > +tests/test-listen$(EXESUF): tests/test-listen.o $(test-util-obj-y)
> > > >  
> > > >  tests/migration/stress$(EXESUF): tests/migration/stress.o
> > > >  	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
> > > > diff --git a/tests/test-listen.c b/tests/test-listen.c
> > > > new file mode 100644
> > > > index 0000000..5c07537
> > > > --- /dev/null
> > > > +++ b/tests/test-listen.c
> > > > @@ -0,0 +1,253 @@
> > > > +/*
> > > > + * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
> > > > + *    Author: Knut Omang <knut.omang@oracle.com>
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or modify
> > > > + * it under the terms of the GNU General Public License version 2
> > > > + * as published by the Free Software Foundation.
> > > 
> > > Can you change that to "version 2 or later" - per the LICENSE file, we don't
> > > accept contributions under "version 2 only" except for 4 specific subdirs:
> > > 
> > > 
> > >   "As of July 2013, contributions under version 2 of the GNU General Public
> > >    License (and no later version) are only accepted for the following files
> > >    or directories: bsd-user/, linux-user/, hw/vfio/, hw/xen/xen_pt*."
> > 
> > Oh, sorry - I wasn't aware of this, +"...or later" is fine with me.
> > Would you like me to send a v7 of the set with only that change, or can you amend 
> > it as part of the merge?
> 
> Since its a copyright statement, I'd prefer if you just sent a v7. I've no
> other comments, so will queue it for merge once you resend, and send a pull
> request once 2.11 opens up.

Ok, will do,

Thanks,
Knut

> Regards,
> Daniel