network: fix dnsmasq/radvd binding to IPv6 on recent kernels

I hit this problem recently when trying to create a bridge with an IPv6
address on a 3.2 kernel: dnsmasq (and, further, radvd) would not bind to
the given address, waiting 20s and then giving up with -EADDRNOTAVAIL
(resp. exiting immediately with "error parsing or activating the config
file", without libvirt noticing it, BTW). This can be reproduced with (I
think) any kernel >= 2.6.39 and the following XML (to be used with
"virsh net-create"):

        <network>
          <name>test-bridge</name>
          <bridge name='testbr0' />
          <ip family='ipv6' address='fd00::1' prefix='64'>
          </ip>
        </network>

(it happens even when you have an IPv4, too)

The problem is that since commit [1] (which, ironically, was made to
“help IPv6 autoconfiguration”) the linux bridge code makes bridges
behave like “real” devices regarding carrier detection. This makes the
bridges created by libvirt, which are started without any up devices,
stay with the NO-CARRIER flag set, and thus prevents DAD (Duplicate
address detection) from happening, thus letting the IPv6 address flagged
as “tentative”. Such addresses cannot be bound to (see RFC 2462), so
dnsmasq fails binding to it (for radvd, it detects that "interface XXX
is not RUNNING", thus that "interface XXX does not exist, ignoring the
interface" (sic)). It seems that this behavior was enhanced somehow with
commit [2] by avoiding setting NO-CARRIER on empty bridges, but I
couldn't reproduce this behavior on my kernel. Anyway, with the “dummy
tap to set MAC address” trick, this wouldn't work.

To fix this, the idea is to get the bridge's attached device to be up so
that DAD can happen (deactivating DAD altogether is not a good idea, I
think). Currently, libvirt creates a dummy TAP device to set the MAC
address of the bridge, keeping it down. But even if we set this device
up, it is not RUNNING as soon as the tap file descriptor attached to it
is closed, thus still preventing DAD. So, we must modify the API a bit,
so that we can get the fd, keep the tap device persistent, run the
daemons, and close it after DAD has taken place. After that, the bridge
will be flagged NO-CARRIER again, but the daemons will be running, even
if not happy about the device's state (but we don't really care about
the bridge's daemons doing anything when no up interface is connected to
it).

Other solutions that I envisioned were:
      * Keeping the *-nic interface up: this would waste an fd for each
        bridge during all its life. May be acceptable, I don't really
        know.
      * Stop using the dummy tap trick, and set the MAC address directly
        on the bridge: it is possible since quite some time it seems,
        even if then there is the problem of the bridge not being
        RUNNING when empty, contrary to what [2] says, so this will need
        fixing (and this fix only happened in 3.1, so it wouldn't work
        for 2.6.39)
      * Using the --interface option of dnsmasq, but I saw somewhere
        that it's not used by libvirt for backward compatibility. I am
        not sure this would solve this problem, though, as I don't know
        how dnsmasq binds itself to it with this option.

This is why this patch does what's described earlier.

This patch also makes radvd start even if the interface is
“missing” (i.e. it is not RUNNING), as it daemonizes before binding to
it, and thus sometimes does it after the interface has been brought down
by us (by closing the tap fd), and then originally stops. This also
makes it stop yelling about it in the logs when the interface is down at
a later time.

[1]
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=1faa4356a3bd89ea11fb92752d897cff3a20ec0e
[2]
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=b64b73d7d0c480f75684519c6134e79d50c1b341

5 files changed, 42 insertions, 16 deletions

diff --git a/.mailmap b/.mailmap
index 3ec5a3f5e..76e651313 100644
--- a/.mailmap
+++ b/.mailmap
@@ -33,6 +33,7 @@
 <josh.durgin@inktank.com> <josh.durgin@dreamhost.com>
 <gerd@egidy.de> <lists@egidy.de>
 <gerd@egidy.de> <gerd.von.egidy@intra2net.com>
+<benoar@dolka.fr> <benjamin.cama@telecom-bretagne.eu>
 
 # Name consolidation:
 # Preferred author spelling <preferred email>
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6e260f774..e1846ee6d 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -65,6 +65,7 @@
 #include "virnetdevtap.h"
 #include "virnetdevvportprofile.h"
 #include "virdbus.h"
+#include "virfile.h"
 
 #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network"
 #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network"
@@ -987,12 +988,15 @@ networkRadvdConfContents(virNetworkObjPtr network, char **configstr)
 
     *configstr = NULL;
 
-    /* create radvd config file appropriate for this network */
+    /* create radvd config file appropriate for this network;
+     * IgnoreIfMissing allows radvd to start even when the bridge is down
+     */
     virBufferAsprintf(&configbuf, "interface %s\n"
                       "{\n"
                       "  AdvSendAdvert on;\n"
                       "  AdvManagedFlag off;\n"
                       "  AdvOtherConfigFlag off;\n"
+                      "  IgnoreIfMissing on;\n"
                       "\n",
                       network->def->bridge);
 
@@ -2061,6 +2065,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
     virErrorPtr save_err = NULL;
     virNetworkIpDefPtr ipdef;
     char *macTapIfName = NULL;
+    int tapfd = -1;
 
     /* Check to see if any network IP collides with an existing route */
     if (networkCheckRouteCollision(network) < 0)
@@ -2082,10 +2087,13 @@ networkStartNetworkVirtual(struct network_driver *driver,
             virReportOOMError();
             goto err0;
         }
