* [PATCH qemu-traditional] qemu: Fix race condition when binding ports
@ 2013-12-09 13:10 Andrew Cooper
2013-12-09 13:17 ` George Dunlap
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2013-12-09 13:10 UTC (permalink / raw)
To: Xen-devel
Cc: George Dunlap, Andrew Cooper, Ian Jackson, Ian Campbell,
Stefano Stabellini
Two Qemus can race to bind the same VNC port. It is valid for multiple
listen()s on the same socket to succeed, but only the first bind() will
succeed.
In the case that two Qemus are racing, the second one will fail with an
EADDRINUSE, and bail with a fatal error which renders the domain functionally
useless.
In the case that bind() fails with EADDRINUSE, rebind the socket again and try
for the next port.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
This fix has been in XenServer for two and a half years
---
qemu-sockets.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 9b9fa77..e54f6ad 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -163,6 +163,7 @@ int inet_listen(const char *str, char *ostr, int olen,
/* create socket + bind */
for (e = res; e != NULL; e = e->ai_next) {
+ rebind:
getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
uaddr,INET6_ADDRSTRLEN,uport,32,
NI_NUMERICHOST | NI_NUMERICSERV);
@@ -186,7 +187,20 @@ int inet_listen(const char *str, char *ostr, int olen,
if (sockets_debug)
fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__,
inet_strfamily(e->ai_family), uaddr, inet_getport(e));
- goto listen;
+ if (listen(slisten,1) == 0) {
+ goto listened;
+ } else {
+ int err = errno;
+
+ perror("listen");
+ closesocket(slisten);
+
+ if (err == EADDRINUSE)
+ goto rebind;
+ freeaddrinfo(res);
+ return -1;
+ }
+
}
try_next = to && (inet_getport(e) <= to + port_offset);
if (!try_next || sockets_debug)
@@ -205,12 +219,7 @@ int inet_listen(const char *str, char *ostr, int olen,
freeaddrinfo(res);
return -1;
-listen:
- if (listen(slisten,1) != 0) {
- perror("listen");
- closesocket(slisten);
- return -1;
- }
+listened:
if (ostr) {
if (e->ai_family == PF_INET6) {
snprintf(ostr, olen, "[%s]:%d%s", uaddr,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH qemu-traditional] qemu: Fix race condition when binding ports
2013-12-09 13:10 [PATCH qemu-traditional] qemu: Fix race condition when binding ports Andrew Cooper
@ 2013-12-09 13:17 ` George Dunlap
2013-12-09 13:25 ` Andrew Cooper
2013-12-09 13:27 ` [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports Andrew Cooper
0 siblings, 2 replies; 9+ messages in thread
From: George Dunlap @ 2013-12-09 13:17 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Stefano Stabellini, Ian Jackson, Ian Campbell
On 12/09/2013 01:10 PM, Andrew Cooper wrote:
> Two Qemus can race to bind the same VNC port. It is valid for multiple
> listen()s on the same socket to succeed, but only the first bind() will
> succeed.
>
> In the case that two Qemus are racing, the second one will fail with an
> EADDRINUSE, and bail with a fatal error which renders the domain functionally
> useless.
>
> In the case that bind() fails with EADDRINUSE, rebind the socket again and try
> for the next port.
Sorry if I'm being a bit dense here, but it looks like you have those
mixed up: if listen() fails with EADDRINUSE, you're going back to call
bind() again; which would suggest that you can have multible bind()s but
only a single listen()? Or am I confused?
In any case, this is clearly a bug fix, and a very well tested one at
that, so:
Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
>
> ---
>
> This fix has been in XenServer for two and a half years
> ---
> qemu-sockets.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 9b9fa77..e54f6ad 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -163,6 +163,7 @@ int inet_listen(const char *str, char *ostr, int olen,
>
> /* create socket + bind */
> for (e = res; e != NULL; e = e->ai_next) {
> + rebind:
> getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
> uaddr,INET6_ADDRSTRLEN,uport,32,
> NI_NUMERICHOST | NI_NUMERICSERV);
> @@ -186,7 +187,20 @@ int inet_listen(const char *str, char *ostr, int olen,
> if (sockets_debug)
> fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__,
> inet_strfamily(e->ai_family), uaddr, inet_getport(e));
> - goto listen;
> + if (listen(slisten,1) == 0) {
> + goto listened;
> + } else {
> + int err = errno;
> +
> + perror("listen");
> + closesocket(slisten);
> +
> + if (err == EADDRINUSE)
> + goto rebind;
> + freeaddrinfo(res);
> + return -1;
> + }
> +
> }
> try_next = to && (inet_getport(e) <= to + port_offset);
> if (!try_next || sockets_debug)
> @@ -205,12 +219,7 @@ int inet_listen(const char *str, char *ostr, int olen,
> freeaddrinfo(res);
> return -1;
>
> -listen:
> - if (listen(slisten,1) != 0) {
> - perror("listen");
> - closesocket(slisten);
> - return -1;
> - }
> +listened:
> if (ostr) {
> if (e->ai_family == PF_INET6) {
> snprintf(ostr, olen, "[%s]:%d%s", uaddr,
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH qemu-traditional] qemu: Fix race condition when binding ports
2013-12-09 13:17 ` George Dunlap
@ 2013-12-09 13:25 ` Andrew Cooper
2013-12-09 13:27 ` [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports Andrew Cooper
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Cooper @ 2013-12-09 13:25 UTC (permalink / raw)
To: George Dunlap; +Cc: Stefano Stabellini, Ian Jackson, Ian Campbell, Xen-devel
On 09/12/13 13:17, George Dunlap wrote:
> On 12/09/2013 01:10 PM, Andrew Cooper wrote:
>> Two Qemus can race to bind the same VNC port. It is valid for multiple
>> listen()s on the same socket to succeed, but only the first bind() will
>> succeed.
>>
>> In the case that two Qemus are racing, the second one will fail with an
>> EADDRINUSE, and bail with a fatal error which renders the domain
>> functionally
>> useless.
>>
>> In the case that bind() fails with EADDRINUSE, rebind the socket
>> again and try
>> for the next port.
>
> Sorry if I'm being a bit dense here, but it looks like you have those
> mixed up: if listen() fails with EADDRINUSE, you're going back to call
> bind() again; which would suggest that you can have multible bind()s
> but only a single listen()? Or am I confused?
Nope - very definitely me who messed up the message. v2 on its way.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
2013-12-09 13:17 ` George Dunlap
2013-12-09 13:25 ` Andrew Cooper
@ 2013-12-09 13:27 ` Andrew Cooper
2013-12-09 13:36 ` Ian Campbell
` (2 more replies)
1 sibling, 3 replies; 9+ messages in thread
From: Andrew Cooper @ 2013-12-09 13:27 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Stefano Stabellini
Two Qemus can race to bind the same VNC port. It is valid for multiple
bind()s on the same socket to succeed, but only the first listen() will
succeed.
In the case that two Qemus are starting at the same time, and both trying to
grab the next free VNC port, the second one will fail with an EADDRINUSE, and
bail with a fatal error which renders the domain functionally useless.
In the case that listen() fails with EADDRINUSE, rebind the socket again and
try for the next port.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes in v2:
* Fix commit message.
This fix has been in XenServer for two and a half years
---
qemu-sockets.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 9b9fa77..e54f6ad 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -163,6 +163,7 @@ int inet_listen(const char *str, char *ostr, int olen,
/* create socket + bind */
for (e = res; e != NULL; e = e->ai_next) {
+ rebind:
getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
uaddr,INET6_ADDRSTRLEN,uport,32,
NI_NUMERICHOST | NI_NUMERICSERV);
@@ -186,7 +187,20 @@ int inet_listen(const char *str, char *ostr, int olen,
if (sockets_debug)
fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__,
inet_strfamily(e->ai_family), uaddr, inet_getport(e));
- goto listen;
+ if (listen(slisten,1) == 0) {
+ goto listened;
+ } else {
+ int err = errno;
+
+ perror("listen");
+ closesocket(slisten);
+
+ if (err == EADDRINUSE)
+ goto rebind;
+ freeaddrinfo(res);
+ return -1;
+ }
+
}
try_next = to && (inet_getport(e) <= to + port_offset);
if (!try_next || sockets_debug)
@@ -205,12 +219,7 @@ int inet_listen(const char *str, char *ostr, int olen,
freeaddrinfo(res);
return -1;
-listen:
- if (listen(slisten,1) != 0) {
- perror("listen");
- closesocket(slisten);
- return -1;
- }
+listened:
if (ostr) {
if (e->ai_family == PF_INET6) {
snprintf(ostr, olen, "[%s]:%d%s", uaddr,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
2013-12-09 13:27 ` [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports Andrew Cooper
@ 2013-12-09 13:36 ` Ian Campbell
2013-12-09 15:23 ` Frediano Ziglio
2013-12-10 15:28 ` Ian Jackson
2 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2013-12-09 13:36 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Stefano Stabellini, Ian Jackson, Xen-devel
On Mon, 2013-12-09 at 13:27 +0000, Andrew Cooper wrote:
> Two Qemus can race to bind the same VNC port. It is valid for multiple
> bind()s on the same socket to succeed, but only the first listen() will
> succeed.
>
> In the case that two Qemus are starting at the same time, and both trying to
> grab the next free VNC port, the second one will fail with an EADDRINUSE, and
> bail with a fatal error which renders the domain functionally useless.
>
> In the case that listen() fails with EADDRINUSE, rebind the socket again and
> try for the next port.
Does this do the right thing if vncunused=0? In that case the user has
given a specific port which we want to bind to, so going to the next
port is not desired.
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> ---
>
> Changes in v2:
> * Fix commit message.
>
> This fix has been in XenServer for two and a half years
> ---
> qemu-sockets.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 9b9fa77..e54f6ad 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -163,6 +163,7 @@ int inet_listen(const char *str, char *ostr, int olen,
>
> /* create socket + bind */
> for (e = res; e != NULL; e = e->ai_next) {
> + rebind:
> getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
> uaddr,INET6_ADDRSTRLEN,uport,32,
> NI_NUMERICHOST | NI_NUMERICSERV);
> @@ -186,7 +187,20 @@ int inet_listen(const char *str, char *ostr, int olen,
> if (sockets_debug)
> fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__,
> inet_strfamily(e->ai_family), uaddr, inet_getport(e));
> - goto listen;
> + if (listen(slisten,1) == 0) {
> + goto listened;
> + } else {
> + int err = errno;
> +
> + perror("listen");
> + closesocket(slisten);
> +
> + if (err == EADDRINUSE)
> + goto rebind;
Does this code actually try for the next port? The try_next check is
after the goto here (you can see it in the context below). Won't it just
try to rebind the same port again and again?
> + freeaddrinfo(res);
> + return -1;
> + }
> +
> }
> try_next = to && (inet_getport(e) <= to + port_offset);
> if (!try_next || sockets_debug)
> @@ -205,12 +219,7 @@ int inet_listen(const char *str, char *ostr, int olen,
> freeaddrinfo(res);
> return -1;
>
> -listen:
> - if (listen(slisten,1) != 0) {
> - perror("listen");
> - closesocket(slisten);
> - return -1;
> - }
> +listened:
> if (ostr) {
> if (e->ai_family == PF_INET6) {
> snprintf(ostr, olen, "[%s]:%d%s", uaddr,
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
2013-12-09 13:27 ` [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports Andrew Cooper
2013-12-09 13:36 ` Ian Campbell
@ 2013-12-09 15:23 ` Frediano Ziglio
2013-12-09 15:50 ` Andrew Cooper
2013-12-10 15:28 ` Ian Jackson
2 siblings, 1 reply; 9+ messages in thread
From: Frediano Ziglio @ 2013-12-09 15:23 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Stefano Stabellini, Ian Campbell, Ian Jackson
> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Andrew Cooper
> Sent: 09 December 2013 1:27 PM
> To: Xen-devel
> Cc: Andrew Cooper; Ian Jackson; Ian Campbell; Stefano Stabellini
> Subject: [Xen-devel] [PATCH v2 qemu-traditional] qemu: Fix race condition
> when opening ports
>
> Two Qemus can race to bind the same VNC port. It is valid for multiple
> bind()s on the same socket to succeed, but only the first listen() will
> succeed.
>
> In the case that two Qemus are starting at the same time, and both trying to
> grab the next free VNC port, the second one will fail with an EADDRINUSE,
> and bail with a fatal error which renders the domain functionally useless.
>
> In the case that listen() fails with EADDRINUSE, rebind the socket again and
> try for the next port.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Andrew, if you are upstream other people stuff you should at least give credit to the original author.
Frediano
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
2013-12-09 15:23 ` Frediano Ziglio
@ 2013-12-09 15:50 ` Andrew Cooper
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Cooper @ 2013-12-09 15:50 UTC (permalink / raw)
To: Frediano Ziglio; +Cc: Ian Jackson, Stefano Stabellini, Ian Campbell, Xen-devel
On 09/12/13 15:23, Frediano Ziglio wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Andrew Cooper
>> Sent: 09 December 2013 1:27 PM
>> To: Xen-devel
>> Cc: Andrew Cooper; Ian Jackson; Ian Campbell; Stefano Stabellini
>> Subject: [Xen-devel] [PATCH v2 qemu-traditional] qemu: Fix race condition
>> when opening ports
>>
>> Two Qemus can race to bind the same VNC port. It is valid for multiple
>> bind()s on the same socket to succeed, but only the first listen() will
>> succeed.
>>
>> In the case that two Qemus are starting at the same time, and both trying to
>> grab the next free VNC port, the second one will fail with an EADDRINUSE,
>> and bail with a fatal error which renders the domain functionally useless.
>>
>> In the case that listen() fails with EADDRINUSE, rebind the socket again and
>> try for the next port.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> Andrew, if you are upstream other people stuff you should at least give credit to the original author.
>
> Frediano
>
The original author was "xenhg" according to the history. It is not even
clear from the original bug ticket who the author could have been.
In this case, my Signed-off-by line falls into category b) of the
Developer’s Certificate of Origin.
This is an unfortunate problem with attempting to weed through a
neglected patch queue where proper authorship was not considered at the
time of commit.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
2013-12-09 13:27 ` [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports Andrew Cooper
2013-12-09 13:36 ` Ian Campbell
2013-12-09 15:23 ` Frediano Ziglio
@ 2013-12-10 15:28 ` Ian Jackson
2013-12-10 15:38 ` Frediano Ziglio
2 siblings, 1 reply; 9+ messages in thread
From: Ian Jackson @ 2013-12-10 15:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Stefano Stabellini, Ian Campbell, Xen-devel
Andrew Cooper writes ("[PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports"):
> Two Qemus can race to bind the same VNC port. It is valid for multiple
> bind()s on the same socket to succeed, but only the first listen() will
> succeed.
I don't think this is correct. The SuS page for listen(2)
http://pubs.opengroup.org/onlinepubs/9699919799/functions/listen.html
doesn't list EADDRINUSE as a possible error condition.
Although wheezy's listen(2) manpage does.
> In the case that two Qemus are starting at the same time, and both trying to
> grab the next free VNC port, the second one will fail with an EADDRINUSE, and
> bail with a fatal error which renders the domain functionally useless.
>
> In the case that listen() fails with EADDRINUSE, rebind the socket again and
> try for the next port.
My preliminary view is that this is a kernel bug.
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
2013-12-10 15:28 ` Ian Jackson
@ 2013-12-10 15:38 ` Frediano Ziglio
0 siblings, 0 replies; 9+ messages in thread
From: Frediano Ziglio @ 2013-12-10 15:38 UTC (permalink / raw)
To: Ian Jackson, Andrew Cooper; +Cc: Stefano Stabellini, Ian Campbell, Xen-devel
>
> Andrew Cooper writes ("[PATCH v2 qemu-traditional] qemu: Fix race
> condition when opening ports"):
> > Two Qemus can race to bind the same VNC port. It is valid for
> > multiple bind()s on the same socket to succeed, but only the first
> > listen() will succeed.
>
> I don't think this is correct. The SuS page for listen(2)
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/listen.html
> doesn't list EADDRINUSE as a possible error condition.
>
> Although wheezy's listen(2) manpage does.
>
> > In the case that two Qemus are starting at the same time, and both
> > trying to grab the next free VNC port, the second one will fail with
> > an EADDRINUSE, and bail with a fatal error which renders the domain
> functionally useless.
> >
> > In the case that listen() fails with EADDRINUSE, rebind the socket
> > again and try for the next port.
>
> My preliminary view is that this is a kernel bug.
>
> Ian.
>
My Ubuntu "man 2 listen" reports
EADDRINUSE
Another socket is already listening on the same port.
(same from http://man7.org/linux/man-pages/man2/listen.2.html)
Frediano
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-12-10 15:38 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-09 13:10 [PATCH qemu-traditional] qemu: Fix race condition when binding ports Andrew Cooper
2013-12-09 13:17 ` George Dunlap
2013-12-09 13:25 ` Andrew Cooper
2013-12-09 13:27 ` [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports Andrew Cooper
2013-12-09 13:36 ` Ian Campbell
2013-12-09 15:23 ` Frediano Ziglio
2013-12-09 15:50 ` Andrew Cooper
2013-12-10 15:28 ` Ian Jackson
2013-12-10 15:38 ` Frediano Ziglio
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).