[Qemu-devel] [PATCH] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Qemu-devel] [PATCH] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Daniel P. Berrange]

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(-)

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

Re: [Qemu-devel] [PATCH] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Daniel P. Berrange]

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
-- 
|: 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] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Markus Armbruster]

"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...

[...]

Re: [Qemu-devel] [PATCH] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Daniel P. Berrange]

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
-- 
|: 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] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Gerd Hoffmann]

  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

Re: [Qemu-devel] [PATCH] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Daniel P. Berrange]

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
-- 
|: 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] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Gerd Hoffmann]

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