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

[Qemu-devel] [PATCH v5 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 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 create_fast_reuse_socket
  sockets: factor out a new try_bind() function
  sockets: Handle race condition between binds to the same port

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

base-commit: 91939262ffcd3c85ea6a4793d3029326eea1d649
-- 
git-series 0.9.1

[Qemu-devel] [PATCH v5 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 v5 2/4] sockets: factor out create_fast_reuse_socket

[Knut Omang]

First 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 | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 1358c81..578f25b 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr,
     return PF_UNSPEC;
 }
 
+static int create_fast_reuse_socket(struct addrinfo *e, Error **errp)
+{
+    int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
+    if (slisten < 0) {
+        if (!e->ai_next) {
+            error_setg_errno(errp, errno, "Failed to create socket");
+        }
+        return -1;
+    }
+
+    socket_set_fast_reuse(slisten);
+    return slisten;
+}
+
 static int inet_listen_saddr(InetSocketAddress *saddr,
                              int port_offset,
                              bool update_addr,
@@ -210,21 +224,17 @@ 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,
 		        NI_NUMERICHOST | NI_NUMERICSERV);
-        slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
+
+        slisten = create_fast_reuse_socket(e, &err);
         if (slisten < 0) {
-            if (!e->ai_next) {
-                error_setg_errno(errp, errno, "Failed to create socket");
-            }
             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 v5 3/4] sockets: factor out a new try_bind() function

[Knut Omang]

Another refactoring step to prepare for the problem
exposed by the test-listen test.

This time 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>
---
 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 578f25b..39f207c 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -163,6 +163,44 @@ static int create_fast_reuse_socket(struct addrinfo *e, Error **errp)
     return slisten;
 }
 
+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,
@@ -238,39 +276,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 v5 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.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
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    | 51 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 39 insertions(+), 14 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 39f207c..8ca3ba6 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -210,7 +210,9 @@ 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;
     Error *err = NULL;
 
     memset(&ai,0, sizeof(ai));
