| Message ID | 20170428121553.22408-1-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/28/2017 07:15 AM, Daniel P. Berrange wrote: > When binding to an IPv6 socket we currently force the > IPV6_V6ONLY flag to off. This means that the IPv6 socket > will accept both IPv4 & IPv6 sockets when QEMU is launched > with something like > > -vnc :::1 At first glance, I thought that was too many :. But I guess it is parsed as host:screen, with host of '::', and port offset of '1'... > > While this is good for that case, it is bad for other > cases. For example if an empty hostname is given, > getaddrinfo resolves it to 2 addresses 0.0.0.0 and ::, > in that order. We will thus bind to 0.0.0.0 first, and > then fail to bind to :: on the same port. The same > problem can happen if any other hostname lookup causes > the IPv4 address to be reported before the IPv6 address. > > When we get an IPv6 bind failure, we should re-try the > same port, but with IPV6_V6ONLY turned on again, to > avoid clash with any IPv4 listener. > > This ensures that > > -vnc :1 ...and matches this being parsed as host of '' and port offset of '1' > > will bind successfully to both 0.0.0.0 and ::, and also > avoid > > -vnc :1,to=2 > > from mistakenly using a 2nd port for the :: listener. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > util/qemu-sockets.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
On Fri, Apr 28, 2017 at 09:57:24AM -0500, Eric Blake wrote: > On 04/28/2017 07:15 AM, Daniel P. Berrange wrote: > > When binding to an IPv6 socket we currently force the > > IPV6_V6ONLY flag to off. This means that the IPv6 socket > > will accept both IPv4 & IPv6 sockets when QEMU is launched > > with something like > > > > -vnc :::1 > > At first glance, I thought that was too many :. But I guess it is > parsed as host:screen, with host of '::', and port offset of '1'... Yeah, I was amazed when it actually worked in fact :-) I thought I was going to have to use {::}:1, but it seems we reverse search for the last ':'. > > > > > While this is good for that case, it is bad for other > > cases. For example if an empty hostname is given, > > getaddrinfo resolves it to 2 addresses 0.0.0.0 and ::, > > in that order. We will thus bind to 0.0.0.0 first, and > > then fail to bind to :: on the same port. The same > > problem can happen if any other hostname lookup causes > > the IPv4 address to be reported before the IPv6 address. > > > > When we get an IPv6 bind failure, we should re-try the > > same port, but with IPV6_V6ONLY turned on again, to > > avoid clash with any IPv4 listener. > > > > This ensures that > > > > -vnc :1 > > ...and matches this being parsed as host of '' and port offset of '1' > > > > > will bind successfully to both 0.0.0.0 and ::, and also > > avoid > > > > -vnc :1,to=2 > > > > from mistakenly using a 2nd port for the :: listener. > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > util/qemu-sockets.c | 30 ++++++++++++++++++++++-------- > > 1 file changed, 22 insertions(+), 8 deletions(-) > > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Fri, Apr 28, 2017 at 09:57:24AM -0500, Eric Blake wrote: >> On 04/28/2017 07:15 AM, Daniel P. Berrange wrote: >> > When binding to an IPv6 socket we currently force the >> > IPV6_V6ONLY flag to off. This means that the IPv6 socket >> > will accept both IPv4 & IPv6 sockets when QEMU is launched >> > with something like >> > >> > -vnc :::1 >> >> At first glance, I thought that was too many :. But I guess it is >> parsed as host:screen, with host of '::', and port offset of '1'... > > Yeah, I was amazed when it actually worked in fact :-) I thought > I was going to have to use {::}:1, but it seems we reverse search > for the last ':'. I would've expected we need ":[::1]". Too many ad hoc parsers... [...]
On Fri, Apr 28, 2017 at 01:15:53PM +0100, Daniel P. Berrange wrote: > When binding to an IPv6 socket we currently force the > IPV6_V6ONLY flag to off. This means that the IPv6 socket > will accept both IPv4 & IPv6 sockets when QEMU is launched > with something like > > -vnc :::1 > > While this is good for that case, it is bad for other > cases. For example if an empty hostname is given, > getaddrinfo resolves it to 2 addresses 0.0.0.0 and ::, > in that order. We will thus bind to 0.0.0.0 first, and > then fail to bind to :: on the same port. The same > problem can happen if any other hostname lookup causes > the IPv4 address to be reported before the IPv6 address. > > When we get an IPv6 bind failure, we should re-try the > same port, but with IPV6_V6ONLY turned on again, to > avoid clash with any IPv4 listener. > > This ensures that > > -vnc :1 > > will bind successfully to both 0.0.0.0 and ::, and also > avoid > > -vnc :1,to=2 > > from mistakenly using a 2nd port for the :: listener. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > util/qemu-sockets.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) Ping Paolo or Gerd - any comments on this patch ? Separately from this patch, I noticed one further possible problem, Currently, the ipv4=on|off/ipv6=on|off settings are only used to determine what getaddrinfo results we request/use. This leads to the somewhat odd situation where if you set ipv4=off,ipv6=on, QEMU won't be listening on an IPv4 socket, but *will still* accept IPv4 clients over the IPv6 socket due to our use of IPV6_V6ONLY=off. So I'm thinking that when ipv4=off, we should in fact set IPV6_V6ONLY=on. My fear though is that this is a semantic change that could cause regression. ie someone might be accidentally relying on fact that ipv4=off,ipv6=on still lets IPv4 clients connection, despite it being a somewhat nonsensical scenario > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index 8188d9a..75d1e0f 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -207,22 +207,36 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > } > > socket_set_fast_reuse(slisten); > -#ifdef IPV6_V6ONLY > - if (e->ai_family == PF_INET6) { > - /* listen on both ipv4 and ipv6 */ > - const int off = 0; > - qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &off, > - sizeof(off)); > - } > -#endif > > port_min = inet_getport(e); > port_max = saddr->has_to ? saddr->to + port_offset : port_min; > for (p = port_min; p <= port_max; p++) { > inet_setport(e, p); > +#ifdef IPV6_V6ONLY > + if (e->ai_family == PF_INET6) { > + /* listen on both ipv4 and ipv6 */ > + const int off = 0; > + qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &off, > + sizeof(off)); > + } > +#endif > if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { > goto listen; > } > + > +#ifdef IPV6_V6ONLY > + if (e->ai_family == PF_INET6 && errno == EADDRINUSE) { > + /* listen on only ipv6 */ > + const int on = 1; > + qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &on, > + sizeof(on)); > + > + if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { > + goto listen; > + } > + } > +#endif > + > if (p == port_max) { > if (!e->ai_next) { > error_setg_errno(errp, errno, "Failed to bind socket"); > -- > 2.9.3 > Regards, Daniel
Hi, > Separately from this patch, I noticed one further possible > problem, > > Currently, the ipv4=on|off/ipv6=on|off settings are only > used to determine what getaddrinfo results we request/use. > > This leads to the somewhat odd situation where if you set > ipv4=off,ipv6=on, QEMU won't be listening on an IPv4 > socket, but *will still* accept IPv4 clients over the IPv6 > socket due to our use of IPV6_V6ONLY=off. Hmm, maybe we should just use IPV6_V6ONLY=on unconditionally? Now that qemu finally supports multiple listening sockets we should be able to do the switch without regressions, and it also should remove all those nasty corner cases ... cheers, Gerd
On Tue, May 16, 2017 at 03:29:51PM +0200, Gerd Hoffmann wrote: > Hi, > > > Separately from this patch, I noticed one further possible > > problem, > > > > Currently, the ipv4=on|off/ipv6=on|off settings are only > > used to determine what getaddrinfo results we request/use. > > > > This leads to the somewhat odd situation where if you set > > ipv4=off,ipv6=on, QEMU won't be listening on an IPv4 > > socket, but *will still* accept IPv4 clients over the IPv6 > > socket due to our use of IPV6_V6ONLY=off. > > Hmm, maybe we should just use IPV6_V6ONLY=on unconditionally? This would certainly be my long term desire... > Now that qemu finally supports multiple listening sockets we should be > able to do the switch without regressions, and it also should remove all > those nasty corner cases ... ...but we can't do it yet - only the VNC server code has been reworked to support multiple listening sockets. We'd still have work todo on the NBD server, chardevs, migration, network socket backend and guest agent to support multiple listeners. Regards, Daniel
On Di, 2017-05-16 at 14:35 +0100, Daniel P. Berrange wrote: > On Tue, May 16, 2017 at 03:29:51PM +0200, Gerd Hoffmann wrote: > > Hi, > > > > > Separately from this patch, I noticed one further possible > > > problem, > > > > > > Currently, the ipv4=on|off/ipv6=on|off settings are only > > > used to determine what getaddrinfo results we request/use. > > > > > > This leads to the somewhat odd situation where if you set > > > ipv4=off,ipv6=on, QEMU won't be listening on an IPv4 > > > socket, but *will still* accept IPv4 clients over the IPv6 > > > socket due to our use of IPV6_V6ONLY=off. > > > > Hmm, maybe we should just use IPV6_V6ONLY=on unconditionally? > > This would certainly be my long term desire... > > > Now that qemu finally supports multiple listening sockets we should be > > able to do the switch without regressions, and it also should remove all > > those nasty corner cases ... > > ...but we can't do it yet - only the VNC server code has been reworked > to support multiple listening sockets. We'd still have work todo on the > NBD server, chardevs, migration, network socket backend and guest agent > to support multiple listeners. Ah, ok. I guess then I would not worry too much that we break stuff in case ipv4=off actually does what it is supposed to do ... cheers, Gerd
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 8188d9a..75d1e0f 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -207,22 +207,36 @@ static int inet_listen_saddr(InetSocketAddress *saddr, } socket_set_fast_reuse(slisten); -#ifdef IPV6_V6ONLY - if (e->ai_family == PF_INET6) { - /* listen on both ipv4 and ipv6 */ - const int off = 0; - qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &off, - sizeof(off)); - } -#endif port_min = inet_getport(e); port_max = saddr->has_to ? saddr->to + port_offset : port_min; for (p = port_min; p <= port_max; p++) { inet_setport(e, p); +#ifdef IPV6_V6ONLY + if (e->ai_family == PF_INET6) { + /* listen on both ipv4 and ipv6 */ + const int off = 0; + qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &off, + sizeof(off)); + } +#endif if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { goto listen; } + +#ifdef IPV6_V6ONLY + if (e->ai_family == PF_INET6 && errno == EADDRINUSE) { + /* listen on only ipv6 */ + const int on = 1; + qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, &on, + sizeof(on)); + + if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { + goto listen; + } + } +#endif + if (p == port_max) { if (!e->ai_next) { error_setg_errno(errp, errno, "Failed to bind socket");
When binding to an IPv6 socket we currently force the IPV6_V6ONLY flag to off. This means that the IPv6 socket will accept both IPv4 & IPv6 sockets when QEMU is launched with something like -vnc :::1 While this is good for that case, it is bad for other cases. For example if an empty hostname is given, getaddrinfo resolves it to 2 addresses 0.0.0.0 and ::, in that order. We will thus bind to 0.0.0.0 first, and then fail to bind to :: on the same port. The same problem can happen if any other hostname lookup causes the IPv4 address to be reported before the IPv6 address. When we get an IPv6 bind failure, we should re-try the same port, but with IPV6_V6ONLY turned on again, to avoid clash with any IPv4 listener. This ensures that -vnc :1 will bind successfully to both 0.0.0.0 and ::, and also avoid -vnc :1,to=2 from mistakenly using a 2nd port for the :: listener. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- util/qemu-sockets.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)