Andrew Cooper
2013-Dec-09 13:10 UTC
[PATCH qemu-traditional] qemu: Fix race condition when binding ports
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
George Dunlap
2013-Dec-09 13:17 UTC
Re: [PATCH qemu-traditional] qemu: Fix race condition when binding ports
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,
Andrew Cooper
2013-Dec-09 13:25 UTC
Re: [PATCH qemu-traditional] qemu: Fix race condition when binding ports
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
Andrew Cooper
2013-Dec-09 13:27 UTC
[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>
---
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
Ian Campbell
2013-Dec-09 13:36 UTC
Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
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,
Frediano Ziglio
2013-Dec-09 15:23 UTC
Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
> -----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
Andrew Cooper
2013-Dec-09 15:50 UTC
Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
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
Ian Jackson
2013-Dec-10 15:28 UTC
Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
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.
Frediano Ziglio
2013-Dec-10 15:38 UTC
Re: [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports
> > 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