+        /* Keep tun fd open and interface up to allow for IPv6 DAD to happen */
         if (virNetDevTapCreateInBridgePort(network->def->bridge,
                                            &macTapIfName, &network->def->mac,
-                                           NULL, NULL, NULL, NULL,
-                                           VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE) < 0) {
+                                           NULL, &tapfd, NULL, NULL,
+                                           VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE |
+                                           VIR_NETDEV_TAP_CREATE_IFUP |
+                                           VIR_NETDEV_TAP_CREATE_PERSIST) < 0) {
             VIR_FREE(macTapIfName);
             goto err0;
         }
@@ -2149,6 +2157,15 @@ networkStartNetworkVirtual(struct network_driver *driver,
     if (v6present && networkStartRadvd(network) < 0)
         goto err4;
 
+    /* DAD has happened (dnsmasq waits for it), dnsmasq is now bound to the
+     * bridge's IPv6 address, so we can now set the dummy tun down.
+     */
+    if (tapfd >= 0) {
+        if (virNetDevSetOnline(macTapIfName, false) < 0)
+            goto err4;
+        VIR_FORCE_CLOSE(tapfd);
+    }
+
     if (virNetDevBandwidthSet(network->def->bridge, network->def->bandwidth) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("cannot set bandwidth limits on %s"),
@@ -2187,6 +2204,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
         save_err = virSaveLastError();
 
     if (macTapIfName) {
+        VIR_FORCE_CLOSE(tapfd);
         ignore_value(virNetDevTapDelete(macTapIfName));
         VIR_FREE(macTapIfName);
     }
@@ -2887,8 +2905,8 @@ networkUpdate(virNetworkPtr net,
      * is active, else change CONFIG
     */
     isActive = virNetworkObjIsActive(network);
-    if ((flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE
-                   | VIR_NETWORK_UPDATE_AFFECT_CONFIG)) ==
+    if ((flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE |
+                  VIR_NETWORK_UPDATE_AFFECT_CONFIG)) ==
         VIR_NETWORK_UPDATE_AFFECT_CURRENT) {
         if (isActive)
             flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE;
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index a317bcc66..e8b877952 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -142,7 +142,8 @@ umlConnectTapDevice(virConnectPtr conn,
                                        vm->uuid, NULL,
                                        virDomainNetGetActualVirtPortProfile(net),
                                        virDomainNetGetActualVlan(net),
-                                       VIR_NETDEV_TAP_CREATE_IFUP) < 0) {
+                                       VIR_NETDEV_TAP_CREATE_IFUP |
+                                       VIR_NETDEV_TAP_CREATE_PERSIST) < 0) {
         if (template_ifname)
             VIR_FREE(net->ifname);
         goto error;
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 9f2dca17f..0eadd6cf2 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -112,18 +112,20 @@ virNetDevProbeVnetHdr(int tapfd)
  *
  *   VIR_NETDEV_TAP_CREATE_VNET_HDR
  *     - Enable IFF_VNET_HDR on the tap device
+ *   VIR_NETDEV_TAP_CREATE_PERSIST
+ *     - The device will persist after the file descriptor is closed
  *
  * Creates a tap interface.
- * If the @tapfd parameter is supplied, the open tap device file
- * descriptor will be returned, otherwise the TAP device will be made
- * persistent and closed. The caller must use virNetDevTapDelete to
- * remove a persistent TAP devices when it is no longer needed.
+ * If the @tapfd parameter is supplied, the open tap device file descriptor
+ * will be returned, otherwise the TAP device will be closed. The caller must
+ * use virNetDevTapDelete to remove a persistent TAP device when it is no
+ * longer needed.
  *
  * Returns 0 in case of success or -1 on failure.
  */
 int virNetDevTapCreate(char **ifname,
                        int *tapfd,
-                       unsigned int flags ATTRIBUTE_UNUSED)
+                       unsigned int flags)
 {
     int fd;
     struct ifreq ifr;
@@ -160,7 +162,7 @@ int virNetDevTapCreate(char **ifname,
         goto cleanup;
     }
 
-    if (!tapfd &&
+    if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) &&
         (errno = ioctl(fd, TUNSETPERSIST, 1))) {
         virReportSystemError(errno,
                              _("Unable to set tap device %s to persistent"),
@@ -261,14 +263,16 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED)
  *     - Enable IFF_VNET_HDR on the tap device
  *   VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE
  *     - Set this interface's MAC as the bridge's MAC address
+ *   VIR_NETDEV_TAP_CREATE_PERSIST
+ *     - The device will persist after the file descriptor is closed
  *
  * This function creates a new tap device on a bridge. @ifname can be either
  * a fixed name or a name template with '%d' for dynamic name allocation.
  * in either case the final name for the bridge will be stored in @ifname.
- * If the @tapfd parameter is supplied, the open tap device file
- * descriptor will be returned, otherwise the TAP device will be made
- * persistent and closed. The caller must use virNetDevTapDelete to remove
- * a persistent TAP devices when it is no longer needed.
+ * If the @tapfd parameter is supplied, the open tap device file descriptor
+ * will be returned, otherwise the TAP device will be closed. The caller must
+ * use virNetDevTapDelete to remove a persistent TAP device when it is no
+ * longer needed.
  *
  * Returns 0 in case of success or -1 on failure
  */
diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index fa6a647a0..980db61e5 100644
--- a/src/util/virnetdevtap.h
+++ b/src/util/virnetdevtap.h
@@ -43,6 +43,8 @@ typedef enum {
    VIR_NETDEV_TAP_CREATE_VNET_HDR           = 1 << 1,
    /* Set this interface's MAC as the bridge's MAC address */
    VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE = 1 << 2,
+   /* The device will persist after the file descriptor is closed */
+   VIR_NETDEV_TAP_CREATE_PERSIST            = 1 << 3,
 } virNetDevTapCreateFlags;
 
 int virNetDevTapCreateInBridgePort(const char *brname,