| Message ID | 20171106153342.24147-3-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Peter, Seems this test was lost along the way? I thought it got merged with Daniel's fix to the potential leak that the test exposed in your build, but it seems not? Thanks, Knut On Mon, 2017-11-06 at 15:33 +0000, Daniel P. Berrange wrote: > From: Knut Omang <knut.omang@oracle.com> > > 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> > Signed-off-by: Daniel P. Berrange <berrange@redhat.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 434a2ce868..e4bb88bd3d 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -154,6 +154,7 @@ gcov-files-check-bufferiszero-y = util/bufferiszero.c > check-unit-y += tests/test-uuid$(EXESUF) > check-unit-y += tests/ptimer-test$(EXESUF) > gcov-files-ptimer-test-y = hw/core/ptimer.c > +check-unit-y += tests/test-listen$(EXESUF) > check-unit-y += tests/test-qapi-util$(EXESUF) > gcov-files-test-qapi-util-y = qapi/qapi-util.c > > @@ -804,6 +805,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 0000000000..03c4c8f03b > --- /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 or later > + * 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(); > +}
On Tue, May 01, 2018 at 04:00:35PM +0200, Knut Omang wrote: > Hi Peter, > > Seems this test was lost along the way? > > I thought it got merged with Daniel's fix to the potential leak > that the test exposed in your build, but it seems not? I've never been able to get this test, nor another one of my own to succesfully pass automated testing in all the various test envs Peter and our many CI systems use. Regards, Daniel
On Tue, 2018-05-01 at 15:07 +0100, Daniel P. Berrangé wrote: > On Tue, May 01, 2018 at 04:00:35PM +0200, Knut Omang wrote: > > Hi Peter, > > > > Seems this test was lost along the way? > > > > I thought it got merged with Daniel's fix to the potential leak > > that the test exposed in your build, but it seems not? > > I've never been able to get this test, nor another one of my own > to succesfully pass automated testing in all the various test > envs Peter and our many CI systems use. Ok, that explains it - no big deal from my side, I just thought it was "value lost" against future errors. Would it make sense to let the test check for environment and just really run in cases where it reliably passes? That will also serve as documentation as to where there might be additional issues hidden? Knut
On Tue, May 01, 2018 at 04:21:01PM +0200, Knut Omang wrote: > On Tue, 2018-05-01 at 15:07 +0100, Daniel P. Berrangé wrote: > > On Tue, May 01, 2018 at 04:00:35PM +0200, Knut Omang wrote: > > > Hi Peter, > > > > > > Seems this test was lost along the way? > > > > > > I thought it got merged with Daniel's fix to the potential leak > > > that the test exposed in your build, but it seems not? > > > > I've never been able to get this test, nor another one of my own > > to succesfully pass automated testing in all the various test > > envs Peter and our many CI systems use. > > Ok, that explains it - no big deal from my side, I just thought > it was "value lost" against future errors. > > Would it make sense to let the test check for environment and just > really run in cases where it reliably passes? That's why I've actually been trying to do - I hacked some checks into the start of the test suite that were supposed to detect if it can run reliably but it still got failures. Because of the large number of socketss it opens it was hitting FD resource limits at times IIRC FWIW, I still have the patch on my local queue and hope one day I'll have an insight to fix it enough to merge... Regards, Daniel
diff --git a/tests/Makefile.include b/tests/Makefile.include index 434a2ce868..e4bb88bd3d 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -154,6 +154,7 @@ gcov-files-check-bufferiszero-y = util/bufferiszero.c check-unit-y += tests/test-uuid$(EXESUF) check-unit-y += tests/ptimer-test$(EXESUF) gcov-files-ptimer-test-y = hw/core/ptimer.c +check-unit-y += tests/test-listen$(EXESUF) check-unit-y += tests/test-qapi-util$(EXESUF) gcov-files-test-qapi-util-y = qapi/qapi-util.c @@ -804,6 +805,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 0000000000..03c4c8f03b --- /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 or later + * 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(); +}