@@ -277,27 +279,50 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         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;
+                }
+            }
+            rc = listen(slisten, 1);
+            if (!rc) {
+                goto listen_ok;
+            } else if (errno == EADDRINUSE) {
+                /* 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, errp);
+                if (slisten >= 0) {
+                    continue;
                 }
             }
+            error_setg_errno(errp, errno, "Failed to listen on socket");
+            goto listen_failed;
         }
+    }
+    if (err) {
+        error_propagate(errp, err);
+    } else {
+        error_setg_errno(errp, errno, "Failed to find an available port");
+    }
+
+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 v5 2/4] sockets: factor out create_fast_reuse_socket

[Daniel P. Berrange]

On Sat, Jul 22, 2017 at 09:49:31AM +0200, Knut Omang wrote:
> First 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 | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 1358c81..578f25b 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr,
>      return PF_UNSPEC;
>  }
>  
> +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp)
> +{
> +    int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
> +    if (slisten < 0) {
> +        if (!e->ai_next) {
> +            error_setg_errno(errp, errno, "Failed to create socket");
> +        }
> +        return -1;
> +    }
> +
> +    socket_set_fast_reuse(slisten);
> +    return slisten;
> +}

As mentioned in the previous review, I don't think we should have
methods like this which sometimes report an error on failure, and
sometimes don't report an error. It makes it hard to review the
callers for correctness of error handling. 

> +
>  static int inet_listen_saddr(InetSocketAddress *saddr,
>                               int port_offset,
>                               bool update_addr,
> @@ -210,21 +224,17 @@ 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,
>  		        NI_NUMERICHOST | NI_NUMERICSERV);
> -        slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
> +
> +        slisten = create_fast_reuse_socket(e, &err);
>          if (slisten < 0) {
> -            if (!e->ai_next) {
> -                error_setg_errno(errp, errno, "Failed to create socket");
> -            }

So please leave this here.

>              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++) {


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 v5 3/4] sockets: factor out a new try_bind() function

[Daniel P. Berrange]

On Sat, Jul 22, 2017 at 09:49:32AM +0200, Knut Omang wrote:
> Another refactoring step to prepare for the problem
> exposed by the test-listen test.
> 
> This time 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>
> ---
>  util/qemu-sockets.c | 69 ++++++++++++++++++++++++++--------------------
>  1 file changed, 39 insertions(+), 30 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 v5 4/4] sockets: Handle race condition between binds to the same port

[Daniel P. Berrange]

On Sat, Jul 22, 2017 at 09:49:33AM +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.
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> 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    | 51 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 39 insertions(+), 14 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 39f207c..8ca3ba6 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -210,7 +210,9 @@ 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;
>      Error *err = NULL;
>  
>      memset(&ai,0, sizeof(ai));
> @@ -277,27 +279,50 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>          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) {

Add in  '&& e->ai_next' here, so we trigger the error handling
on the else branch if this was the last address we had.

> +                    continue;
> +                } else {
>                      error_setg_errno(errp, errno, "Failed to bind socket");
> +                    goto listen_failed;
> +                }
> +            }
> +            rc = listen(slisten, 1);
> +            if (!rc) {

Assigning to rc seems redundant here.

> +                goto listen_ok;
> +            } else if (errno == EADDRINUSE) {
> +                /* 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, errp);

create_fast_reuse_socket may report an error here

> +                if (slisten >= 0) {
> +                    continue;
>                  }
>              }
> +            error_setg_errno(errp, errno, "Failed to listen on socket");

at which point you report another error on top of it with a stale
errno value here. This bug would be more obvious if error reporting
was pulled out of create_fast_reuse_socket() as described in the
earlier patch.

I think it'd be clearer if we reduced the nesting here. eg

     if (listen() == 0) {
        goto listen_ok;
     }
     if (errno != EADDRINUSE) {
         error_setg_error(errp, errno, "....");
	 goto listen_failed;
     }
     closesocket(listen);
     slisten = create_fast_reuse_socket(e);
     if (slisten < 0 && !e->ai_next) {
         error_setg_error(errp, errno, "....");
	 goto listen_failed;
     }

> +            goto listen_failed;
>          }
> +    }
> +    if (err) {
> +        error_propagate(errp, err);
> +    } else {
> +        error_setg_errno(errp, errno, "Failed to find an available port");
> +    }

if we tidied up the first call to create_fast_reuse_socket() too, it would
remove the need for this.

> +
> +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);

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 v5 2/4] sockets: factor out create_fast_reuse_socket

[Knut Omang]

On Tue, 2017-07-25 at 10:38 +0100, Daniel P. Berrange wrote:
> On Sat, Jul 22, 2017 at 09:49:31AM +0200, Knut Omang wrote:
> > First 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 | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 1358c81..578f25b 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr,
> >      return PF_UNSPEC;
> >  }
> >  
> > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp)
> > +{
> > +    int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
> > +    if (slisten < 0) {
> > +        if (!e->ai_next) {
> > +            error_setg_errno(errp, errno, "Failed to create socket");
> > +        }
> > +        return -1;
> > +    }
> > +
> > +    socket_set_fast_reuse(slisten);
> > +    return slisten;
> > +}
> 
> As mentioned in the previous review, I don't think we should have
> methods like this which sometimes report an error on failure, and
> sometimes don't report an error. It makes it hard to review the
> callers for correctness of error handling. 

Sorry, somehow this one slipped through..

> 
> > +
> >  static int inet_listen_saddr(InetSocketAddress *saddr,
> >                               int port_offset,
> >                               bool update_addr,
> > @@ -210,21 +224,17 @@ 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,
> >  		        NI_NUMERICHOST | NI_NUMERICSERV);
> > -        slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
> > +
> > +        slisten = create_fast_reuse_socket(e, &err);
> >          if (slisten < 0) {
> > -            if (!e->ai_next) {
> > -                error_setg_errno(errp, errno, "Failed to create socket");
> > -            }
> 
> So please leave this here.

Since we are already outside of the scope of the original patch, 
let me suggest:

As far as I can see there's no good reason to have 
different behavior if creating sockets on the last 'struct addrinfo'
in the list fails than if an intermediate addrinfo fails and the next 
addrinfo in the list just happened to have no free ports.

As far as I can see the 'Failed to create socket' message will be
most useful to the user if creating a socket fails on all 
elements in the list, to detect issues with the network setup.

I have implemented this semantics in v6 - it should be an enhancement while 
also simplifying the code inside the for loops, getting rid  
of the conditional error setting and avoid code duplication by 
handling the same error twice in proximity.

Thanks,
Knut

> 
> >              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++) {
> 
> 
> Regards,
> Daniel

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

[Knut Omang]

On Tue, 2017-07-25 at 10:54 +0100, Daniel P. Berrange wrote:
> On Sat, Jul 22, 2017 at 09:49:33AM +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.
> > 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > 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    | 51 ++++++++++++++++++++++++++++++++-----------
> >  2 files changed, 39 insertions(+), 14 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 39f207c..8ca3ba6 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -210,7 +210,9 @@ 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;
> >      Error *err = NULL;
> >  
> >      memset(&ai,0, sizeof(ai));
> > @@ -277,27 +279,50 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
> >          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) {
> 
> Add in  '&& e->ai_next' here, so we trigger the error handling
> on the else branch if this was the last address we had.

See my comment on patch 2 - with that reporting semantics, inability to
create a socket will only be reported if none of the addrinfo structs
allowed socket creation so this reporting case goes away.

> > +                    continue;
> > +                } else {
> >                      error_setg_errno(errp, errno, "Failed to bind socket");
> > +                    goto listen_failed;
> > +                }
> > +            }
> > +            rc = listen(slisten, 1);
> > +            if (!rc) {
> 
> Assigning to rc seems redundant here.

removed.

> > +                goto listen_ok;
> > +            } else if (errno == EADDRINUSE) {
> > +                /* 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, errp);
> 
> create_fast_reuse_socket may report an error here
> 
> > +                if (slisten >= 0) {
> > +                    continue;
> >                  }
> >              }
> > +            error_setg_errno(errp, errno, "Failed to listen on socket");
> 
> at which point you report another error on top of it with a stale
> errno value here. This bug would be more obvious if error reporting
> was pulled out of create_fast_reuse_socket() as described in the
> earlier patch.
> 
> I think it'd be clearer if we reduced the nesting here. eg
> 
>      if (listen() == 0) {
>         goto listen_ok;
>      }
>      if (errno != EADDRINUSE) {
>          error_setg_error(errp, errno, "....");
> 	 goto listen_failed;
>      }
>      closesocket(listen);
>      slisten = create_fast_reuse_socket(e);
>      if (slisten < 0 && !e->ai_next) {
>          error_setg_error(errp, errno, "....");
> 	 goto listen_failed;
>      }

Good idea to push the != case up, done!

> > +            goto listen_failed;
> >          }
> > +    }
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +    } else {
> > +        error_setg_errno(errp, errno, "Failed to find an available port");
> > +    }
> 
> if we tidied up the first call to create_fast_reuse_socket() too, it would
> remove the need for this.

I think v6 now solves this in a better way by separating between 
whether one or more ports have actually been tried and the case where no 
valid/working addrinfo records have been supplied.

Thanks,
Knut
> 
> > +
> > +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);
> 
> Regards,
> Daniel